Re: [Xen-devel] [PATCH 3/3] x86/nvmx: update exit bitmap on vmexit

["Tian, Kevin" <kevin.tian@intel.com>]

> 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