[Xen-devel] [PATCH 0/2] fix nested interrupt injection on virtual vmexit

[Xen-devel] [PATCH 0/2] fix nested interrupt injection on virtual vmexit

[Roger Pau Monne]

Hello,

The following patch fixes interrupt injection when using nested
virtualization, and allows the usage of x2APIC by the L1 VMM (tested
with Xen). Patch #2 re-enables exposing the x2APIC CPUID feature bit.

Thanks, Roger.

Roger Pau Monne (2):
  nvmx: fix handling of interrupts
  Revert "tools/libxc: disable x2APIC when using nested virtualization"

 tools/libxc/xc_cpuid_x86.c  | 11 -----------
 xen/arch/x86/hvm/vmx/vvmx.c | 32 --------------------------------
 2 files changed, 43 deletions(-)

-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monne]

When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
interrupts shouldn't be injected using the virtual interrupt delivery
mechanism, and instead should be signaled in the vmcs using the exit
reason and the interruption-information field if the "Acknowledge
interrupt on exit" vmexit control is set.

Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
interrupts on virtual vmexit using the virtual interrupt delivery
assistance, and it's also bogus to ack interrupts without checking if
the vmexit "Acknowledge interrupt on exit" vmexit control is set.
nvmx_intr_intercept already handles interrupts correctly on virtual
vmexit.

Note that this fixes the usage of x2APIC by the L1 VMM, at least when
the L1 VMM is Xen.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d8ab167d62..af48b0beef 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1316,35 +1316,6 @@ static void sync_exception_state(struct vcpu *v)
     }
 }
 
-static void nvmx_update_apicv(struct vcpu *v)
-{
-    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
-    unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
-    uint32_t intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
-
-    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
-         nvmx->intr.source == hvm_intsrc_lapic &&
-         (intr_info & INTR_INFO_VALID_MASK) )
-    {
-        uint16_t status;
-        uint32_t rvi, ppr;
-        uint32_t vector = intr_info & 0xff;
-        struct vlapic *vlapic = vcpu_vlapic(v);
-
-        vlapic_ack_pending_irq(v, vector, 1);
-
-        ppr = vlapic_set_ppr(vlapic);
-        WARN_ON((ppr & 0xf0) != (vector & 0xf0));
-
-        status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
-        rvi = vlapic_has_pending_irq(v);
-        if ( rvi != -1 )
-            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
-
-        __vmwrite(GUEST_INTR_STATUS, status);
-    }
-}
-
 static void virtual_vmexit(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
@@ -1393,9 +1364,6 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
     /* updating host cr0 to sync TS bit */
     __vmwrite(HOST_CR0, v->arch.hvm.vmx.host_cr0);
 
-    if ( cpu_has_vmx_virtual_intr_delivery )
-        nvmx_update_apicv(v);
-
     nvcpu->nv_vmswitch_in_progress = 0;
 }
 
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] Revert "tools/libxc: disable x2APIC when using nested virtualization"

[Roger Pau Monne]

This reverts commit 7b3c5b70a32303b46d0d051e695f18d72cce5ed0 and
re-enables the usage of x2APIC with nested virtualization.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index ac38c1406e..2540aa1e1c 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -653,17 +653,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
         p->extd.itsc = true;
         p->basic.vmx = true;
         p->extd.svm = true;
-
-        /*
-         * BODGE: don't announce x2APIC mode when using nested virtualization,
-         * as it doesn't work properly. This should be removed once the
-         * underlying bug(s) are fixed.
-         */
-        rc = xc_hvm_param_get(xch, domid, HVM_PARAM_NESTEDHVM, &val);
-        if ( rc )
-            goto out;
-        if ( val )
-            p->basic.x2apic = false;
     }
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] Revert "tools/libxc: disable x2APIC when using nested virtualization"

[Wei Liu]

On Wed, Jan 08, 2020 at 11:38:57AM +0100, Roger Pau Monne wrote:
> This reverts commit 7b3c5b70a32303b46d0d051e695f18d72cce5ed0 and
> re-enables the usage of x2APIC with nested virtualization.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wl@xen.org>

(subject to acceptance of patch 1, of course)

