Re: [Xen-devel] [PATCH v3] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 16, 2017 at 06:20:01AM -0600, Jan Beulich wrote:
> So far callers of the libxc interface passed in a domain ID which was
> then ignored in the hypervisor. Instead, make the hypervisor honor it
> (accepting DOMID_INVALID to obtain original behavior), allowing to
> query whether a device can be assigned to a particular domain.
> 
> Drop XSM's test_assign_{,dt}device hooks as no longer being
> individually useful.

Can you also say in the commit message that you consolidate some code as
well?

Assuming the disagreement on the semantics of the call is settled:

Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: Drop test-assign XSM hooks.
> v2: Alter the semantics to check whether the device can be assigned to
>     the passed in domain.
> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -391,11 +391,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      switch ( op->cmd )
>      {
> -    case XEN_DOMCTL_createdomain:
>      case XEN_DOMCTL_test_assign_device:
> +        if ( op->domain == DOMID_INVALID )
> +        {
> +    case XEN_DOMCTL_createdomain:
>      case XEN_DOMCTL_gdbsx_guestmemio:
> -        d = NULL;
> -        break;
> +            d = NULL;
> +            break;
> +        }
> +        /* fall through */

I know there is already code like this but I would rather not mix if and
case labels. Anyway, that's just my personal taste and I won't block
this patch because of that.

>      default:
>          d = rcu_lock_domain_by_id(op->domain);
>          if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -143,12 +143,15 @@ int iommu_do_dt_domctl(struct xen_domctl
>      switch ( domctl->cmd )
>      {
>      case XEN_DOMCTL_assign_device:
> +        ASSERT(d);
> +        /* fall through */
> +    case XEN_DOMCTL_test_assign_device:
>          ret = -ENODEV;
>          if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>              break;
>  
>          ret = -EINVAL;
> -        if ( d->is_dying || domctl->u.assign_device.flags )
> +        if ( (d && d->is_dying) || domctl->u.assign_device.flags )
>              break;
>  
>          ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> @@ -161,6 +164,17 @@ int iommu_do_dt_domctl(struct xen_domctl
>          if ( ret )
>              break;
>  
> +        if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> +        {
> +            if ( iommu_dt_device_is_assigned(dev) )
> +            {
> +                printk(XENLOG_G_ERR "%s already assigned.\n",
> +                       dt_node_full_name(dev));
> +                ret = -EINVAL;
> +            }
> +            break;
> +        }
> +

Move the ASSERT(d) here?

>          ret = iommu_assign_dt_device(d, dev);
>  


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