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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.17 at 17:25, <wei.liu2@xxxxxxxxxx> wrote:
> 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?

Am I consolidating code beyond what is reasonable to achieve
the intended effect? I don't view the merging of the two case
blocks 

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

Understood. I, otoh, prefer this style to limit code duplication.

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

That would be a possibility, but personally I think it's better placed
where it is now. It helps, for example, understanding why there is
a NULL check of d somewhere in the middle. In a domctl handler d
being NULL isn't a usual thing.

Jan


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