> ---
>  tools/libxc/xc_cpuid_x86.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index ac38c1406e..2540aa1e1c 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -653,17 +653,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>          p->extd.itsc = true;
>          p->basic.vmx = true;
>          p->extd.svm = true;
> -
> -        /*
> -         * BODGE: don't announce x2APIC mode when using nested virtualization,
> -         * as it doesn't work properly. This should be removed once the
> -         * underlying bug(s) are fixed.
> -         */
> -        rc = xc_hvm_param_get(xch, domid, HVM_PARAM_NESTEDHVM, &val);
> -        if ( rc )
> -            goto out;
> -        if ( val )
> -            p->basic.x2apic = false;
>      }
>  
>      rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
> -- 
> 2.24.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, January 8, 2020 6:39 PM
> 
> When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> interrupts shouldn't be injected using the virtual interrupt delivery
> mechanism, and instead should be signaled in the vmcs using the exit
> reason and the interruption-information field if the "Acknowledge
> interrupt on exit" vmexit control is set.
> 
> Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> interrupts on virtual vmexit using the virtual interrupt delivery
> assistance, and it's also bogus to ack interrupts without checking if
> the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> nvmx_intr_intercept already handles interrupts correctly on virtual
> vmexit.
> 
> Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> the L1 VMM is Xen.

while this fix makes sense to me, can you also test other L1 VMMs,
so we don't overlook some other intentions covered or hidden by
removed logic?

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index d8ab167d62..af48b0beef 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1316,35 +1316,6 @@ static void sync_exception_state(struct vcpu *v)
>      }
>  }
> 
> -static void nvmx_update_apicv(struct vcpu *v)
> -{
> -    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> -    unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
> -    uint32_t intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
> -
> -    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> -         nvmx->intr.source == hvm_intsrc_lapic &&
> -         (intr_info & INTR_INFO_VALID_MASK) )
> -    {
> -        uint16_t status;
> -        uint32_t rvi, ppr;
> -        uint32_t vector = intr_info & 0xff;
> -        struct vlapic *vlapic = vcpu_vlapic(v);
> -
> -        vlapic_ack_pending_irq(v, vector, 1);
> -
> -        ppr = vlapic_set_ppr(vlapic);
> -        WARN_ON((ppr & 0xf0) != (vector & 0xf0));
> -
> -        status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> -        rvi = vlapic_has_pending_irq(v);
> -        if ( rvi != -1 )
> -            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> -
> -        __vmwrite(GUEST_INTR_STATUS, status);
> -    }
> -}
> -
>  static void virtual_vmexit(struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
> @@ -1393,9 +1364,6 @@ static void virtual_vmexit(struct cpu_user_regs
> *regs)
>      /* updating host cr0 to sync TS bit */
>      __vmwrite(HOST_CR0, v->arch.hvm.vmx.host_cr0);
> 
> -    if ( cpu_has_vmx_virtual_intr_delivery )
> -        nvmx_update_apicv(v);
> -
>      nvcpu->nv_vmswitch_in_progress = 0;
>  }
> 
> --
> 2.24.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monné]

On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Wednesday, January 8, 2020 6:39 PM
> > 
> > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > interrupts shouldn't be injected using the virtual interrupt delivery
> > mechanism, and instead should be signaled in the vmcs using the exit
> > reason and the interruption-information field if the "Acknowledge
> > interrupt on exit" vmexit control is set.
> > 
> > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > interrupts on virtual vmexit using the virtual interrupt delivery
> > assistance, and it's also bogus to ack interrupts without checking if
> > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > nvmx_intr_intercept already handles interrupts correctly on virtual
> > vmexit.
> > 
> > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > the L1 VMM is Xen.
> 
> while this fix makes sense to me, can you also test other L1 VMMs,
> so we don't overlook some other intentions covered or hidden by
> removed logic?

I could test other hypervisors, but do we really expect anything
that's not Xen on Xen to work?

