Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.01.15 at 15:25, <julien.grall@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -337,6 +337,13 @@ int iommu_do_domctl(
>      ret = iommu_do_pci_domctl(domctl, d, u_domctl);
>  #endif
>  
> +    if ( ret != -ENOSYS )
> +        return ret;
> +
> +#ifdef HAS_DEVICE_TREE
> +    ret = iommu_do_dt_domctl(domctl, d, u_domctl);
> +#endif

Please move the (inverted) if() into the #ifdef block (but also see
below regarding the specific error code used).

> @@ -1533,13 +1534,19 @@ int iommu_do_pci_domctl(
>      break;
>  
>      case XEN_DOMCTL_test_assign_device:
> -        ret = xsm_test_assign_device(XSM_HOOK, 
> domctl->u.assign_device.machine_sbdf);
> +        ret = -ENOSYS;
> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
> +            break;

I'm rather uncertain about the use of -ENOSYS here: The hypercall
isn't unimplemented after all. Provided you use consistent error
codes in the PCI and DT variants, there's no problem calling the
other variant if that specific error code got returned from the first
one - the second would then just return the same one again, even
if the first one failed on something other than the device type
check. As a result, I'd much prefer -ENODEV here.

> +
> +        machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
> +
> +        ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
>          if ( ret )
>              break;
>  
> -        seg = domctl->u.assign_device.machine_sbdf >> 16;
> -        bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
> -        devfn = domctl->u.assign_device.machine_sbdf & 0xff;
> +        seg = machine_sbdf >> 16;
> +        bus = (machine_sbdf >> 8) & 0xff;
> +        devfn = machine_sbdf & 0xff;

If you fiddle with these, please make them use at least PCI_BUS()
and PCI_DEVFN2() (we don't have a matching macro for retrieving
the segment).

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -475,12 +475,23 @@ typedef struct xen_domctl_sendtrigger 
> xen_domctl_sendtrigger_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
>  
>  
> -/* Assign PCI device to HVM guest. Sets up IOMMU structures. */
> +/* Assign a device to a guest. Sets up IOMMU structures. */
>  /* XEN_DOMCTL_assign_device */
>  /* XEN_DOMCTL_test_assign_device */
>  /* XEN_DOMCTL_deassign_device */
> +#define XEN_DOMCTL_DEV_PCI      0
> +#define XEN_DOMCTL_DEV_DT       1
>  struct xen_domctl_assign_device {
> -    uint32_t  machine_sbdf;   /* machine PCI ID of assigned device */
> +    uint32_t dev;   /* XEN_DOMCTL_DEV_* */
> +    union {
> +        struct {
> +            uint32_t machine_sbdf;   /* machine PCI ID of assigned device */
> +        } pci;
> +        struct {
> +            uint32_t size; /* Length of the path */
> +            XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node 
> */
> +        } dt;
> +    } u;
>  };

An incompatible change like this requires bumping
XEN_DOMCTL_INTERFACE_VERSION when not already done during
the current release cycle.

Jan


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