2022-08-29 10:14:48

by Santosh Shukla

[permalink] [raw]
Subject: [PATCHv4 1/8] x86/cpu: Add CPUID feature bit for VNMI

VNMI feature allows the hypervisor to inject NMI into the guest w/o
using Event injection mechanism, The benefit of using VNMI over the
event Injection that does not require tracking the Guest's NMI state and
intercepting the IRET for the NMI completion. VNMI achieves that by
exposing 3 capability bits in VMCB intr_cntrl which helps with
virtualizing NMI injection and NMI_Masking.

The presence of this feature is indicated via the CPUID function
0x8000000A_EDX[25].

Reviewed-by: Maxim Levitsky <[email protected]>
Signed-off-by: Santosh Shukla <[email protected]>
---
arch/x86/include/asm/cpufeatures.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index ef4775c6db01..33e3603be09e 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -356,6 +356,7 @@
#define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
#define X86_FEATURE_X2AVIC (15*32+18) /* Virtual x2apic */
#define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
+#define X86_FEATURE_V_NMI (15*32+25) /* Virtual NMI */
#define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* "" SVME addr check */

/* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
--
2.25.1


2022-08-31 23:52:26

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCHv4 1/8] x86/cpu: Add CPUID feature bit for VNMI

On Mon, Aug 29, 2022 at 3:09 AM Santosh Shukla <[email protected]> wrote:
>
> VNMI feature allows the hypervisor to inject NMI into the guest w/o
> using Event injection mechanism, The benefit of using VNMI over the
> event Injection that does not require tracking the Guest's NMI state and
> intercepting the IRET for the NMI completion. VNMI achieves that by
> exposing 3 capability bits in VMCB intr_cntrl which helps with
> virtualizing NMI injection and NMI_Masking.
>
> The presence of this feature is indicated via the CPUID function
> 0x8000000A_EDX[25].
>
> Reviewed-by: Maxim Levitsky <[email protected]>
> Signed-off-by: Santosh Shukla <[email protected]>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index ef4775c6db01..33e3603be09e 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -356,6 +356,7 @@
> #define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
> #define X86_FEATURE_X2AVIC (15*32+18) /* Virtual x2apic */
> #define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
> +#define X86_FEATURE_V_NMI (15*32+25) /* Virtual NMI */
> #define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* "" SVME addr check */

Why is it "V_NMI," but "VGIF"?

Reviewed-by: Jim Mattson <[email protected]>

2022-09-01 13:37:41

by Santosh Shukla

[permalink] [raw]
Subject: Re: [PATCHv4 1/8] x86/cpu: Add CPUID feature bit for VNMI

Hi Jim,

On 9/1/2022 5:12 AM, Jim Mattson wrote:
> On Mon, Aug 29, 2022 at 3:09 AM Santosh Shukla <[email protected]> wrote:
>>
>> VNMI feature allows the hypervisor to inject NMI into the guest w/o
>> using Event injection mechanism, The benefit of using VNMI over the
>> event Injection that does not require tracking the Guest's NMI state and
>> intercepting the IRET for the NMI completion. VNMI achieves that by
>> exposing 3 capability bits in VMCB intr_cntrl which helps with
>> virtualizing NMI injection and NMI_Masking.
>>
>> The presence of this feature is indicated via the CPUID function
>> 0x8000000A_EDX[25].
>>
>> Reviewed-by: Maxim Levitsky <[email protected]>
>> Signed-off-by: Santosh Shukla <[email protected]>
>> ---
>> arch/x86/include/asm/cpufeatures.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index ef4775c6db01..33e3603be09e 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -356,6 +356,7 @@
>> #define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
>> #define X86_FEATURE_X2AVIC (15*32+18) /* Virtual x2apic */
>> #define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
>> +#define X86_FEATURE_V_NMI (15*32+25) /* Virtual NMI */
>> #define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* "" SVME addr check */
>
> Why is it "V_NMI," but "VGIF"?
>
I guess you are asking why I chose V_NMI and not VNMI, right?
if so then there are two reasons for going with V_NMI - IP bits are named in order
V_NMI, V_NMI_MASK, and V_NMI_ENABLE style and also Intel already using VNMI (X86_FEATURE_VNMI)

Thanks,
Santosh

> Reviewed-by: Jim Mattson <[email protected]>

2022-09-01 18:07:58

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCHv4 1/8] x86/cpu: Add CPUID feature bit for VNMI