I'm asking because that's the only combination that's actually tested
by osstest.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Monday, January 20, 2020 6:19 PM
> 
> On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > Sent: Wednesday, January 8, 2020 6:39 PM
> > >
> > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > mechanism, and instead should be signaled in the vmcs using the exit
> > > reason and the interruption-information field if the "Acknowledge
> > > interrupt on exit" vmexit control is set.
> > >
> > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > assistance, and it's also bogus to ack interrupts without checking if
> > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > vmexit.
> > >
> > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > the L1 VMM is Xen.
> >
> > while this fix makes sense to me, can you also test other L1 VMMs,
> > so we don't overlook some other intentions covered or hidden by
> > removed logic?
> 
> I could test other hypervisors, but do we really expect anything
> that's not Xen on Xen to work?
> 
> I'm asking because that's the only combination that's actually tested
> by osstest.
> 
> Thanks, Roger.

If others are OK with your assumption, then it's fine. I didn't tightly 
follow the nested virtualization requirements in Xen.

On the other hand, I think this patch needs a revision. It is not bogus
to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
on exit" is off. In such case, the delivery doesn't happen until L1 
hypervisor enables interrupt to clear interrupt window. Then it does
save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
check "Ack interrupt on exit". So I prefer to add such check there 
instead of completely removing this optimization.

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monné]

On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: Monday, January 20, 2020 6:19 PM
> > 
> > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > >
> > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > reason and the interruption-information field if the "Acknowledge
> > > > interrupt on exit" vmexit control is set.
> > > >
> > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > vmexit.
> > > >
> > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > > the L1 VMM is Xen.
> > >
> > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > so we don't overlook some other intentions covered or hidden by
> > > removed logic?
> > 
> > I could test other hypervisors, but do we really expect anything
> > that's not Xen on Xen to work?
> > 
> > I'm asking because that's the only combination that's actually tested
> > by osstest.
> > 
> > Thanks, Roger.
> 
> If others are OK with your assumption, then it's fine. I didn't tightly 
> follow the nested virtualization requirements in Xen.

I can try KVM or bhyve on top of Xen, but I'm not sure whether anyone
has actually tested this, so I could be triggering other bugs in the
nested code.

> On the other hand, I think this patch needs a revision. It is not bogus
> to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> on exit" is off. In such case, the delivery doesn't happen until L1 
> hypervisor enables interrupt to clear interrupt window. Then it does
> save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> check "Ack interrupt on exit". So I prefer to add such check there 
> instead of completely removing this optimization.

Right, if "Ack interrupt on exit" is off the interrupt will trigger a
vmexit, but it won't be acked and the vmexit interrupt information
should have bit 31 set to 0, which I think we don't set correctly.

The Intel SDM states:

"For other VM exits (including those due to external interrupts when
the “acknowledge interrupt on exit” VM-exit control is 0), the field
is marked invalid (by clearing bit 31) and the remainder of the field
is undefined."

AFAICT sync_exception_state also needs to check if VM_EXIT_CONTROLS
has VM_EXIT_ACK_INTR_ON_EXIT set, and only set VM_EXIT_INTR_INFO in
that case, do you agree?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Wednesday, January 22, 2020 12:47 AM
> 
> On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: Monday, January 20, 2020 6:19 PM
> > >
> > > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > > >
> > > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > > reason and the interruption-information field if the "Acknowledge
> > > > > interrupt on exit" vmexit control is set.
> > > > >
> > > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > > vmexit.
> > > > >
> > > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > > > the L1 VMM is Xen.
> > > >
> > > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > > so we don't overlook some other intentions covered or hidden by
> > > > removed logic?
> > >
> > > I could test other hypervisors, but do we really expect anything
> > > that's not Xen on Xen to work?
> > >
> > > I'm asking because that's the only combination that's actually tested
> > > by osstest.
> > >
> > > Thanks, Roger.
> >
> > If others are OK with your assumption, then it's fine. I didn't tightly
> > follow the nested virtualization requirements in Xen.
> 
> I can try KVM or bhyve on top of Xen, but I'm not sure whether anyone
> has actually tested this, so I could be triggering other bugs in the
> nested code.
> 
> > On the other hand, I think this patch needs a revision. It is not bogus
> > to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> > on exit" is off. In such case, the delivery doesn't happen until L1
> > hypervisor enables interrupt to clear interrupt window. Then it does
> > save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> > check "Ack interrupt on exit". So I prefer to add such check there
> > instead of completely removing this optimization.
> 
> Right, if "Ack interrupt on exit" is off the interrupt will trigger a
> vmexit, but it won't be acked and the vmexit interrupt information
> should have bit 31 set to 0, which I think we don't set correctly.
> 
> The Intel SDM states:
> 
> "For other VM exits (including those due to external interrupts when
> the “acknowledge interrupt on exit” VM-exit control is 0), the field
> is marked invalid (by clearing bit 31) and the remainder of the field
> is undefined."
> 
> AFAICT sync_exception_state also needs to check if VM_EXIT_CONTROLS
> has VM_EXIT_ACK_INTR_ON_EXIT set, and only set VM_EXIT_INTR_INFO in
> that case, do you agree?
> 

