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

[Julien Grall <julien.grall@linaro.org>]

On 20/01/15 09:34, Jan Beulich wrote:
>>>> On 13.01.15 at 15:25, <julien.grall@linaro.org> 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).

What do you mean by the "inverted if()"?

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

I will use -ENODEV on the next version.

>> +
>> +        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).

Ok.

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

The XEN_DOMCTL_INTERFACE_VERSION will be bumped in patch #1 of this
series [1].

Regards,

[1] https://patches.linaro.org/43014/

-- 
Julien Grall