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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/11/2024 11:26 am, Jan Beulich wrote:
> 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 *.

My opposition, and the C standards committee, and MISRA to name but a few.

>  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>

Thankyou.

~Andrew