nice catch.

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monné]

On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: Monday, January 20, 2020 6:19 PM
> > 
> > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > >
> > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > reason and the interruption-information field if the "Acknowledge
> > > > interrupt on exit" vmexit control is set.
> > > >
> > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > vmexit.
> > > >
> > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > > the L1 VMM is Xen.
> > >
> > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > so we don't overlook some other intentions covered or hidden by
> > > removed logic?
> > 
> > I could test other hypervisors, but do we really expect anything
> > that's not Xen on Xen to work?
> > 
> > I'm asking because that's the only combination that's actually tested
> > by osstest.
> > 
> > Thanks, Roger.
> 
> If others are OK with your assumption, then it's fine. I didn't tightly 
> follow the nested virtualization requirements in Xen.
> 
> On the other hand, I think this patch needs a revision. It is not bogus
> to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> on exit" is off. In such case, the delivery doesn't happen until L1 
> hypervisor enables interrupt to clear interrupt window. Then it does
> save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> check "Ack interrupt on exit". So I prefer to add such check there 
> instead of completely removing this optimization.

I went back over this, and I'm still not sure calling
nvmx_update_apicv is actually required: AFAICT vmx_intr_assist will
already inject the interrupt correctly using virtual interrupt
delivery if left pending on the vlapic. I guess the code in
nvmx_update_apicv doesn't hurt as long as it's gated on "Ack on exit"
not being enabled, but it's likely redundant.

I will send an updated patch anyway, since I would like to get this
sorted out sooner rather than later and will follow your advise of
leaving nvmx_update_apicv in place gated by a check of whether "Ack on
exit" is not enabled.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Thursday, January 23, 2020 8:32 PM
> 
> On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: Monday, January 20, 2020 6:19 PM
> > >
> > > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > > >
> > > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > > reason and the interruption-information field if the "Acknowledge
> > > > > interrupt on exit" vmexit control is set.
> > > > >
> > > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > > vmexit.
> > > > >
> > > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > > > the L1 VMM is Xen.
> > > >
> > > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > > so we don't overlook some other intentions covered or hidden by
> > > > removed logic?
> > >
> > > I could test other hypervisors, but do we really expect anything
> > > that's not Xen on Xen to work?
> > >
> > > I'm asking because that's the only combination that's actually tested
> > > by osstest.
> > >
> > > Thanks, Roger.
> >
> > If others are OK with your assumption, then it's fine. I didn't tightly
> > follow the nested virtualization requirements in Xen.
> >
> > On the other hand, I think this patch needs a revision. It is not bogus
> > to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> > on exit" is off. In such case, the delivery doesn't happen until L1
> > hypervisor enables interrupt to clear interrupt window. Then it does
> > save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> > check "Ack interrupt on exit". So I prefer to add such check there
> > instead of completely removing this optimization.
> 
> I went back over this, and I'm still not sure calling
> nvmx_update_apicv is actually required: AFAICT vmx_intr_assist will
> already inject the interrupt correctly using virtual interrupt
> delivery if left pending on the vlapic. I guess the code in
> nvmx_update_apicv doesn't hurt as long as it's gated on "Ack on exit"
> not being enabled, but it's likely redundant.

It's not redundant. If you look at the code sequence, vmx_intr_assist
is invoked before nvmx_switch_guest. At that time, the L1 vCPU is still
in nested guest mode, thereby nvmx_intr_intercept takes effect which
injects the pending vector into vmcs02 and bypasses the remaining
virtual interrupt delivery logic for vmcs01. That is the main reason, imo,
why nvmx_update_apicv is introduced.

