RE: [PATCH 5/6] KVM: x86/VMX: add kvm_vmx_reinject_nmi_irq() for NMI/IRQ reinjection

Li, Xin3
2022-11-10T20:40:13Z

> > +#if IS_ENABLED(CONFIG_KVM_INTEL)
> > +/*
> > + * KVM VMX reinjects NMI/IRQ on its current stack, it's a sync
>
> Please use a verb other than "reinject". There is no event injection of any kind,
> KVM is simply making a function call. KVM already uses "inject" and "reinject"
> for KVM where KVM is is literally injecting events into the guest.
>
> The "kvm_vmx" part is also weird IMO. The function is in x86's
> traps/exceptions namespace, not the KVM VMX namespace.

right, "kvm_vmx" doesn't look good per your explanation.

>
> Maybe exc_raise_nmi_or_irq()?

It's good for me.

>
> > + * call thus the values in the pt_regs structure are not used in
> > + * executing NMI/IRQ handlers,
>
> Won't this break stack traces to some extent?
>

The pt_regs structure, and its IP/CS, is NOT part of the call stack, thus
I don't see a problem. No?

> > +static void handle_interrupt_nmi_irqoff(struct kvm_vcpu *vcpu, u32
> > +vector)
> > {
> > - bool is_nmi = entry == (unsigned long)asm_exc_nmi_noist;
> > -
> > - kvm_before_interrupt(vcpu, is_nmi ? KVM_HANDLING_NMI :
> KVM_HANDLING_IRQ);
> > - vmx_do_interrupt_nmi_irqoff(entry);
> > + kvm_before_interrupt(vcpu, vector == NMI_VECTOR ?
> > + KVM_HANDLING_NMI :
> KVM_HANDLING_IRQ);
> > + kvm_vmx_reinject_nmi_irq(vector);
>
> This is where I strongly object to kvm_vmx_reinject_nmi_irq(). This looks like
> KVM is reinjecting the event into the guest, which is all kinds of confusing.
>
> > kvm_after_interrupt(vcpu);
> > }