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 <[email protected]>
> 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