iiuc, nvmx_intr_intercept and nvmx_update_apicv work together to
complete nested interrupt injection:

(1) If "Ack interrupt on exit" is on, the pending vector is acked by 
the former and delivered in vvmexit information field.
(2) If "Ack interrupt on exit" is off and no virtual interrupt delivery, 
no ack and interrupt window is enabled by the former.
(3) Otherwise, the vector is acked by the latter and delivered through
virtual interrupt delivery (where vmcs01 has been switched in). 

However, there are two issues in current code. One is about (3), i.e.,
as you identified nvmx_update_apicv shouldn't blindly enable the
optimization without checking the Ack setting. the other is new 
about (2) - currently nvmx_intr_interrupt always enables interrupt 
window when the Ack setting is off, which actually negates the 
optimization of nvmx_update_apicv. Both should be fixed.

> 
> I will send an updated patch anyway, since I would like to get this
> sorted out sooner rather than later and will follow your advise of
> leaving nvmx_update_apicv in place gated by a check of whether "Ack on
> exit" is not enabled.
> 
> Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monné]

On Mon, Feb 03, 2020 at 07:24:04AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: Thursday, January 23, 2020 8:32 PM
> > 
> > On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > Sent: Monday, January 20, 2020 6:19 PM
> > > >
> > > > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > > > >
> > > > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > > > reason and the interruption-information field if the "Acknowledge
> > > > > > interrupt on exit" vmexit control is set.
> > > > > >
> > > > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > > > vmexit.
> > > > > >
> > > > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> > > > > > the L1 VMM is Xen.
> > > > >
> > > > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > > > so we don't overlook some other intentions covered or hidden by
> > > > > removed logic?
> > > >
> > > > I could test other hypervisors, but do we really expect anything
> > > > that's not Xen on Xen to work?
> > > >
> > > > I'm asking because that's the only combination that's actually tested
> > > > by osstest.
> > > >
> > > > Thanks, Roger.
> > >
> > > If others are OK with your assumption, then it's fine. I didn't tightly
> > > follow the nested virtualization requirements in Xen.
> > >
> > > On the other hand, I think this patch needs a revision. It is not bogus
> > > to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> > > on exit" is off. In such case, the delivery doesn't happen until L1
> > > hypervisor enables interrupt to clear interrupt window. Then it does
> > > save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> > > check "Ack interrupt on exit". So I prefer to add such check there
> > > instead of completely removing this optimization.
> > 
> > I went back over this, and I'm still not sure calling
> > nvmx_update_apicv is actually required: AFAICT vmx_intr_assist will
> > already inject the interrupt correctly using virtual interrupt
> > delivery if left pending on the vlapic. I guess the code in
> > nvmx_update_apicv doesn't hurt as long as it's gated on "Ack on exit"
> > not being enabled, but it's likely redundant.
> 
> It's not redundant. If you look at the code sequence, vmx_intr_assist
> is invoked before nvmx_switch_guest. At that time, the L1 vCPU is still
> in nested guest mode, thereby nvmx_intr_intercept takes effect which
> injects the pending vector into vmcs02 and bypasses the remaining
> virtual interrupt delivery logic for vmcs01. That is the main reason, imo,
> why nvmx_update_apicv is introduced.
> 
> iiuc, nvmx_intr_intercept and nvmx_update_apicv work together to
> complete nested interrupt injection:
> 
> (1) If "Ack interrupt on exit" is on, the pending vector is acked by 
> the former and delivered in vvmexit information field.
> (2) If "Ack interrupt on exit" is off and no virtual interrupt delivery, 
> no ack and interrupt window is enabled by the former.
> (3) Otherwise, the vector is acked by the latter and delivered through
> virtual interrupt delivery (where vmcs01 has been switched in). 
> 
> However, there are two issues in current code. One is about (3), i.e.,
> as you identified nvmx_update_apicv shouldn't blindly enable the
> optimization without checking the Ack setting. the other is new 
> about (2) - currently nvmx_intr_interrupt always enables interrupt 
> window when the Ack setting is off, which actually negates the 
> optimization of nvmx_update_apicv. Both should be fixed.

