Re: [RFC PATCH 01/22] x86/msr: MSR_PLATFORM_INFO shouldn't claim that turbo is programmable

[Edwin Torok <edwin.torok@xxxxxxxxx>]

> On 25 Oct 2023, at 21:33, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 25/10/2023 8:29 pm, Edwin Török wrote:
>> From: Edwin Török <edvin.torok@xxxxxxxxxx>
>> 
>> Xen forbids writes to the various turbo control MSRs, however 
>> MSR_PLATFORM_INFO claims that these MSRs are writable.
>> Override MSR_PLATFORM_INFO bits to indicate lack of support.
>> 
>> See Intel SDM Volume 4, 2.17.6 "MSRs Introduced in the Intel Xeon Scaslable 
>> Processor Family",
>> which describes that MSR_PLATFORM_INFO.[28] = 1 implies that 
>> MSR_TURBO_RATIO_LIMIT is R/W,
>> and similarly bit 29 for TDP control, and bit 30 for MSR_TEMPERATURE_TARGET.
>> 
>> These bits were not all present on earlier processors, however where missing 
>> the bits were reserved,
>> and when present they are always present in the same bits.
>> 
>> (Curiously bit 31 that Xen uses is not documented anywhere in this manual 
>> but a separate one).
>> 
>> Backport: 4.0+
>> 
>> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
> 
> p->platform_info never has any bit other than cpuid_faulting set in it. 
> We still don't even report the proper raw value, because we don't (yet)
> have clean MSR derivation logic.
> 
> I'm confused as to how you managed to find these set.  Even back in Xen
> 4.13, PLATFORM_INFO was covered by the msr_policy (later merged into
> cpu_policy).  Furthermore, even patch 3 oughtn't to have such an effect.
> 
> Sadly, the whole of this MSR is model specific.  Vol4 2.17 is only for
> one SKX/CLX/ICX/SPR.  Technically its wrong to treat the cpuid_faulting
> in the way we do, but it is enumerated separately, and we intentionally
> don't have an Intel check because we need to emulate CPUID faulting on
> AMD hardware to make PVShim work.
> 

xen/lib/x86/msr.c:    COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw);

This code made me believe that the underlying MSR value was copied, and only 
specific values from it were overwritten, I didn't spot any zeroing.
Although running rdmsr now (on 4.13.5) does show that all the other bits are 
zero, so the zeroing must happen somewhere else in the code, making this patch 
obsolete.
I'll drop it.

Thanks,
--Edwin

> ~Andrew