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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 20 Jan 2015, Jan Beulich wrote:
> >>> 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).

Maybe we should?



> > --- 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