OK, I think I got it. It's likely however that vmx_intr_assist is also
called with the vmcs already switched to vmcs01 (if there's a pending
softirq for example), I guess vmx_intr_assist also copes correctly
when called with the vmcs already switched?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Monday, February 3, 2020 7:56 PM
> 
> On Mon, Feb 03, 2020 at 07:24:04AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: Thursday, January 23, 2020 8:32 PM
> > >
> > > On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > > Sent: Monday, January 20, 2020 6:19 PM
> > > > >
> > > > > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > > > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > > > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > > > > >
> > > > > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > > > > interrupts shouldn't be injected using the virtual interrupt delivery
> > > > > > > mechanism, and instead should be signaled in the vmcs using the
> exit
> > > > > > > reason and the interruption-information field if the "Acknowledge
> > > > > > > interrupt on exit" vmexit control is set.
> > > > > > >
> > > > > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to
> inject
> > > > > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > > > > assistance, and it's also bogus to ack interrupts without checking if
> > > > > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > > > > vmexit.
> > > > > > >
> > > > > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least
> when
> > > > > > > the L1 VMM is Xen.
> > > > > >
> > > > > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > > > > so we don't overlook some other intentions covered or hidden by
> > > > > > removed logic?
> > > > >
> > > > > I could test other hypervisors, but do we really expect anything
> > > > > that's not Xen on Xen to work?
> > > > >
> > > > > I'm asking because that's the only combination that's actually tested
> > > > > by osstest.
> > > > >
> > > > > Thanks, Roger.
> > > >
> > > > If others are OK with your assumption, then it's fine. I didn't tightly
> > > > follow the nested virtualization requirements in Xen.
> > > >
> > > > On the other hand, I think this patch needs a revision. It is not bogus
> > > > to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> > > > on exit" is off. In such case, the delivery doesn't happen until L1
> > > > hypervisor enables interrupt to clear interrupt window. Then it does
> > > > save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> > > > check "Ack interrupt on exit". So I prefer to add such check there
> > > > instead of completely removing this optimization.
> > >
> > > I went back over this, and I'm still not sure calling
> > > nvmx_update_apicv is actually required: AFAICT vmx_intr_assist will
> > > already inject the interrupt correctly using virtual interrupt
> > > delivery if left pending on the vlapic. I guess the code in
> > > nvmx_update_apicv doesn't hurt as long as it's gated on "Ack on exit"
> > > not being enabled, but it's likely redundant.
> >
> > It's not redundant. If you look at the code sequence, vmx_intr_assist
> > is invoked before nvmx_switch_guest. At that time, the L1 vCPU is still
> > in nested guest mode, thereby nvmx_intr_intercept takes effect which
> > injects the pending vector into vmcs02 and bypasses the remaining
> > virtual interrupt delivery logic for vmcs01. That is the main reason, imo,
> > why nvmx_update_apicv is introduced.
> >
> > iiuc, nvmx_intr_intercept and nvmx_update_apicv work together to
> > complete nested interrupt injection:
> >
> > (1) If "Ack interrupt on exit" is on, the pending vector is acked by
> > the former and delivered in vvmexit information field.
> > (2) If "Ack interrupt on exit" is off and no virtual interrupt delivery,
> > no ack and interrupt window is enabled by the former.
> > (3) Otherwise, the vector is acked by the latter and delivered through
> > virtual interrupt delivery (where vmcs01 has been switched in).
> >
> > However, there are two issues in current code. One is about (3), i.e.,
> > as you identified nvmx_update_apicv shouldn't blindly enable the
> > optimization without checking the Ack setting. the other is new
> > about (2) - currently nvmx_intr_interrupt always enables interrupt
> > window when the Ack setting is off, which actually negates the
> > optimization of nvmx_update_apicv. Both should be fixed.
> 
> OK, I think I got it. It's likely however that vmx_intr_assist is also
> called with the vmcs already switched to vmcs01 (if there's a pending
> softirq for example), I guess vmx_intr_assist also copes correctly
> when called with the vmcs already switched?
> 

Yes. What I described is for the case without pending softirqs.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel