On Tue, Apr 12, 2022 at 11:19 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Tue, Apr 12, 2022, Paolo Bonzini wrote: > > On 4/11/22 21:08, Sean Christopherson 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: > > > > > > if (!(mcg_cap & MCG_CMCI_P) && !msr_info->host_initiated) > > > return 1; > > > > > > > This is somewhat dangerous as it complicates (or removes) the invariants > > that other code can rely on. Thus, usually, only the default value is > > allowed for KVM_SET_MSR. > > Heh, I don't know if "usually" is the right word, that implies KVM is consistent > enough to have a simple majority for any behavior, whatever that behavior may be :-) > > Anyways, on second look, I agree that KVM should require that userspace first enable > CMCI via mcg_cap. I thought that vcpu->arch.mcg_cap could be written via the MSR > interface, i.e. via userspace writes to MSR_IA32_MCG_CAP, and could create dependencies > within KVM_SET_MSRS. But KVM only allows reading the MSR. vcpu->arch.mcg_cap is written via kvm_vcpu_ioctl_x86_setup_mce and it requires explicit enablement in kvm_mce_cap_supported for MCG_CMCI_P. The pattern of KVM_SET/GET_MSRS depending on bits in vcpu->arch.mcg_cap seems common so I will keep the check of MCG_CMCI_P in V2. As only allowing 0 to be set to MCi_CTL2 in set_msr_mce, it is more consistent to hardware behavior and the invariants that other code depends on as Paolo pointed out. I will keep this implementation in V2 as well. Thanks, -Jue
