Re: [PATCH 07/10] x86/ucode: Move the CPIO path string into microcode_ops

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/2024 2:25 pm, Jan Beulich wrote:
> On 28.10.2024 10:18, Andrew Cooper wrote:
>> We've got a perfectly good vendor abstraction already for microcode.  No need
>> for a second ad-hoc one in microcode_scan_module().
>>
>> This is in preparation to use ucode_ops.cpio_path in multiple places.
>>
>> These paths are only used during __init, so take the opportunity to move them
>> into __initconst.
> As an alternative to this, how about ...
>
>> --- a/xen/arch/x86/cpu/microcode/private.h
>> +++ b/xen/arch/x86/cpu/microcode/private.h
>> @@ -59,6 +59,13 @@ struct microcode_ops {
>>       */
>>      enum microcode_match_result (*compare_patch)(
>>          const struct microcode_patch *new, const struct microcode_patch 
>> *old);
>> +
>> +    /*
>> +     * For Linux inird microcode compatibliity.
>> +     *
>> +     * The path where this vendor's microcode can be found in CPIO.
>> +     */
>> +    const char *cpio_path;
>     const char cpio_path[];
>
> inheriting the __initconst from the struct instances?
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> with a slight preference to the form without the extra pointer.

I'm slightly surprised at this request, given that the form with the
pointer results in less data held at runtime.


>  Except that:
> gcc14 looks to be buggy when it comes to the copying of such a struct. The
> example below yields an internal compiler error. And the direct structure
> assignment also doesn't quite do what I would expect it to do (visible when
> commenting out the "else" branch. Bottom line - leave the code as is.

It's unfortunate to hit an ICE, but the copy cannot possibly work in the
first place.

ucode_ops is in a separate translation unit and has no space allocated
after the flexible member.   Any copy into it is memory corruption of
whatever object happens to be sequentially after ucode_ops.

The only way it would work is having `const char cpio_path[40];` which
is long enough for anything we'd expect to find.

But again, that involves holding init-only data post init.

~Andrew