On Mon, Nov 21, 2022, Maxim Levitsky wrote: > On Thu, 2022-11-17 at 18:54 +0000, Sean Christopherson wrote: > > E.g. with HF_NMI_MASK => svm->nmi_masked, the end result can be something like: > > > > static bool __is_vnmi_enabled(struct *vmcb) > > { > > return !!(vmcb->control.int_ctl & V_NMI_ENABLE); > > } > > > > static bool is_vnmi_enabled(struct vcpu_svm *svm) > > { > > struct vmcb *vmcb = get_vnmi_vmcb(svm); > > > > return vmcb && __is_vnmi_enabled(vmcb); > > } > > > > static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu) > > { > > struct vcpu_svm *svm = to_svm(vcpu); > > struct vmcb *vmcb = get_vnmi_vmcb(svm); > > > > if (vmcb && __is_vnmi_enabled(vmcb)) > > return !!(vmcb->control.int_ctl & V_NMI_MASK); > > else > > return !!(vcpu->arch.hflags & HF_NMI_MASK); > > } > > > > static void svm_set_or_clear_vnmi_mask(struct vmcb *vmcb, bool set) > > { > > if (set) > > vmcb->control.int_ctl |= V_NMI_MASK; > > else > > vmcb->control.int_ctl &= ~V_NMI_MASK; > > } > > > > static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) > > { > > struct vcpu_svm *svm = to_svm(vcpu); > > struct vmcb *vmcb = get_vnmi_vmcb(svm); > > > > if (vmcb && __is_vnmi_enabled(vmcb)) { > > if (masked) > > vmcb->control.int_ctl |= V_NMI_MASK; > > else > > vmcb->control.int_ctl &= ~V_NMI_MASK; > > } else { > > svm->nmi_masked = masked; > > } > > > > if (!masked) > > svm_disable_iret_interception(svm); > > } > > OK, this is one of the ways to do it, makes sense overall. > I actualy wanted to do something like that but opted to not touch > the original code too much, but only what I needed. I can do this > in a next version. After looking at more of this code, I think having get_vnmi_vmcb() is a mistake. It just ends up being a funky wrapper to the current svm->vmcb. And the manual check on the "vnmi" global is pointless. If KVM sets V_NMI_ENABLE in any VMCB when vnmi=false, then that's a KVM bug. Dropping the wrapper eliminates the possibility of a NULL VMCB pointer, and IMO yields far more readable code. static bool is_vnmi_enabled(struct vcpu_svm *svm) { return !!(svm->vmcb->control.int_ctl & V_NMI_ENABLE); } static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); if (is_vnmi_enabled(svm)) return !!(svm->vmcb->control.int_ctl & V_NMI_MASK); else return !!(vcpu->arch.hflags & HF_NMI_MASK); } static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) { struct vcpu_svm *svm = to_svm(vcpu); if (is_vnmi_enabled(svm)) { if (masked) svm->vmcb->control.int_ctl |= V_NMI_MASK; else svm->vmcb->control.int_ctl &= ~V_NMI_MASK; } else { svm->nmi_masked = masked; } if (!masked) svm_disable_iret_interception(svm); }