Re: [PATCH v2 2/3] x86/ucode: Fix cache handling in microcode_update_helper()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2024 22:19, Andrew Cooper wrote:
> microcode_update_cache() now has a single caller, but inlining it shows how
> unnecessarily complicated the logic really is.
> 
> Outside of error paths, there is always one microcode patch to free.  Its
> either result of parse_blob(), or it's the old cached value.
> 
> In order to fix this, have a local patch pointer (mostly to avoid the
> unnecessary verbosity of patch_with_flags.patch), and always free it at the
> end.  The only error path needing care is the IS_ERR(patch) path, which is
> easy enough to handle.
> 
> Also, widen the scope of result.  We only need to call compare_patch() once,
> and the answer is still good later when updating the cache.  In order to
> update the cache, simply SWAP() the patch and the cache pointers, allowing the
> singular xfree() at the end to cover both cases.
> 
> This also removes all callers microcode_free_patch() which fixes the need to
> cast away const to allow it to compile.

I'm sure you're well aware that this in turn is just because of your opposition
to xfree() and alike taking const void *. Pointers needing to be to non-const
just because of eventual freeing is precisely the scenario why freeing (and
unmapping) functions better wouldn't take mutable pointers. Then ...

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -86,7 +86,7 @@ struct patch_with_flags {
>  static bool ucode_in_nmi = true;
>  
>  /* Protected by microcode_mutex */
> -static const struct microcode_patch *microcode_cache;
> +static struct microcode_patch *microcode_cache;

... this imo pretty undesirable change also wouldn't be needed.

Nevertheless, in the interest of not blocking this change over a long-standing
disagreement we have,
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan