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