On Thu, Sep 1, 2022 at 5:45 AM Shukla, Santosh <[email protected]> wrote:
>
> Hi Jim,
>
> On 9/1/2022 5:12 AM, Jim Mattson wrote:
> > On Mon, Aug 29, 2022 at 3:09 AM Santosh Shukla <[email protected]> wrote:
> >>
> >> VNMI feature allows the hypervisor to inject NMI into the guest w/o
> >> using Event injection mechanism, The benefit of using VNMI over the
> >> event Injection that does not require tracking the Guest's NMI state and
> >> intercepting the IRET for the NMI completion. VNMI achieves that by
> >> exposing 3 capability bits in VMCB intr_cntrl which helps with
> >> virtualizing NMI injection and NMI_Masking.
> >>
> >> The presence of this feature is indicated via the CPUID function
> >> 0x8000000A_EDX[25].
> >>
> >> Reviewed-by: Maxim Levitsky <[email protected]>
> >> Signed-off-by: Santosh Shukla <[email protected]>
> >> ---
> >> arch/x86/include/asm/cpufeatures.h | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> >> index ef4775c6db01..33e3603be09e 100644
> >> --- a/arch/x86/include/asm/cpufeatures.h
> >> +++ b/arch/x86/include/asm/cpufeatures.h
> >> @@ -356,6 +356,7 @@
> >> #define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
> >> #define X86_FEATURE_X2AVIC (15*32+18) /* Virtual x2apic */
> >> #define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
> >> +#define X86_FEATURE_V_NMI (15*32+25) /* Virtual NMI */
> >> #define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* "" SVME addr check */
> >
> > Why is it "V_NMI," but "VGIF"?
> >
> I guess you are asking why I chose V_NMI and not VNMI, right?
> if so then there are two reasons for going with V_NMI - IP bits are named in order
> V_NMI, V_NMI_MASK, and V_NMI_ENABLE style and also Intel already using VNMI (X86_FEATURE_VNMI)

I would argue that inconsistency and arbitrary underscores
unnecessarily increase the cognitive load. It is not immediately
obvious to me that an extra underscore implies AMD. What's wrong with
X86_FEATURE_AMD_VNMI? We already have over half a dozen AMD feature
bits that are distinguished from the Intel version by an AMD prefix.

2022-09-05 08:21:09

by Santosh Shukla

[permalink] [raw]
Subject: Re: [PATCHv4 1/8] x86/cpu: Add CPUID feature bit for VNMI



On 9/1/2022 11:13 PM, Jim Mattson wrote:
> On Thu, Sep 1, 2022 at 5:45 AM Shukla, Santosh <[email protected]> wrote:
>>
>> Hi Jim,
>>
>> On 9/1/2022 5:12 AM, Jim Mattson wrote:
>>> On Mon, Aug 29, 2022 at 3:09 AM Santosh Shukla <[email protected]> wrote:
>>>>
>>>> VNMI feature allows the hypervisor to inject NMI into the guest w/o
>>>> using Event injection mechanism, The benefit of using VNMI over the
>>>> event Injection that does not require tracking the Guest's NMI state and
>>>> intercepting the IRET for the NMI completion. VNMI achieves that by
>>>> exposing 3 capability bits in VMCB intr_cntrl which helps with
>>>> virtualizing NMI injection and NMI_Masking.
>>>>
>>>> The presence of this feature is indicated via the CPUID function
>>>> 0x8000000A_EDX[25].
>>>>
>>>> Reviewed-by: Maxim Levitsky <[email protected]>
>>>> Signed-off-by: Santosh Shukla <[email protected]>
>>>> ---
>>>> arch/x86/include/asm/cpufeatures.h | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>>>> index ef4775c6db01..33e3603be09e 100644
>>>> --- a/arch/x86/include/asm/cpufeatures.h
>>>> +++ b/arch/x86/include/asm/cpufeatures.h
>>>> @@ -356,6 +356,7 @@
>>>> #define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
>>>> #define X86_FEATURE_X2AVIC (15*32+18) /* Virtual x2apic */
>>>> #define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
>>>> +#define X86_FEATURE_V_NMI (15*32+25) /* Virtual NMI */
>>>> #define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* "" SVME addr check */
>>>
>>> Why is it "V_NMI," but "VGIF"?
>>>
>> I guess you are asking why I chose V_NMI and not VNMI, right?
>> if so then there are two reasons for going with V_NMI - IP bits are named in order
>> V_NMI, V_NMI_MASK, and V_NMI_ENABLE style and also Intel already using VNMI (X86_FEATURE_VNMI)
>
> I would argue that inconsistency and arbitrary underscores
> unnecessarily increase the cognitive load. It is not immediately
> obvious to me that an extra underscore implies AMD. What's wrong with
> X86_FEATURE_AMD_VNMI? We already have over half a dozen AMD feature

AMD prefix (X86_FEATURE_AMD_VNMI) is fine with me.

> bits that are distinguished from the Intel version by an AMD prefix.

Hi Paolo,

Is there any other suggestions/comment on v4? Should I send v5 with Prefix change or
you're ok to consider v4 with AMD prefix change?

Thanks,
Santosh