The necessity of injecting a hardware exception reported in VMX IDT vectoring information

The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Li, Xin3]

The VMCS IDT vectoring information field is used to report basic information
associated with the event that was being delivered when a VM exit occurred.
such an event itself doesn't trigger a VM exit, however, a condition to deliver
the event is not met, e.g., EPT violation.

When the IDT vectoring information field reports a maskable external interrupt,
KVM reinjects this external interrupt after handling the VM exit. Otherwise,
the external interrupt is lost.

KVM handles a hardware exception reported in the IDT vectoring information
field in the same way, which makes nothing wrong. This piece of code is in
__vmx_complete_interrupts():

        case INTR_TYPE_SOFT_EXCEPTION:
                vcpu->arch.event_exit_inst_len = vmcs_read32(instr_len_field);
                fallthrough;
        case INTR_TYPE_HARD_EXCEPTION:
                if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
                        u32 err = vmcs_read32(error_code_field);
                        kvm_requeue_exception_e(vcpu, vector, err);
                } else
                        kvm_requeue_exception(vcpu, vector);
                break;

But if KVM just ignores any hardware exception in such a case, the CPU will
re-generate it once it resumes guest execution, which looks cleaner.

The question is, must KVM inject a hardware exception from the IDT vectoring
information field? Is there any correctness issue if KVM does not?

If no correctness issue, it's better to not do it, because the injected event
from IDT vectoring could trigger another exception, i.e., a nested exception,
and after the nested exception is handled, the CPU resumes to re-trigger the
original event, which makes not much sense to inject it.

In addition, the benefits of not doing so are:
1) Less code.
2) Faster execution. Calling kvm_requeue_exception_e()/kvm_requeue_exception()
   consumes a few hundred cycles at least, although it's a rare case with EPT,
   but a lot with shadow (who cares?). And vmx_inject_exception() also has a
   cost.
3) An IDT vectoring could trigger more than one VM exit, e.g., the first is an
   EPT violation, and the second a PML full, KVM needs to reinject it twice
   (extremely rare).

Thanks!
  Xin

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Paolo Bonzini]

On Wed, Apr 5, 2023 at 11:34 AM Li, Xin3 <xin3.li@intel.com> wrote:
> The question is, must KVM inject a hardware exception from the IDT vectoring
> information field? Is there any correctness issue if KVM does not?

Fault exceptions probably can be handled as you say, but traps
definitely have to be reinjected. For example, not reinjecting a
singlestep #DB would cause the guest to miss the exception for that
instruction.

> If no correctness issue, it's better to not do it, because the injected event
> from IDT vectoring could trigger another exception, i.e., a nested exception,
> and after the nested exception is handled, the CPU resumes to re-trigger the
> original event, which makes not much sense to inject it.

(Let's use "second" exception instead of "nested" exception).

The CPU doesn't re-trigger the original event unless the second
exception causes a vmexit and the hypervisor moves the IDT-vectored
event fields to the event injection fields. In this case, the first
exception wasn't injected at all.

If the second exception does not cause a vmexit, it is handled as
usual by the processor (by checking if the two exceptions are benign,
contributory or page faults). The behavior is the same even if the
first exception comes from VMX event injection.

Paolo

> In addition, the benefits of not doing so are:
> 1) Less code.
> 2) Faster execution. Calling kvm_requeue_exception_e()/kvm_requeue_exception()
>    consumes a few hundred cycles at least, although it's a rare case with EPT,
>    but a lot with shadow (who cares?). And vmx_inject_exception() also has a
>    cost.
> 3) An IDT vectoring could trigger more than one VM exit, e.g., the first is an
>    EPT violation, and the second a PML full, KVM needs to reinject it twice
>    (extremely rare).
>
> Thanks!
>   Xin
>

RE: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Li, Xin3]

