Re: [Xen-devel] [PATCH v8 2/7] VMX: Properly handle pi when all the assigned devices are removed

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

> From: Wu, Feng
> Sent: Friday, November 18, 2016 9:57 AM
> 
> This patch handles some corner cases when the last assigned device
> is removed from the domain. In this case we should carefully handle
> pi descriptor and the per-cpu blocking list, to make sure:
> - all the PI descriptor are in the right state when next time a
> devices is assigned to the domain again.
> - No remaining vcpus of the domain in the per-cpu blocking list.
> 
> Here we call vmx_pi_unblock_vcpu() to remove the vCPU from the blocking list
> if it is on the list. However, this could happen when vmx_vcpu_block() is
> being called, hence we might incorrectly add the vCPU to the blocking list
> while the last devcie is detached from the domain. Consider that the situation
> can only occur when detaching the last device from the domain and it is not
> a frequent operation, so we use domain_pause before that, which is considered
> as an clean and maintainable solution for the situation.
> 
> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v7:
> - Prevent the domain from pausing itself.
> 
> v6:
> - Comments changes
> - Rename vmx_pi_list_remove() to vmx_pi_unblock_vcpu()
> 
> v5:
> - Remove a no-op wrapper
> 
> v4:
> - Rename some functions:
>       vmx_pi_remove_vcpu_from_blocking_list() -> vmx_pi_list_remove()
>       vmx_pi_blocking_cleanup() -> vmx_pi_list_cleanup()
> - Remove the check in vmx_pi_list_cleanup()
> - Comments adjustment
> 
>  xen/arch/x86/hvm/vmx/vmx.c    | 28 ++++++++++++++++++++++++----
>  xen/drivers/passthrough/pci.c | 14 ++++++++++++++
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index f3911f2..a8dcabe 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -158,14 +158,12 @@ static void vmx_pi_switch_to(struct vcpu *v)
>      pi_clear_sn(pi_desc);
>  }
> 
> -static void vmx_pi_do_resume(struct vcpu *v)
> +static void vmx_pi_unblock_vcpu(struct vcpu *v)
>  {
>      unsigned long flags;
>      spinlock_t *pi_blocking_list_lock;
>      struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> 
> -    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> -
>      /*
>       * Set 'NV' field back to posted_intr_vector, so the
>       * Posted-Interrupts can be delivered to the vCPU when
> @@ -173,12 +171,12 @@ static void vmx_pi_do_resume(struct vcpu *v)
>       */
>      write_atomic(&pi_desc->nv, posted_intr_vector);
> 
> -    /* The vCPU is not on any blocking list. */
>      pi_blocking_list_lock = v->arch.hvm_vmx.pi_blocking.lock;
> 
>      /* Prevent the compiler from eliminating the local variable.*/
>      smp_rmb();
> 
> +    /* The vCPU is not on any blocking list. */
>      if ( pi_blocking_list_lock == NULL )
>          return;
> 
> @@ -198,6 +196,13 @@ static void vmx_pi_do_resume(struct vcpu *v)
>      spin_unlock_irqrestore(pi_blocking_list_lock, flags);
>  }
> 
> +static void vmx_pi_do_resume(struct vcpu *v)
> +{
> +    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> +
> +    vmx_pi_unblock_vcpu(v);
> +}
> +
>  /* This function is called when pcidevs_lock is held */
>  void vmx_pi_hooks_assign(struct domain *d)
>  {
> @@ -215,11 +220,21 @@ void vmx_pi_hooks_assign(struct domain *d)
>  /* This function is called when pcidevs_lock is held */
>  void vmx_pi_hooks_deassign(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>      if ( !iommu_intpost || !has_hvm_container_domain(d) )
>          return;
> 
>      ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
> 
> +    /*
> +     * Pausing the domain can make sure the vCPU is not

"the vCPUs are"

> +     * running and hence not calling the hooks simultaneously
> +     * when deassigning the PI hooks and removing the vCPU
> +     * from the blocking list.
> +     */
> +    domain_pause(d);
> +
>      d->arch.hvm_domain.vmx.vcpu_block = NULL;
>      d->arch.hvm_domain.vmx.pi_switch_from = NULL;
>      d->arch.hvm_domain.vmx.pi_do_resume = NULL;
> @@ -230,6 +245,11 @@ void vmx_pi_hooks_deassign(struct domain *d)
>       * assigned and "from" hook is NULL. However, it is not straightforward
>       * to find a clear solution, so just leave it here.
>       */
> +
> +    for_each_vcpu ( d, v )
> +        vmx_pi_unblock_vcpu(v);
> +
> +    domain_unpause(d);
>  }
> 
>  static int vmx_domain_initialise(struct domain *d)
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 8bce213..e71732f 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1602,6 +1602,13 @@ int iommu_do_pci_domctl(
>          break;
> 
>      case XEN_DOMCTL_assign_device:
> +        /* no domain_pause() */
> +        if ( d == current->domain )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +

don't understand why adding above check, and why "no domain_pause"
matters in this change.

>          ret = -ENODEV;
>          if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
>              break;
> @@ -1642,6 +1649,13 @@ int iommu_do_pci_domctl(
>          break;
> 
>      case XEN_DOMCTL_deassign_device:
> +        /* no domain_pause() */
> +        if ( d == current->domain )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
>          ret = -ENODEV;
>          if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
>              break;
> --
> 2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel