linux-kernel - Re: [PATCH] kvm: x86: emulate MSR_PLATFORM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZOOMwvPd/Cz/cEmv@google.com>
Date:   Mon, 21 Aug 2023 09:11:46 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Hao Xiang <hao.xiang@...ux.alibaba.com>
Cc:     Chao Gao <chao.gao@...el.com>, kvm@...r.kernel.org,
        shannon.zhao@...ux.alibaba.com, pbonzini@...hat.com,
        linux-kernel@...r.kernel.org, Aaron Lewis <aaronlewis@...gle.com>
Subject: Re: [PATCH] kvm: x86: emulate MSR_PLATFORM_INFO msr bits

+Aaron

When resending a patch, e.g. to change To: or Cc:, tag it RESEND.  I got three
copies of this...

On Mon, Aug 21, 2023, Hao Xiang wrote:
> 
> 
> On 2023/8/21 18:44, Chao Gao wrote:
> > On Mon, Aug 21, 2023 at 05:11:16PM +0800, Hao Xiang wrote:
> > > For reason that,
> > > 
> > > The turbo frequency info depends on specific machine type. And the msr value
> > > of MSR_PLATFORM_INFO may be diferent on diffrent generation machine.
> > > 
> > > Get following msr bits (needed by turbostat on intel platform) by rdmsr
> > > MSR_PLATFORM_INFO directly in KVM is more reasonable. And set these msr bits
> > > as vcpu->arch.msr_platform_info default value.
> > > -bit 15:8, Maximum Non-Turbo Ratio (MAX_NON_TURBO_LIM_RATIO)
> > > -bit 47:40, Maximum Efficiency Ratio (MAX_EFFICIENCY_RATIO)
> > 
> > I don't get why QEMU cannot do this with the existing interface, e.g.,
> > KVM_SET_MSRS.
> > 
> > will the MSR value be migrated during VM migration?
> > 
> > looks we are in a dilemma. on one side, if the value is migrated, the value can
> > become inconsisntent with hardware value. On the other side, changing the ratio
> > bits at runtime isn't the architectural behavior.
> > 
> > And the MSR is per-socket. In theory, a system can have two sockets with
> > different values of the MSR. what if a vCPU is created on a socket and then
> > later runs on the other socket?
> > 
> 
> Set these msr bits (needed by turbostat on intel platform) in KVM by
> default.
> Of cource, QEMU can also set MSR value by need. It does not conflict.

It doesn't conflict per se, but it's still problematic.  By stuffing a default
value, KVM _forces_ userspace to override the MSR to align with the topology and
CPUID defined by userspace.  And if userspace uses KVM's "default" CPUID, or lack
thereof, using the underlying values from hardware are all but guaranteed to be
wrong.

The existing code that sets MSR_PLATFORM_INFO_CPUID_FAULT really should not exist,
i.e. KVM shouldn't shouldn't assume userspace wants to expose CPUID faulting to
the guest.  That particular one probably isn't worth trying to retroactively fix.

Ditto for setting MSR_IA32_ARCH_CAPABILITIES; KVM is overstepping, but doing so
likely doesn't cause problems.

MSR_IA32_PERF_CAPABILITIES is a different story.  Setting a non-zero default value
is blatantly wrong, as KVM will advertise vPMU features even if userspace doesn't
advertise.  Aaron is planning on sending a patch for this one (I'm hoping we can
get away with retroactively dropping the code without having to add a quirk).

*If* we need KVM to expose the ratios to userspace, then the correct way to do so
is handle turbo and efficiency ratio information is to by implementing support in
kvm_get_msr_feature(), i.e. KVM_GET_MSRS on /dev/kvm.  Emphasis on "if", because
I would prefer to do nothing in KVM if that information is already surfaced to
userspace through other mechanisms in the kernel.