Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv
> From: Roger Pau Monné > Sent: Thursday, March 26, 2020 5:20 PM > > On Thu, Mar 26, 2020 at 03:13:56AM +, Tian, Kevin wrote: > > > From: Roger Pau Monne > > > Sent: Wednesday, March 25, 2020 6:19 PM > > > > > > Updating SVI is required when an interrupt has been injected using the > > > Ack on exit VMEXIT feature, so that the in service interrupt in the > > > GUEST_INTR_STATUS matches the vector that is signaled in > > > VM_EXIT_INTR_INFO. > > > > > > Updating RVI however is not tied to the Ack on exit feature, as it > > > signals the next vector to be injected, and hence should always be > > > updated to the next pending vector, regardless of whether Ack on exit > > > is enabled. > > > > > > Signed-off-by: Roger Pau Monné > > > --- > > > xen/arch/x86/hvm/vmx/vvmx.c | 22 +- > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c > b/xen/arch/x86/hvm/vmx/vvmx.c > > > index 1753005c91..8431c912a1 100644 > > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > > @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu > *v) > > > struct nestedvmx *nvmx = _2_nvmx(v); > > > unsigned long reason = get_vvmcs(v, VM_EXIT_REASON); > > > unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO); > > > +int rvi; > > > > > > 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; > > > +uint32_t ppr; > > > +unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK; > > > struct vlapic *vlapic = vcpu_vlapic(v); > > > > > > +/* > > > + * Update SVI to record the currently in service interrupt that's > > > + * signaled in EXIT_INTR_INFO. > > > + */ > > > 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); > > > +} > > > + > > > +rvi = vlapic_has_pending_irq(v); > > > +if ( rvi != -1 ) > > > +{ > > > +unsigned long status; > > > > > > +__vmread(GUEST_INTR_STATUS, ); > > > +status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > > > +status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > > > __vmwrite(GUEST_INTR_STATUS, status); > > > } > > > > I have two minor comments. First, original code requires only one > > __vmwrite but now needs two writes and one read for Ack-on-exit > > is set. Can we optimize to sustain the original behavior? > > I think I can manage to do a single write by using a global status > variable initialized to 0 and only do the vmwrite if the value is != 0 > by the end of the function. > > > Second, > > although I didn't come up a case where always updating RVI (w/ > > pending interrupt) may bring some bad effect, it is anyway safer > > to put the whole logic within the check of vmexit reason and intr > > source. > > I also considered this and decided there was no harm in always > updating RVI if there's a pending interrupt, it could even prevent a > future VMEXIT to update RVI AFAICT? > > If you prefer I can return early from the function if reason != > EXIT_REASON_EXTERNAL_INTERRUPT. > I agree with your but given the trickiness of nested intr handling I prefer to a more conservative but safer approach. So yes, return early sounds good here. Thanks Kevin
Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv
On Thu, Mar 26, 2020 at 03:13:56AM +, Tian, Kevin wrote: > > From: Roger Pau Monne > > Sent: Wednesday, March 25, 2020 6:19 PM > > > > Updating SVI is required when an interrupt has been injected using the > > Ack on exit VMEXIT feature, so that the in service interrupt in the > > GUEST_INTR_STATUS matches the vector that is signaled in > > VM_EXIT_INTR_INFO. > > > > Updating RVI however is not tied to the Ack on exit feature, as it > > signals the next vector to be injected, and hence should always be > > updated to the next pending vector, regardless of whether Ack on exit > > is enabled. > > > > Signed-off-by: Roger Pau Monné > > --- > > xen/arch/x86/hvm/vmx/vvmx.c | 22 +- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > index 1753005c91..8431c912a1 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu *v) > > struct nestedvmx *nvmx = _2_nvmx(v); > > unsigned long reason = get_vvmcs(v, VM_EXIT_REASON); > > unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO); > > +int rvi; > > > > 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; > > +uint32_t ppr; > > +unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK; > > struct vlapic *vlapic = vcpu_vlapic(v); > > > > +/* > > + * Update SVI to record the currently in service interrupt that's > > + * signaled in EXIT_INTR_INFO. > > + */ > > 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); > > +} > > + > > +rvi = vlapic_has_pending_irq(v); > > +if ( rvi != -1 ) > > +{ > > +unsigned long status; > > > > +__vmread(GUEST_INTR_STATUS, ); > > +status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > > +status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > > __vmwrite(GUEST_INTR_STATUS, status); > > } > > I have two minor comments. First, original code requires only one > __vmwrite but now needs two writes and one read for Ack-on-exit > is set. Can we optimize to sustain the original behavior? I think I can manage to do a single write by using a global status variable initialized to 0 and only do the vmwrite if the value is != 0 by the end of the function. > Second, > although I didn't come up a case where always updating RVI (w/ > pending interrupt) may bring some bad effect, it is anyway safer > to put the whole logic within the check of vmexit reason and intr > source. I also considered this and decided there was no harm in always updating RVI if there's a pending interrupt, it could even prevent a future VMEXIT to update RVI AFAICT? If you prefer I can return early from the function if reason != EXIT_REASON_EXTERNAL_INTERRUPT. Thanks, Roger.
Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv
> From: Roger Pau Monne > Sent: Wednesday, March 25, 2020 6:19 PM > > Updating SVI is required when an interrupt has been injected using the > Ack on exit VMEXIT feature, so that the in service interrupt in the > GUEST_INTR_STATUS matches the vector that is signaled in > VM_EXIT_INTR_INFO. > > Updating RVI however is not tied to the Ack on exit feature, as it > signals the next vector to be injected, and hence should always be > updated to the next pending vector, regardless of whether Ack on exit > is enabled. > > Signed-off-by: Roger Pau Monné > --- > xen/arch/x86/hvm/vmx/vvmx.c | 22 +- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 1753005c91..8431c912a1 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu *v) > struct nestedvmx *nvmx = _2_nvmx(v); > unsigned long reason = get_vvmcs(v, VM_EXIT_REASON); > unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO); > +int rvi; > > 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; > +uint32_t ppr; > +unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK; > struct vlapic *vlapic = vcpu_vlapic(v); > > +/* > + * Update SVI to record the currently in service interrupt that's > + * signaled in EXIT_INTR_INFO. > + */ > 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); > +} > + > +rvi = vlapic_has_pending_irq(v); > +if ( rvi != -1 ) > +{ > +unsigned long status; > > +__vmread(GUEST_INTR_STATUS, ); > +status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > +status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK; > __vmwrite(GUEST_INTR_STATUS, status); > } I have two minor comments. First, original code requires only one __vmwrite but now needs two writes and one read for Ack-on-exit is set. Can we optimize to sustain the original behavior? Second, although I didn't come up a case where always updating RVI (w/ pending interrupt) may bring some bad effect, it is anyway safer to put the whole logic within the check of vmexit reason and intr source. Thanks Kevin