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 <jbeul...@suse.com>
Jan
