On Tue, Apr 12, 2022, Jue Wang wrote: > > > + > > > + if (!(mcg_cap & MCG_CMCI_P) && > > > + (data || !msr_info->host_initiated)) > > > > This looks wrong, userspace should either be able to write the MSR or not, '0' > > isn't special. Unless there's a danger to KVM, which I don't think there is, > > userspace should be allowed to ignore architectural restrictions, i.e. bypass > > the MCG_CMCI_P check, so that KVM doesn't create an unnecessary dependency between > > ioctls. I.e. this should be: > > I think the idea was writing unsupported values into an MSR should > cause #GP. Right, but when userspace is stuffing MSR values KVM only needs to prevent userspace from writing values that are completely bogus, i.e. not supported by KVM at all. Userspace is allowed to violated the guest vCPU model. > The code is consistent (copied) from the code that handles MSR_IA32_MCG_CTL: > > https://elixir.bootlin.com/linux/v5.17-rc8/source/arch/x86/kvm/x86.c#L3177 Ah. Sadly, KVM has these sorts of bad examples lurking in dark corners :-/ > Can you elaborate what unnecessary dependency between ioctls this may cause? Enforcing the vCPU model checks on userspace write means userspace must do KVM_X86_SETUP_MCE before KVM_SET_MSRS, otherwise stuffing the MSRs will fail. It's not a big deal since userspace is likely using this ordering given that no one has complained about MSR_IA32_MCG_CTL, but it's easy to remedy and it makes the code simpler.
