Re: [PATCH 4/9] x86: Merge struct msr_policy into struct cpu_policy

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2023 13:11, Andrew Cooper wrote:
> On 30/03/2023 10:30 am, Jan Beulich wrote:
>> On 29.03.2023 22:51, Andrew Cooper wrote:
>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>> @@ -3,7 +3,6 @@
>>>  #define XEN_LIB_X86_POLICIES_H
>>>  
>>>  #include <xen/lib/x86/cpuid-autogen.h>
>>> -#include <xen/lib/x86/msr.h>
>>>  
>>>  #define FEATURESET_1d     0 /* 0x00000001.edx      */
>>>  #define FEATURESET_1c     1 /* 0x00000001.ecx      */
>>> @@ -107,6 +106,9 @@ const char *x86_cpuid_vendor_to_str(unsigned int 
>>> vendor);
>>>       CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE +  \
>>>       CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )
>>>  
>>> +/* Maximum number of MSRs written when serialising msr_policy. */
>>> +#define MSR_MAX_SERIALISED_ENTRIES 2
>> The comment better wouldn't refer to msr_policy anymore, I think.
> 
> Ah yes.  (There's so much comment and library cleanup still to do)
> 
>>  I also
>> wonder whether the comment wouldn't better move ...
>>
>>> @@ -324,6 +326,44 @@ struct cpu_policy
>>>          };
>>>      } extd;
>>>  
>>> +    /*
>>> +     * 0x000000ce - MSR_INTEL_PLATFORM_INFO
>> ... e.g. above here, to increase the chance of it being spotted that
>> it needs updating if another MSR is added here.
> 
> I'm not sure about that.  In its current position, it's next to it's
> CPUID partner.
> 
> The unit tests in test-cpu-policy cross-check that we never get -ENOBUFS
> for a MSR_MAX_* sized destination, so I'm not worried about it actually
> getting stale.

Well, I wasn't worried so much about ending up with a bad commit, but
rather that something you could spot on the first pass while writing
new code might cause you a build or test failure later on, resulting
in extra hunting. But this was merely a thought, nothing I would insist
one (the more that your CPUID sibling argument is of course also
relevant).

Jan