> On Wed, Apr 5, 2023 at 11:34 AM Li, Xin3 <xin3.li@intel.com> wrote:
> > The question is, must KVM inject a hardware exception from the IDT
> > vectoring information field? Is there any correctness issue if KVM does not?
> 
> Fault exceptions probably can be handled as you say, but traps definitely have to
> be reinjected. For example, not reinjecting a singlestep #DB would cause the
> guest to miss the exception for that instruction.

Good point, we need to inject #DB otherwise it's lost.

> > If no correctness issue, it's better to not do it, because the
> > injected event from IDT vectoring could trigger another exception,
> > i.e., a nested exception, and after the nested exception is handled,
> > the CPU resumes to re-trigger the original event, which makes not much sense
> to inject it.
> 
> (Let's use "second" exception instead of "nested" exception).
> 
> The CPU doesn't re-trigger the original event unless the second exception causes
> a vmexit and the hypervisor moves the IDT-vectored event fields to the event
> injection fields. In this case, the first exception wasn't injected at all.
> 
> If the second exception does not cause a vmexit, it is handled as usual by the
> processor (by checking if the two exceptions are benign, contributory or page
> faults). The behavior is the same even if the first exception comes from VMX
> event injection.

The case I was thinking is, both the first and the second exception don't cause
any VM exit, however the first exception triggered an EPT violation. Later
KVM injects the first exception and delivering of the first exception by the
CPU triggers the second exception, then the information about the first
KVM-injected exception is lost, and it can be re-generated once the second
exception is correctly handled.

  Xin
> 
> Paolo
> 
> > In addition, the benefits of not doing so are:
> > 1) Less code.
> > 2) Faster execution. Calling
> kvm_requeue_exception_e()/kvm_requeue_exception()
> >    consumes a few hundred cycles at least, although it's a rare case with EPT,
> >    but a lot with shadow (who cares?). And vmx_inject_exception() also has a
> >    cost.
> > 3) An IDT vectoring could trigger more than one VM exit, e.g., the first is an
> >    EPT violation, and the second a PML full, KVM needs to reinject it twice
> >    (extremely rare).
> >
> > Thanks!
> >   Xin
> >

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Paolo Bonzini]

On 4/5/23 20:03, Li, Xin3 wrote:
>> If the second exception does not cause a vmexit, it is handled as usual by the
>> processor (by checking if the two exceptions are benign, contributory or page
>> faults). The behavior is the same even if the first exception comes from VMX
>> event injection.
>
> The case I was thinking is, both the first and the second exception don't cause
> any VM exit, however the first exception triggered an EPT violation. Later
> KVM injects the first exception and delivering of the first exception by the
> CPU triggers the second exception, then the information about the first
> KVM-injected exception is lost, and it can be re-generated once the second
> exception is correctly handled.

That's not a problem, the behavior is the same as on bare metal 
(depending on whether the two exceptions are benign/contributory/page 
faults).

Paolo

RE: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Li, Xin3]

> >> If the second exception does not cause a vmexit, it is handled as
> >> usual by the processor (by checking if the two exceptions are benign,
> >> contributory or page faults). The behavior is the same even if the
> >> first exception comes from VMX event injection.
> >
> > The case I was thinking is, both the first and the second exception
> > don't cause any VM exit, however the first exception triggered an EPT
> > violation. Later KVM injects the first exception and delivering of the
> > first exception by the CPU triggers the second exception, then the
> > information about the first KVM-injected exception is lost, and it can
> > be re-generated once the second exception is correctly handled.
> 
> That's not a problem, the behavior is the same as on bare metal (depending on
> whether the two exceptions are benign/contributory/page faults).

My point is that if KVM doesn't inject the first exception in this specific
case after it handles EPT violation,  as you said, the behavior is still the
same as on bare metal.

It makes no difference whether to inject it *in this specific case*.

But as you and Sean said, it can't deal the case that L1 injects an exception
not related to the code that L2 is executing.

Thanks!
  Xin

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Sean Christopherson]

