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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.10.2024 15:38, Andrew Cooper wrote:
> 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.

No, it doesn't. Yet I only now realize that ...

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

... my expectation of how the copy ought to work (and how the C standard,
at least in close enough an example, specifies it) would specifically _not_
suit our needs. The copy ought to only cover sizeof(struct ...), i.e. not
the string. Yet we'd need that string to be copied to be usable for our
purposes.

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

This, indeed, would increase post-init size. Yet with the compiler issue
no question arises anyway as to how this needs doing.

Jan