| Message ID | 20200325101910.29168-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/nvmx: fixes for interrupt injection | expand |
> From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Wednesday, March 25, 2020 6:19 PM > > Force an update of the EOI exit bitmap in nvmx_update_apicv, because > the one performed in vmx_intr_assist might not be reached if the > interrupt is intercepted 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. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Reword commit message. > --- > xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- > xen/arch/x86/hvm/vmx/vvmx.c | 2 ++ > xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ > 3 files changed, 17 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 8431c912a1..845dd87f75 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1418,6 +1418,8 @@ 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); Similarly, I'd like to do the sync within the conditional block, when intr status is actually changed. Otherwise, it becomes checking bitmap change in every vmentry when apicv is enabled. > } > > static void virtual_vmexit(struct cpu_user_regs *regs) > 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.26.0
On Thu, Mar 26, 2020 at 03:17:59AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: Wednesday, March 25, 2020 6:19 PM > > > > Force an update of the EOI exit bitmap in nvmx_update_apicv, because > > the one performed in vmx_intr_assist might not be reached if the > > interrupt is intercepted 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. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - Reword commit message. > > --- > > xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- > > xen/arch/x86/hvm/vmx/vvmx.c | 2 ++ > > xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ > > 3 files changed, 17 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 8431c912a1..845dd87f75 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -1418,6 +1418,8 @@ 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); > > Similarly, I'd like to do the sync within the conditional block, when intr > status is actually changed. Otherwise, it becomes checking bitmap change > in every vmentry when apicv is enabled. No - it will only check the bitmap when there's a virtual vmexit (which is where nvmx_update_apicv gets called), not on every vmentry. I can try to do this conditionally on whether GUEST_INTR_STATUS is actually changed. Thanks, Roger.
> From: Roger Pau Monné <roger.pau@citrix.com> > Sent: Thursday, March 26, 2020 5:22 PM > > On Thu, Mar 26, 2020 at 03:17:59AM +0000, Tian, Kevin wrote: > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > Sent: Wednesday, March 25, 2020 6:19 PM > > > > > > Force an update of the EOI exit bitmap in nvmx_update_apicv, because > > > the one performed in vmx_intr_assist might not be reached if the > > > interrupt is intercepted 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. > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > Changes since v1: > > > - Reword commit message. > > > --- > > > xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- > > > xen/arch/x86/hvm/vmx/vvmx.c | 2 ++ > > > xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ > > > 3 files changed, 17 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 8431c912a1..845dd87f75 100644 > > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > > @@ -1418,6 +1418,8 @@ 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); > > > > Similarly, I'd like to do the sync within the conditional block, when intr > > status is actually changed. Otherwise, it becomes checking bitmap change > > in every vmentry when apicv is enabled. > > No - it will only check the bitmap when there's a virtual vmexit > (which is where nvmx_update_apicv gets called), not on every vmentry. you are right. I overlooked it. but still it's unnecessary to do it for every virtual vmexit.
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 8431c912a1..845dd87f75 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1418,6 +1418,8 @@ 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); } static void virtual_vmexit(struct cpu_user_regs *regs) 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);
Force an update of the EOI exit bitmap in nvmx_update_apicv, because the one performed in vmx_intr_assist might not be reached if the interrupt is intercepted 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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Reword commit message. --- xen/arch/x86/hvm/vmx/intr.c | 21 +++++++++++++-------- xen/arch/x86/hvm/vmx/vvmx.c | 2 ++ xen/include/asm-x86/hvm/vmx/vmx.h | 2 ++ 3 files changed, 17 insertions(+), 8 deletions(-)