On Wed, Apr 05, 2023, Li, Xin3 wrote:
> The VMCS IDT vectoring information field is used to report basic information
> associated with the event that was being delivered when a VM exit occurred.
> such an event itself doesn't trigger a VM exit, however, a condition to deliver
> the event is not met, e.g., EPT violation.
> 
> When the IDT vectoring information field reports a maskable external interrupt,
> KVM reinjects this external interrupt after handling the VM exit. Otherwise,
> the external interrupt is lost.
> 
> KVM handles a hardware exception reported in the IDT vectoring information
> field in the same way, which makes nothing wrong. This piece of code is in
> __vmx_complete_interrupts():
> 
>         case INTR_TYPE_SOFT_EXCEPTION:
>                 vcpu->arch.event_exit_inst_len = vmcs_read32(instr_len_field);
>                 fallthrough;
>         case INTR_TYPE_HARD_EXCEPTION:
>                 if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
>                         u32 err = vmcs_read32(error_code_field);
>                         kvm_requeue_exception_e(vcpu, vector, err);
>                 } else
>                         kvm_requeue_exception(vcpu, vector);
>                 break;
> 
> But if KVM just ignores any hardware exception in such a case, the CPU will
> re-generate it once it resumes guest execution, which looks cleaner.

That's not strictly guaranteed, especially if KVM injected the exception in the
first place.  It's definitely broken if KVM is running L2 and L1 injected an
exception, in which case the exception (from L1) doesn't necessarily have anything
at all to do with the code being executed by L2.  Ditto for exceptions synthesized
and/or migrated from userspace.

And as Paolo called out, it doesn't work for traps.  

There are also likely edge cases around Accessed bits and whatnot.

> The question is, must KVM inject a hardware exception from the IDT vectoring
> information field? Is there any correctness issue if KVM does not?

Yes.  I'm guessing if we start walking through the myriad flows and edge cases,
we'll find more.

> If no correctness issue, it's better to not do it,

In a vacuum, if we were developing a hypervisor from scratch, maybe.  It's most
definitely not better when we're talking about undoing ~15 years of behavior (and
bugs and fixes) in one of the most gnarly areas in x86 virtualization.  E.g. see 

  https://lore.kernel.org/all/20220830231614.3580124-1-seanjc@google.com

for all the work it took to get KVM to correctly handle L1 exception intercept,
and the messy history of the many hacks that came before.

In short, I am not willing to even consider such a change without an absolutely
insane amount of tests and documentation proving correctness, _and_ very strong
evidence that such a change would actually benefit anyone.

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Paolo Bonzini]

On 4/6/23 08:37, Yao, Yuan wrote:
> It's definitely broken for nested case if the exception Is injected
> by L1 in the first place, but if it's injected after interception (by
> L1) for same exception, and it's trap, it can be regenerated by
                                    ^^^^

if it's a fault

> re-execute the L2 code.

You cannot know why L1 injected an exception.  For example L1 could have 
injected a MCE just to test the code in L1.

This is a scary change, in a scary area of code, with unclear benefits. 
It's going to be hard to convince people. :)

Paolo

RE: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Yao, Yuan]

>-----Original Message-----
>From: Paolo Bonzini <pbonzini@redhat.com>
>Sent: Thursday, April 06, 2023 15:33
>To: Yao, Yuan <yuan.yao@intel.com>; Christopherson,, Sean <seanjc@google.com>; Li, Xin3 <xin3.li@intel.com>
>Cc: kvm@vger.kernel.org; Li, Xiaoyao <xiaoyao.li@intel.com>; Dong, Eddie <eddie.dong@intel.com>; Tian, Kevin
><kevin.tian@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>; H.Peter Anvin <hpa@zytor.com>; Mallick, Asit K
><asit.k.mallick@intel.com>
>Subject: Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information
>
>On 4/6/23 08:37, Yao, Yuan wrote:
>> It's definitely broken for nested case if the exception Is injected
>> by L1 in the first place, but if it's injected after interception (by
>> L1) for same exception, and it's trap, it can be regenerated by
>                                    ^^^^
>
>if it's a fault

