Extend the FRED NMI entry point from KVM to take an extra argument to
allow KVM to invoke the FRED event dispatch framework with event data.
This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
Read the VMCS exit qualification field to get the NMI-source information
and store it as event data precisely in the format expected by the FRED
event framework.
Read the VMCS exit qualification unconditionally since almost all
upcoming CPUs are expected to enable FRED and NMI-source together. In
the rare case that NMI-source isn't enabled, the extra VMREAD would be
harmless since the exit qualification is expected to be zero.
Suggested-by: Sean Christopherson <seanjc@google.com>
Originally-by: Zeng Guang <guang.zeng@intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: Pass the event data from KVM only for NMI. (Sean)
v6: No change
v5: Read the VMCS exit qualification unconditionally. (Sean)
Combine related patches into one.
---
arch/x86/entry/entry_64_fred.S | 2 +-
arch/x86/include/asm/fred.h | 11 ++++++-----
arch/x86/kvm/vmx/vmx.c | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index 29c5c32c16c3..1c9c6e036233 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
* +--------+-----------------+
*/
push $0 /* Reserved, must be 0 */
- push $0 /* Event data, 0 for IRQ/NMI */
+ push %rsi /* Event data for NMI */
push %rdi /* fred_ss handed in by the caller */
push %rbp
pushf
diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index 552332ca060c..bccf4a3c06b8 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -66,14 +66,14 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs)
void asm_fred_entrypoint_user(void);
void asm_fred_entrypoint_kernel(void);
-void asm_fred_entry_from_kvm(struct fred_ss);
+void asm_fred_entry_from_kvm(struct fred_ss ss, unsigned long edata);
__visible void fred_entry_from_user(struct pt_regs *regs);
__visible void fred_entry_from_kernel(struct pt_regs *regs);
__visible void __fred_entry_from_kvm(struct pt_regs *regs);
/* Must be called from noinstr code, thus __always_inline */
-static __always_inline void fred_nmi_from_kvm(void)
+static __always_inline void fred_nmi_from_kvm(unsigned long edata)
{
struct fred_ss ss = {
.ss = __KERNEL_DS,
@@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
.lm = 1,
};
- asm_fred_entry_from_kvm(ss);
+ asm_fred_entry_from_kvm(ss, edata);
}
static inline void fred_irq_from_kvm(unsigned int vector)
@@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
.lm = 1,
};
- asm_fred_entry_from_kvm(ss);
+ /* Event data is always zero for IRQ */
+ asm_fred_entry_from_kvm(ss, 0);
}
void cpu_init_fred_exceptions(void);
@@ -123,7 +124,7 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs) { ret
static inline void cpu_init_fred_exceptions(void) { }
static inline void cpu_init_fred_rsps(void) { }
static inline void fred_complete_exception_setup(void) { }
-static __always_inline void fred_nmi_from_kvm(void) { }
+static __always_inline void fred_nmi_from_kvm(unsigned long edata) { }
static inline void fred_irq_from_kvm(unsigned int vector) { }
static inline void fred_sync_rsp0(unsigned long rsp0) { }
static inline void fred_update_rsp0(void) { }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 8183886eda3d..69832ca31e5b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7264,7 +7264,7 @@ noinstr void vmx_handle_nmi(struct kvm_vcpu *vcpu)
kvm_before_interrupt(vcpu, KVM_HANDLING_NMI);
if (cpu_feature_enabled(X86_FEATURE_FRED))
- fred_nmi_from_kvm();
+ fred_nmi_from_kvm(vmx_get_exit_qual(vcpu));
else
vmx_do_nmi_irqoff();
kvm_after_interrupt(vcpu);
--
2.43.0On 6/12/2025 2:48 PM, Sohil Mehta wrote:
> Extend the FRED NMI entry point from KVM to take an extra argument to
> allow KVM to invoke the FRED event dispatch framework with event data.
>
> This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
> Read the VMCS exit qualification field to get the NMI-source information
> and store it as event data precisely in the format expected by the FRED
> event framework.
>
> Read the VMCS exit qualification unconditionally since almost all
> upcoming CPUs are expected to enable FRED and NMI-source together. In
> the rare case that NMI-source isn't enabled, the extra VMREAD would be
> harmless since the exit qualification is expected to be zero.
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Originally-by: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
A couple of nits below, otherwise:
Reviewed-by: Xin Li (Intel) <xin@zytor.com>
> ---
> v7: Pass the event data from KVM only for NMI. (Sean)
>
> v6: No change
>
> v5: Read the VMCS exit qualification unconditionally. (Sean)
> Combine related patches into one.
> ---
> arch/x86/entry/entry_64_fred.S | 2 +-
> arch/x86/include/asm/fred.h | 11 ++++++-----
> arch/x86/kvm/vmx/vmx.c | 2 +-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
> index 29c5c32c16c3..1c9c6e036233 100644
> --- a/arch/x86/entry/entry_64_fred.S
> +++ b/arch/x86/entry/entry_64_fred.S
> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
> * +--------+-----------------+
> */
> push $0 /* Reserved, must be 0 */
> - push $0 /* Event data, 0 for IRQ/NMI */
> + push %rsi /* Event data for NMI */
Maybe a bit more accurate?
/* Event data, NMI-source bitmap only so far */
> push %rdi /* fred_ss handed in by the caller */
> push %rbp
> pushf
> diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
> index 552332ca060c..bccf4a3c06b8 100644
> --- a/arch/x86/include/asm/fred.h
> +++ b/arch/x86/include/asm/fred.h
> @@ -66,14 +66,14 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs)
>
> void asm_fred_entrypoint_user(void);
> void asm_fred_entrypoint_kernel(void);
> -void asm_fred_entry_from_kvm(struct fred_ss);
> +void asm_fred_entry_from_kvm(struct fred_ss ss, unsigned long edata);
>
> __visible void fred_entry_from_user(struct pt_regs *regs);
> __visible void fred_entry_from_kernel(struct pt_regs *regs);
> __visible void __fred_entry_from_kvm(struct pt_regs *regs);
>
> /* Must be called from noinstr code, thus __always_inline */
> -static __always_inline void fred_nmi_from_kvm(void)
> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
> {
> struct fred_ss ss = {
> .ss = __KERNEL_DS,
> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
> .lm = 1,
> };
>
> - asm_fred_entry_from_kvm(ss);
> + asm_fred_entry_from_kvm(ss, edata);
> }
>
> static inline void fred_irq_from_kvm(unsigned int vector)
> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
> .lm = 1,
> };
>
> - asm_fred_entry_from_kvm(ss);
> + /* Event data is always zero for IRQ */
/* Event data not used for IRQ thus 0 */
On 6/18/2025 10:02 PM, Xin Li wrote:
>> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
>> index 29c5c32c16c3..1c9c6e036233 100644
>> --- a/arch/x86/entry/entry_64_fred.S
>> +++ b/arch/x86/entry/entry_64_fred.S
>> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
>> * +--------+-----------------+
>> */
>> push $0 /* Reserved, must be 0 */
>> - push $0 /* Event data, 0 for IRQ/NMI */
>> + push %rsi /* Event data for NMI */
>
> Maybe a bit more accurate?
>
Actually, I am wondering if it might be better to make it less precise
than it is right now. How about
/* Event data passed in by the caller */
The problem with having precise comments for a generic implementation is
that the caller might get updated, but we would forget to update this
comment since nothing in this function needs to change.
> /* Event data, NMI-source bitmap only so far */
>
>> push %rdi /* fred_ss handed in by the caller */
>> push %rbp
>> pushf
...
>> /* Must be called from noinstr code, thus __always_inline */
>> -static __always_inline void fred_nmi_from_kvm(void)
>> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
>> {
>> struct fred_ss ss = {
>> .ss = __KERNEL_DS,
>> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
>> .lm = 1,
>> };
>>
>> - asm_fred_entry_from_kvm(ss);
>> + asm_fred_entry_from_kvm(ss, edata);
>> }
>>
>> static inline void fred_irq_from_kvm(unsigned int vector)
>> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
>> .lm = 1,
>> };
>>
>> - asm_fred_entry_from_kvm(ss);
>> + /* Event data is always zero for IRQ */
>
> /* Event data not used for IRQ thus 0 */
Event data "not used" might imply that the architecture provides
something, but the kernel is choosing to not use it. There is no event
data for IRQ, right?
I want to say that the event data for IRQ has to be zero until the
architecture changes — Similar to the /* Reserved, must be 0 */ comment
in asm_fred_entry_from_kvm().
On 6/19/2025 3:15 PM, Sohil Mehta wrote:
> On 6/18/2025 10:02 PM, Xin Li wrote:
>>> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
>>> index 29c5c32c16c3..1c9c6e036233 100644
>>> --- a/arch/x86/entry/entry_64_fred.S
>>> +++ b/arch/x86/entry/entry_64_fred.S
>>> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
>>> * +--------+-----------------+
>>> */
>>> push $0 /* Reserved, must be 0 */
>>> - push $0 /* Event data, 0 for IRQ/NMI */
>>> + push %rsi /* Event data for NMI */
>>
>> Maybe a bit more accurate?
>>
>
> Actually, I am wondering if it might be better to make it less precise
> than it is right now. How about
>
> /* Event data passed in by the caller */
>
> The problem with having precise comments for a generic implementation is
> that the caller might get updated, but we would forget to update this
> comment since nothing in this function needs to change.
No strong preference, I'm okay if you take this approach.
>
>> /* Event data, NMI-source bitmap only so far */
>>
>>> push %rdi /* fred_ss handed in by the caller */
>>> push %rbp
>>> pushf
>
> ...
>
>>> /* Must be called from noinstr code, thus __always_inline */
>>> -static __always_inline void fred_nmi_from_kvm(void)
>>> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
>>> {
>>> struct fred_ss ss = {
>>> .ss = __KERNEL_DS,
>>> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
>>> .lm = 1,
>>> };
>>>
>>> - asm_fred_entry_from_kvm(ss);
>>> + asm_fred_entry_from_kvm(ss, edata);
>>> }
>>>
>>> static inline void fred_irq_from_kvm(unsigned int vector)
>>> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
>>> .lm = 1,
>>> };
>>>
>>> - asm_fred_entry_from_kvm(ss);
>>> + /* Event data is always zero for IRQ */
>>
>> /* Event data not used for IRQ thus 0 */
>
> Event data "not used" might imply that the architecture provides
> something, but the kernel is choosing to not use it. There is no event
> data for IRQ, right?
>
> I want to say that the event data for IRQ has to be zero until the
> architecture changes — Similar to the /* Reserved, must be 0 */ comment
> in asm_fred_entry_from_kvm().
>
FRED spec says:
For any other event, the event data are not currently defined and will
be zero until they are.
So "Event data not defined for IRQ thus 0."
On 6/19/2025 3:45 PM, Xin Li wrote: > On 6/19/2025 3:15 PM, Sohil Mehta wrote: >> >> I want to say that the event data for IRQ has to be zero until the >> architecture changes — Similar to the /* Reserved, must be 0 */ comment >> in asm_fred_entry_from_kvm(). >> > > FRED spec says: > > For any other event, the event data are not currently defined and will > be zero until they are. > > So "Event data not defined for IRQ thus 0." I am fine with this. Not *defined* removes the ambiguity.
On 2025-06-19 15:57, Sohil Mehta wrote: > On 6/19/2025 3:45 PM, Xin Li wrote: >> On 6/19/2025 3:15 PM, Sohil Mehta wrote: >>> >>> I want to say that the event data for IRQ has to be zero until the >>> architecture changes — Similar to the /* Reserved, must be 0 */ comment >>> in asm_fred_entry_from_kvm(). >>> >> >> FRED spec says: >> >> For any other event, the event data are not currently defined and will >> be zero until they are. >> >> So "Event data not defined for IRQ thus 0." > > I am fine with this. Not *defined* removes the ambiguity. > So I was thinking about this, and wonder: how expensive is it to get the event data exit information out of VMX? If it is not very expensive, it would arguably be a good thing to future-proof by fetching that information, even if it is currently always zero. -hpa
On Fri, Jun 20, 2025, H. Peter Anvin wrote: > On 2025-06-19 15:57, Sohil Mehta wrote: > > On 6/19/2025 3:45 PM, Xin Li wrote: > > > On 6/19/2025 3:15 PM, Sohil Mehta wrote: > > > > > > > > I want to say that the event data for IRQ has to be zero until the > > > > architecture changes — Similar to the /* Reserved, must be 0 */ comment > > > > in asm_fred_entry_from_kvm(). > > > > > > > > > > FRED spec says: > > > > > > For any other event, the event data are not currently defined and will > > > be zero until they are. > > > > > > So "Event data not defined for IRQ thus 0." > > > > I am fine with this. Not *defined* removes the ambiguity. > > > > So I was thinking about this, and wonder: how expensive is it to get the > event data exit information out of VMX? If it is not very expensive, it > would arguably be a good thing to future-proof by fetching that information, > even if it is currently always zero. It's trivially easy to do in KVM, and the cost of the VMREAD should be less than 20 cycles. So quite cheap in the grand scheme. If VMREAD is more costly than that, then we have bigger problems :-)
On 2025-06-20 16:18, Sean Christopherson wrote: >> >> So I was thinking about this, and wonder: how expensive is it to get the >> event data exit information out of VMX? If it is not very expensive, it >> would arguably be a good thing to future-proof by fetching that information, >> even if it is currently always zero. > > It's trivially easy to do in KVM, and the cost of the VMREAD should be less than > 20 cycles. So quite cheap in the grand scheme. If VMREAD is more costly than > that, then we have bigger problems :-) > LOL. Since it is up to you, Paulo, etc. to decide how to do the tradeoffs formaintainability, debuggability and performance in KVM I am guessing this is a vote in favor? (You can always take it out if it is a performance problem, until such time that the kernel itself starts consuming this information for reasons currently unknown.) -hpa
On Fri, Jun 20, 2025, H. Peter Anvin wrote: > On 2025-06-20 16:18, Sean Christopherson wrote: > > > > > > So I was thinking about this, and wonder: how expensive is it to get the > > > event data exit information out of VMX? If it is not very expensive, it > > > would arguably be a good thing to future-proof by fetching that information, > > > even if it is currently always zero. > > > > It's trivially easy to do in KVM, and the cost of the VMREAD should be less than > > 20 cycles. So quite cheap in the grand scheme. If VMREAD is more costly than > > that, then we have bigger problems :-) > > > > LOL. Since it is up to you, Paulo, etc. to decide how to do the tradeoffs > formaintainability, debuggability and performance in KVM I am guessing this > is a vote in favor? (You can always take it out if it is a performance > problem, until such time that the kernel itself starts consuming this > information for reasons currently unknown.) Unless you can pinky swear that vmcs.EXIT_QUALIFICATION will provide event data for IRQ exits, then I'd prefer to pass '0' unconditionally. '0' will always be safe, if potentially suboptimal. But passing what could in theory be something other than FRED-formatted event data could lead to buggy behavior. Per the FRED spec, Revision 7.0, exit-qualification doesn't hold event data for IRQ exits. For some events for which event data is defined (see Section 5.2.1), the event data is saved in the exit-qualification field. (This is done for #DB, #PF, and NMI.)
On June 23, 2025 8:39:41 AM PDT, Sean Christopherson <seanjc@google.com> wrote: >On Fri, Jun 20, 2025, H. Peter Anvin wrote: >> On 2025-06-20 16:18, Sean Christopherson wrote: >> > > >> > > So I was thinking about this, and wonder: how expensive is it to get the >> > > event data exit information out of VMX? If it is not very expensive, it >> > > would arguably be a good thing to future-proof by fetching that information, >> > > even if it is currently always zero. >> > >> > It's trivially easy to do in KVM, and the cost of the VMREAD should be less than >> > 20 cycles. So quite cheap in the grand scheme. If VMREAD is more costly than >> > that, then we have bigger problems :-) >> > >> >> LOL. Since it is up to you, Paulo, etc. to decide how to do the tradeoffs >> formaintainability, debuggability and performance in KVM I am guessing this >> is a vote in favor? (You can always take it out if it is a performance >> problem, until such time that the kernel itself starts consuming this >> information for reasons currently unknown.) > >Unless you can pinky swear that vmcs.EXIT_QUALIFICATION will provide event data >for IRQ exits, then I'd prefer to pass '0' unconditionally. '0' will always be >safe, if potentially suboptimal. But passing what could in theory be something >other than FRED-formatted event data could lead to buggy behavior. Per the FRED >spec, Revision 7.0, exit-qualification doesn't hold event data for IRQ exits. > > For some events for which event data is defined (see Section 5.2.1), the event > data is saved in the exit-qualification field. (This is done for #DB, #PF, and NMI.) I agree, let's stick to that for now, since this is a kernel internal interface and nothing consumes it. After all, it will save a handful of cycles. I will still check into the "pinky swear", though.
On Thu, Jun 12, 2025, Sohil Mehta wrote:
> Extend the FRED NMI entry point from KVM to take an extra argument to
> allow KVM to invoke the FRED event dispatch framework with event data.
>
> This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
> Read the VMCS exit qualification field to get the NMI-source information
> and store it as event data precisely in the format expected by the FRED
> event framework.
>
> Read the VMCS exit qualification unconditionally since almost all
> upcoming CPUs are expected to enable FRED and NMI-source together. In
> the rare case that NMI-source isn't enabled, the extra VMREAD would be
> harmless since the exit qualification is expected to be zero.
Nit, instead of "is expected to be zero", something like this
harmless since the exit qualification is architecturally guaranteed to be
zero on CPUs that don't support NMI-source reporting. Per the SDM's
"Exit qualification" subsection of "Basic VM-Exit Information":
For all other VM exits, this field is cleared.
--
to make it very explicit that reading the exit qualification on older CPUs is 100%
safe, e.g. even on non-FRED CPUs (see https://lore.kernel.org/all/aBUiwLV4ZY2HdRbz@google.com).
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Originally-by: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
Acked-by: Sean Christopherson <seanjc@google.com>
On 6/12/2025 5:18 PM, Sean Christopherson wrote: >> Read the VMCS exit qualification unconditionally since almost all >> upcoming CPUs are expected to enable FRED and NMI-source together. In >> the rare case that NMI-source isn't enabled, the extra VMREAD would be >> harmless since the exit qualification is expected to be zero. > > Nit, instead of "is expected to be zero", something like this > > harmless since the exit qualification is architecturally guaranteed to be > zero on CPUs that don't support NMI-source reporting. Per the SDM's > "Exit qualification" subsection of "Basic VM-Exit Information": > > For all other VM exits, this field is cleared. > -- > Looks good. Clarifying it explicitly reduces ambiguity. If this patchset gets applied directly, I am hoping the clarification can be included while applying. If we end up doing another version, I'll add it to the log. > to make it very explicit that reading the exit qualification on older CPUs is 100% > safe, e.g. even on non-FRED CPUs (see https://lore.kernel.org/all/aBUiwLV4ZY2HdRbz@google.com). > That's interesting. Thanks for the link. >> Suggested-by: Sean Christopherson <seanjc@google.com> >> Originally-by: Zeng Guang <guang.zeng@intel.com> >> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com> >> --- > > Acked-by: Sean Christopherson <seanjc@google.com> Thank you!
© 2016 - 2026 Red Hat, Inc.