| Message ID | 20200320190737.42110-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/nvmx: attempt to fix interrupt injection | expand |
> From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Saturday, March 21, 2020 3:08 AM > > Current code in nvmx_update_apicv set the guest interrupt status field > but doesn't update the exit bitmap, which can cause issues of lost > interrupts on the L1 hypervisor if vmx_intr_assist gets > short-circuited by nvmx_intr_intercept returning true. Above is not accurate. Currently Xen didn't choose to update the EOI exit bitmap every time when there is a change. Instead, it chose to batch the update before resuming to the guest. sort of optimization. So it is not related to whether SVI is changed. We should always do the bitmap update in nvmx_update_apicv, regardless of the setting of Ack-on-exit ... Thanks Kevin > > Extract the code to update the exit bitmap from vmx_intr_assist into a > helper and use it in nvmx_update_apicv when updating the guest > interrupt status field. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- > xen/arch/x86/hvm/vmx/vvmx.c | 1 + > xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ > 3 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c > index 49a1295f09..000e14af49 100644 > --- a/xen/arch/x86/hvm/vmx/intr.c > +++ b/xen/arch/x86/hvm/vmx/intr.c > @@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v, struct > hvm_intack intack) > return 0; > } > > +void vmx_sync_exit_bitmap(struct vcpu *v) > +{ > + const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); > + unsigned int i; > + > + while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n)) < > n ) > + { > + clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed); > + __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]); > + } > +} > + > void vmx_intr_assist(void) > { > struct hvm_intack intack; > @@ -318,7 +330,6 @@ void vmx_intr_assist(void) > intack.source != hvm_intsrc_vector ) > { > unsigned long status; > - unsigned int i, n; > > /* > * intack.vector is the highest priority vector. So we set eoi_exit_bitmap > @@ -379,13 +390,7 @@ void vmx_intr_assist(void) > intack.vector; > __vmwrite(GUEST_INTR_STATUS, status); > > - n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); > - while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, > - n)) < n ) > - { > - clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed); > - __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]); > - } > + vmx_sync_exit_bitmap(v); > > pt_intr_post(v, intack); > } > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 180d01e385..e041ecc115 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1414,6 +1414,7 @@ static void nvmx_update_apicv(struct vcpu *v) > status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > > __vmwrite(GUEST_INTR_STATUS, status); > + vmx_sync_exit_bitmap(v); > } > } > > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm- > x86/hvm/vmx/vmx.h > index b334e1ec94..111ccd7e61 100644 > --- a/xen/include/asm-x86/hvm/vmx/vmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h > @@ -610,6 +610,8 @@ void update_guest_eip(void); > void vmx_pi_per_cpu_init(unsigned int cpu); > void vmx_pi_desc_fixup(unsigned int cpu); > > +void vmx_sync_exit_bitmap(struct vcpu *v); > + > #ifdef CONFIG_HVM > void vmx_pi_hooks_assign(struct domain *d); > void vmx_pi_hooks_deassign(struct domain *d); > -- > 2.25.0
On Tue, Mar 24, 2020 at 06:22:43AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: Saturday, March 21, 2020 3:08 AM > > > > Current code in nvmx_update_apicv set the guest interrupt status field > > but doesn't update the exit bitmap, which can cause issues of lost > > interrupts on the L1 hypervisor if vmx_intr_assist gets > > short-circuited by nvmx_intr_intercept returning true. > > Above is not accurate. Currently Xen didn't choose to update the EOI > exit bitmap every time when there is a change. Instead, it chose to > batch the update before resuming to the guest. sort of optimization. > So it is not related to whether SVI is changed. We should always do the > bitmap update in nvmx_update_apicv, regardless of the setting of > Ack-on-exit ... But if Ack on exit is not set the GUEST_INTR_STATUS won't be changed by nvmx_intr_intercept, and hence there's no need to update the EOI exit map? If OTOH the GUEST_INTR_STATUS field is changed by vmx_intr_assist the bitmap will already be updated there. Thanks, Roger.
> From: Roger Pau Monné <roger.pau@citrix.com> > Sent: Tuesday, March 24, 2020 5:59 PM > > On Tue, Mar 24, 2020 at 06:22:43AM +0000, Tian, Kevin wrote: > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > Sent: Saturday, March 21, 2020 3:08 AM > > > > > > Current code in nvmx_update_apicv set the guest interrupt status field > > > but doesn't update the exit bitmap, which can cause issues of lost > > > interrupts on the L1 hypervisor if vmx_intr_assist gets > > > short-circuited by nvmx_intr_intercept returning true. > > > > Above is not accurate. Currently Xen didn't choose to update the EOI > > exit bitmap every time when there is a change. Instead, it chose to > > batch the update before resuming to the guest. sort of optimization. > > So it is not related to whether SVI is changed. We should always do the > > bitmap update in nvmx_update_apicv, regardless of the setting of > > Ack-on-exit ... > > But if Ack on exit is not set the GUEST_INTR_STATUS won't be changed > by nvmx_intr_intercept, and hence there's no need to update the EOI > exit map? > > If OTOH the GUEST_INTR_STATUS field is changed by vmx_intr_assist the > bitmap will already be updated there. > If you agree with my comment in patch 2/3 about setting RVI for ack-on-exit=0, then EOI bitmap update should be done there too. Thanks Kevin
On Tue, Mar 24, 2020 at 10:16:52AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monné <roger.pau@citrix.com> > > Sent: Tuesday, March 24, 2020 5:59 PM > > > > On Tue, Mar 24, 2020 at 06:22:43AM +0000, Tian, Kevin wrote: > > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > > Sent: Saturday, March 21, 2020 3:08 AM > > > > > > > > Current code in nvmx_update_apicv set the guest interrupt status field > > > > but doesn't update the exit bitmap, which can cause issues of lost > > > > interrupts on the L1 hypervisor if vmx_intr_assist gets > > > > short-circuited by nvmx_intr_intercept returning true. > > > > > > Above is not accurate. Currently Xen didn't choose to update the EOI > > > exit bitmap every time when there is a change. Instead, it chose to > > > batch the update before resuming to the guest. sort of optimization. > > > So it is not related to whether SVI is changed. We should always do the > > > bitmap update in nvmx_update_apicv, regardless of the setting of > > > Ack-on-exit ... > > > > But if Ack on exit is not set the GUEST_INTR_STATUS won't be changed > > by nvmx_intr_intercept, and hence there's no need to update the EOI > > exit map? > > > > If OTOH the GUEST_INTR_STATUS field is changed by vmx_intr_assist the > > bitmap will already be updated there. > > > > If you agree with my comment in patch 2/3 about setting RVI for > ack-on-exit=0, then EOI bitmap update should be done there too. Yes, I agree, see my reply to your comment. I (again) misremembered the sequence and assumed the switch will happen prior to calling vmx_intr_assist which is not the case. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index 49a1295f09..000e14af49 100644 --- a/xen/arch/x86/hvm/vmx/intr.c +++ b/xen/arch/x86/hvm/vmx/intr.c @@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v, struct hvm_intack intack) return 0; } +void vmx_sync_exit_bitmap(struct vcpu *v) +{ + const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); + unsigned int i; + + while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n)) < n ) + { + clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed); + __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]); + } +} + void vmx_intr_assist(void) { struct hvm_intack intack; @@ -318,7 +330,6 @@ void vmx_intr_assist(void) intack.source != hvm_intsrc_vector ) { unsigned long status; - unsigned int i, n; /* * intack.vector is the highest priority vector. So we set eoi_exit_bitmap @@ -379,13 +390,7 @@ void vmx_intr_assist(void) intack.vector; __vmwrite(GUEST_INTR_STATUS, status); - n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); - while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, - n)) < n ) - { - clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed); - __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]); - } + vmx_sync_exit_bitmap(v); pt_intr_post(v, intack); } diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 180d01e385..e041ecc115 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1414,6 +1414,7 @@ static void nvmx_update_apicv(struct vcpu *v) status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; __vmwrite(GUEST_INTR_STATUS, status); + vmx_sync_exit_bitmap(v); } } diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index b334e1ec94..111ccd7e61 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -610,6 +610,8 @@ void update_guest_eip(void); void vmx_pi_per_cpu_init(unsigned int cpu); void vmx_pi_desc_fixup(unsigned int cpu); +void vmx_sync_exit_bitmap(struct vcpu *v); + #ifdef CONFIG_HVM void vmx_pi_hooks_assign(struct domain *d); void vmx_pi_hooks_deassign(struct domain *d);
Current code in nvmx_update_apicv set the guest interrupt status field but doesn't update the exit bitmap, which can cause issues of lost interrupts on the L1 hypervisor if vmx_intr_assist gets short-circuited by nvmx_intr_intercept returning true. Extract the code to update the exit bitmap from vmx_intr_assist into a helper and use it in nvmx_update_apicv when updating the guest interrupt status field. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- xen/arch/x86/hvm/vmx/vvmx.c | 1 + xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ 3 files changed, 16 insertions(+), 8 deletions(-)