Yes, my typo, I was thinking fault but wrote trap ...

>
>> re-execute the L2 code.
>
>You cannot know why L1 injected an exception.  For example L1 could have
>injected a MCE just to test the code in L1.
>
>This is a scary change, in a scary area of code, with unclear benefits.
>It's going to be hard to convince people. :)

Yes I understood, thanks Paolo.

>
>Paolo

RE: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Li, Xin3]

> > re-execute the L2 code.
> 
> You cannot know why L1 injected an exception.  For example L1 could have
> injected a MCE just to test the code in L1.

Good example!

> 
> This is a scary change, in a scary area of code, with unclear benefits.
> It's going to be hard to convince people. :)

Yes, totally, I'm not seeking a change but more of a direction check.

How about adding comments about why we are NOT doing it?

Thanks!
  Xin

RE: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Li, Xin3]

> > But if KVM just ignores any hardware exception in such a case, the CPU will
> > re-generate it once it resumes guest execution, which looks cleaner.
> 
> That's not strictly guaranteed, especially if KVM injected the exception in the
> first place.  It's definitely broken if KVM is running L2 and L1 injected an
> exception, in which case the exception (from L1) doesn't necessarily have
> anything at all to do with the code being executed by L2. 

Didn't think about this case, but it's real. And KVM must inject it.

> Ditto for exceptions synthesized and/or migrated from userspace.
> 
> And as Paolo called out, it doesn't work for traps.

Yes, to be a bit clearer, AFAIK, the only case is a #DB trap, whose event type is
3, i.e., hardware exception.


> 
> There are also likely edge cases around Accessed bits and whatnot.

Do you think, for a hardware *fault* caused by the current instruction, KVM
doesn't need to inject it?

I'm thinking to add comments about what is crystal clear, what are still vague,
helping whoever is interested to understand the scary details.


> > The question is, must KVM inject a hardware exception from the IDT vectoring
> > information field? Is there any correctness issue if KVM does not?
> 
> Yes.  I'm guessing if we start walking through the myriad flows and edge cases,
> we'll find more.

I do want to see such cases listed in the comments whenever we notice one.


> > If no correctness issue, it's better to not do it,
> 
> In a vacuum, if we were developing a hypervisor from scratch, maybe.  It's most
> definitely not better when we're talking about undoing ~15 years of behavior

I also noticed the original code was from 2008, early days of KVM.

It's definitely safer to do so.


> (and bugs and fixes) in one of the most gnarly areas in x86 virtualization.  E.g. see
> 
>   https://lore.kernel.org/all/20220830231614.3580124-1-seanjc@google.com
> 
> for all the work it took to get KVM to correctly handle L1 exception intercept,
> and the messy history of the many hacks that came before.

It's a fundamental job to inject events from IDT vectoring, a lot of proven
reliably working code are based on it, I think this is your point.


> In short, I am not willing to even consider such a change without an absolutely
> insane amount of tests and documentation proving correctness, _and_ very
> strong  evidence that such a change would actually benefit anyone.

This is more of a direction check, based on the case you mentioned, I think
I have the answer already.

Thanks!
  Xin

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Xiaoyao Li]

