| Message ID | 20221129193717.513824-10-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM: vNMI (with my fixes) | expand |
On Tue, Nov 29, 2022, Maxim Levitsky wrote: > From: Santosh Shukla <santosh.shukla@amd.com> > > VNMI exposes 3 capability bits (V_NMI, V_NMI_MASK, and V_NMI_ENABLE) to > virtualize NMI and NMI_MASK, Those capability bits are part of > VMCB::intr_ctrl - > V_NMI(11) - Indicates whether a virtual NMI is pending in the guest. > V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest. > V_NMI_ENABLE(26) - Enables the NMI virtualization feature for the guest. > > When Hypervisor wants to inject NMI, it will set V_NMI bit, Processor > will clear the V_NMI bit and Set the V_NMI_MASK which means the Guest is > handling NMI, After the guest handled the NMI, The processor will clear > the V_NMI_MASK on the successful completion of IRET instruction Or if > VMEXIT occurs while delivering the virtual NMI. > > To enable the VNMI capability, Hypervisor need to program > V_NMI_ENABLE bit 1. > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com> > --- > arch/x86/include/asm/svm.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index cb1ee53ad3b189..26d6f549ce2b46 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -203,6 +203,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area { > #define X2APIC_MODE_SHIFT 30 > #define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT) > > +#define V_NMI_PENDING_SHIFT 11 > +#define V_NMI_PENDING (1 << V_NMI_PENDING_SHIFT) > +#define V_NMI_MASK_SHIFT 12 > +#define V_NMI_MASK (1 << V_NMI_MASK_SHIFT) Argh, more KVM warts. The existing INT_CTL defines all use "mask" in the name, so looking at V_NMI_MASK in the context of other code reads "vNMI is pending", not "vNMIs are blocked". IMO, the existing _MASK terminology is the one that's wrong, but there's an absurd amount of prior art in svm.h :-( And the really annoying one is V_INTR_MASKING_MASK, which IIRC says "virtual INTR masking is enabled", not "virtual INTRs are blocked". So maybe call this V_NMI_BLOCKING_MASK? And tack on _MASK too the others (even though I agree it's ugly). > +#define V_NMI_ENABLE_SHIFT 26 > +#define V_NMI_ENABLE (1 << V_NMI_ENABLE_SHIFT) Hrm. I think I would prefer to keep the defines ordered by bit position. Knowing that there's an enable bit isn't all that critical for understanding vNMI pending and blocked.
On 2/1/2023 4:12 AM, Sean Christopherson wrote: > On Tue, Nov 29, 2022, Maxim Levitsky wrote: >> From: Santosh Shukla <santosh.shukla@amd.com> >> >> VNMI exposes 3 capability bits (V_NMI, V_NMI_MASK, and V_NMI_ENABLE) to >> virtualize NMI and NMI_MASK, Those capability bits are part of >> VMCB::intr_ctrl - >> V_NMI(11) - Indicates whether a virtual NMI is pending in the guest. >> V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest. >> V_NMI_ENABLE(26) - Enables the NMI virtualization feature for the guest. >> >> When Hypervisor wants to inject NMI, it will set V_NMI bit, Processor >> will clear the V_NMI bit and Set the V_NMI_MASK which means the Guest is >> handling NMI, After the guest handled the NMI, The processor will clear >> the V_NMI_MASK on the successful completion of IRET instruction Or if >> VMEXIT occurs while delivering the virtual NMI. >> >> To enable the VNMI capability, Hypervisor need to program >> V_NMI_ENABLE bit 1. >> >> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> >> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com> >> --- >> arch/x86/include/asm/svm.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h >> index cb1ee53ad3b189..26d6f549ce2b46 100644 >> --- a/arch/x86/include/asm/svm.h >> +++ b/arch/x86/include/asm/svm.h >> @@ -203,6 +203,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area { >> #define X2APIC_MODE_SHIFT 30 >> #define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT) >> >> +#define V_NMI_PENDING_SHIFT 11 >> +#define V_NMI_PENDING (1 << V_NMI_PENDING_SHIFT) >> +#define V_NMI_MASK_SHIFT 12 >> +#define V_NMI_MASK (1 << V_NMI_MASK_SHIFT) > > Argh, more KVM warts. The existing INT_CTL defines all use "mask" in the name, > so looking at V_NMI_MASK in the context of other code reads "vNMI is pending", > not "vNMIs are blocked". > > IMO, the existing _MASK terminology is the one that's wrong, but there's an absurd > amount of prior art in svm.h :-( > > And the really annoying one is V_INTR_MASKING_MASK, which IIRC says "virtual INTR > masking is enabled", not "virtual INTRs are blocked". > > So maybe call this V_NMI_BLOCKING_MASK? And tack on _MASK too the others (even > though I agree it's ugly). > Sure. >> +#define V_NMI_ENABLE_SHIFT 26 >> +#define V_NMI_ENABLE (1 << V_NMI_ENABLE_SHIFT) > > Hrm. I think I would prefer to keep the defines ordered by bit position. Knowing > that there's an enable bit isn't all that critical for understanding vNMI pending > and blocked. Sure, Sean will include in V3. Thanks, Santosh
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index cb1ee53ad3b189..26d6f549ce2b46 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -203,6 +203,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area { #define X2APIC_MODE_SHIFT 30 #define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT) +#define V_NMI_PENDING_SHIFT 11 +#define V_NMI_PENDING (1 << V_NMI_PENDING_SHIFT) +#define V_NMI_MASK_SHIFT 12 +#define V_NMI_MASK (1 << V_NMI_MASK_SHIFT) +#define V_NMI_ENABLE_SHIFT 26 +#define V_NMI_ENABLE (1 << V_NMI_ENABLE_SHIFT) + #define LBR_CTL_ENABLE_MASK BIT_ULL(0) #define VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK BIT_ULL(1)