On 13.09.2023 13:02, Andrew Cooper wrote: > On 13/09/2023 7:18 am, Jan Beulich wrote: >> On 12.09.2023 18:35, Andrew Cooper wrote: >>> On 12/09/2023 5:23 pm, Roger Pau Monne wrote: >>>> --- a/xen/arch/x86/msr.c >>>> +++ b/xen/arch/x86/msr.c >>>> @@ -240,8 +240,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, >>>> uint64_t *val) >>>> case MSR_K8_HWCR: >>>> if ( !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) >>>> goto gp_fault; >>>> - *val = get_cpu_family(cp->basic.raw_fms, NULL, NULL) >= 0x10 >>>> - ? K8_HWCR_TSC_FREQ_SEL : 0; >>>> + /* >>>> + * OpenBSD 7.3 accesses HWCR unconditionally if the TSC is >>>> reported as >>>> + * Invariant. Do not set TSC_FREQ_SEL as that would trigger >>>> OpenBSD to >>>> + * also poke at PSTATE0. >>>> + */ >>> While this is true, the justification for removing this is because >>> TSC_FREQ_SEL is a model specific bit, not an architectural bit in HWCR. >>> >>> Also because it's addition without writing into the migration stream was >>> bogus irrespective of the specifics of the bit. >>> >>> I'm still of the opinion that it's buggy for OpenBSD to be looking at >>> model specific bits when virtualised, but given my latest reading of the >>> AMD manuals, I think OpenBSD *is* well behaved looking at PSTATE0 if it >>> can see TSC_FREQ_SEL. >> I'm afraid I'm still at a loss seeing what wording in which PPR makes you >> see a connection. If there was a connection, I'd like to ask that we at >> least properly consider exposing PSTATE0 (or if necessary all PSTATEn) as >> well, with zero value (i.e. in particular with the valid bit clear), >> rather than exposing a r/o bit with the wrong value, upsetting Linux. > > This mess is caused by the erroneous advertising of a model specific > bit, and there's no way in hell that giving the guest even more model > specific data is going to fix it. > > The PSTATE MSRs are entirely model specific, fully read/write, and the > Enable bit is not an enable bit; its a "not valid yet" bit that firmware > is required to adjust to be consistent across the coherency fabric. > > Linux is simply wrong with it's printk() under virt, and wants adjusting.
Assuming Roger agrees with this statement, then I think this is another item wanting mentioning in the description. I then still wouldn't ack the change, but I also wouldn't object to it anymore. Jan