On 4/5/2023 5:34 PM, Li, Xin3 wrote:
> The VMCS IDT vectoring information field is used to report basic information
> associated with the event that was being delivered when a VM exit occurred.
> such an event itself doesn't trigger a VM exit, however, a condition to deliver
> the event is not met, e.g., EPT violation.
> 
> When the IDT vectoring information field reports a maskable external interrupt,
> KVM reinjects this external interrupt after handling the VM exit. Otherwise,
> the external interrupt is lost.
> 
> KVM handles a hardware exception reported in the IDT vectoring information
> field in the same way, which makes nothing wrong. This piece of code is in
> __vmx_complete_interrupts():
> 
>          case INTR_TYPE_SOFT_EXCEPTION:
>                  vcpu->arch.event_exit_inst_len = vmcs_read32(instr_len_field);
>                  fallthrough;
>          case INTR_TYPE_HARD_EXCEPTION:
>                  if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
>                          u32 err = vmcs_read32(error_code_field);
>                          kvm_requeue_exception_e(vcpu, vector, err);
>                  } else
>                          kvm_requeue_exception(vcpu, vector);
>                  break;
> 
> But if KVM just ignores any hardware exception in such a case, the CPU will
> re-generate it once it resumes guest execution, which looks cleaner.
> 
> The question is, must KVM inject a hardware exception from the IDT vectoring
> information field? Is there any correctness issue if KVM does not?

Say there is a case that, a virtual interrupt arrives when an exception 
is delivering but hit EPT VIOLATION. The interrupt is pending and 
recorded in RVI.
- If KVM re-injects the exception on next VM entry, IDT vectoring first 
vectors exception handler and at the instruction boundary (before the 
first instruction of exception handler) to deliver the virtual interrupt 
(if allowed)
- If KVM doesn't re-inject the exception but relies on the re-execution 
of the instruction, then the virtual interrupt can be recognized and 
delivered before the instruction causes the exception.

I'm not sure if the order of events matters or not.

Re: The necessity of injecting a hardware exception reported in VMX IDT vectoring information

[Sean Christopherson]

On Fri, Apr 07, 2023, Xiaoyao Li wrote:
> On 4/5/2023 5:34 PM, Li, Xin3 wrote:
> > The VMCS IDT vectoring information field is used to report basic information
> > associated with the event that was being delivered when a VM exit occurred.
> > such an event itself doesn't trigger a VM exit, however, a condition to deliver
> > the event is not met, e.g., EPT violation.
> > 
> > When the IDT vectoring information field reports a maskable external interrupt,
> > KVM reinjects this external interrupt after handling the VM exit. Otherwise,
> > the external interrupt is lost.
> > 
> > KVM handles a hardware exception reported in the IDT vectoring information
> > field in the same way, which makes nothing wrong. This piece of code is in
> > __vmx_complete_interrupts():
> > 
> >          case INTR_TYPE_SOFT_EXCEPTION:
> >                  vcpu->arch.event_exit_inst_len = vmcs_read32(instr_len_field);
> >                  fallthrough;
> >          case INTR_TYPE_HARD_EXCEPTION:
> >                  if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
> >                          u32 err = vmcs_read32(error_code_field);
> >                          kvm_requeue_exception_e(vcpu, vector, err);
> >                  } else
> >                          kvm_requeue_exception(vcpu, vector);
> >                  break;
> > 
> > But if KVM just ignores any hardware exception in such a case, the CPU will
> > re-generate it once it resumes guest execution, which looks cleaner.
> > 
> > The question is, must KVM inject a hardware exception from the IDT vectoring
> > information field? Is there any correctness issue if KVM does not?
> 
> Say there is a case that, a virtual interrupt arrives when an exception is
> delivering but hit EPT VIOLATION. The interrupt is pending and recorded in
> RVI.
> - If KVM re-injects the exception on next VM entry, IDT vectoring first
> vectors exception handler and at the instruction boundary (before the first
> instruction of exception handler) to deliver the virtual interrupt (if
> allowed)
> - If KVM doesn't re-inject the exception but relies on the re-execution of
> the instruction, then the virtual interrupt can be recognized and delivered
> before the instruction causes the exception.
> 
> I'm not sure if the order of events matters or not.

It matters, e.g. if the exception occurs in an STI shadow then I believe the vIRQ
would get incorrectly delivered in the STI shadow.  That'll likely happen anyways
after the resulting IRET, but there's no guarantee the guest's exception handler
will IRET, or that it will even run, e.g. guest might triple fault first.