Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/03/2020 14:16, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>> separately allocated struct microcode_intel.  This is wasteful.
> As indicated elsewhere I'm very much in favor of this, but I think it
> wants doing in one of the earlier series, and then for AMD at the same
> time.

I've got some ideas for an AMD series given the replies I got, and will
be able to do an equivalent microcode_amd => microcode_patch folding on
that side.  There is also further work to do, including unbreaking the
OSVW logic (which has been totally clobbered by the start/end_update
debacle).

However, given that it taken this whole series to make the transition
look safe on the Intel side, I really don't see a way of doing this
"earlier".

In particular, no amount of ifdefary suggested below can AFAICT make it
safe to do this transform without having microcode_patch opaque to being
with.

Yes - there is a bit of churn, but I can't see a safe alternative.

~Andrew

>  Possibly, to limit code churn there, ...
>
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -32,17 +32,12 @@
>>  
>>  #define pr_debug(x...) ((void)0)
>>  
>> -struct microcode_header_intel {
>> +struct microcode_patch {
> ... accompanying this with
>
> #define microcode_header_intel microcode_patch
>
> or even ...
>
>> -    union {
>> -        struct {
>> -            uint16_t year;
>> -            uint8_t day;
>> -            uint8_t month;
>> -        };
>> -        unsigned int date;
>> -    };
>> +    uint16_t year;
>> +    uint8_t  day;
>> +    uint8_t  month;
>>      unsigned int sig;
>>      unsigned int cksum;
>>      unsigned int ldrver;
>> @@ -57,10 +52,7 @@ struct microcode_header_intel {
>>      unsigned int _datasize;
>>      unsigned int _totalsize;
>>      unsigned int reserved[3];
>> -};
>>  
>> -struct microcode_intel {
>> -    struct microcode_header_intel hdr;
>>      uint8_t data[];
>>  };
> ... keeping the two structures separate until here, which would
> make this one what would initially become struct microcode_patch.
> This is in particular because ...
>
>>  static void free_patch(struct microcode_patch *patch)
>>  {
>> -    if ( patch )
>> -    {
>> -        xfree(patch->mc_intel);
>> -        xfree(patch);
>> -    }
>> +    xfree(patch);
>>  }
> ... in that earlier series you've moved the 2nd xfree() here just
> to now delete it again.
>
> Jan