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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 4/9] x86: Merge struct msr_policy into struct cpu_policy
Date: Thu, 30 Mar 2023 12:11:17 +0100	[thread overview]
Message-ID: <bacda91d-a371-ae30-5ea8-19a452e54381@citrix.com> (raw)
In-Reply-To: <885610de-aea4-6aec-af73-28ee9147fb65@suse.com>

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.

But, with the new merged policies, I need to change the serialising
behaviour anyway.

Right now we serialise everything unconditionally because that's the
only way that merging incremental deltas could be made to work, but now
the MSR enumeration bits are in the same structure we can use those
instead, along with the various "clear" passes we already have.

~Andrew