Re: [Xen-devel] [v3][PATCH 04/16] xen/passthrough: extend hypercall to support rdm reservation policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.06.15 at 09:14, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/6/17 18:11, Jan Beulich wrote:
>>>>> On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> wrote:
>>> @@ -1577,9 +1578,10 @@ int iommu_do_pci_domctl(
>>>           seg = machine_sbdf >> 16;
>>>           bus = PCI_BUS(machine_sbdf);
>>>           devfn = PCI_DEVFN2(machine_sbdf);
>>> +        flag = domctl->u.assign_device.flag;
>>>
>>>           ret = device_assigned(seg, bus, devfn) ?:
>>> -              assign_device(d, seg, bus, devfn);
>>> +              assign_device(d, seg, bus, devfn, flag);
>>
>> I think you should range check the flag passed to make future
>> extensions possible (and to avoid ambiguity on what out of
>> range values would mean).
> 
> Yeah.
> 
> Maybe I can set this comment,
> 
>      /* Make sure this is always the last. */ 
> 
> #define XEN_DOMCTL_DEV_NO_RDM           2 
> 
>      uint32_t  flag;   /* flag of assigned device */

Why would you want to needlessly break the interface is a new
constant gets added? It's a domctl, so it can be changed, but we
shouldn't change for no reason.

> and then
> 
>          flag = domctl->u.assign_device.flag;
>          if ( flag > XEN_DOMCTL_DEV_NO_RDM )

All that needs updating when a new constant gets added is this
line.

>          {
>              printk(XENLOG_G_ERR "XEN_DOMCTL_assign_device: "
>                     "assign %04x:%02x:%02x.%u to dom%d failed "
>                     "with unknown rdm flag %x. (%d)\n",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>                     d->domain_id, flag, ret);

I see absolutely no reason for such a log message.

Jan


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