[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [v3][PATCH 04/16] xen/passthrough: extend hypercall to support rdm reservation policy
>>> On 18.06.15 at 10:48, <tiejun.chen@xxxxxxxxx> wrote: > On 2015/6/18 15:53, Jan Beulich wrote: >>>>> 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. > > I just think XEN_DOMCTL_DEV_NO_RDM is prone to represent a sort of > ending of all flags, and I also add this comment, > > /* Make sure this is always the last. */ > >> >>> 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. > > This place really isn't one spotlight to take a attention when a new > flag is introduced, right? So what I intend to is trying to make sure we > don't need to change this. Anyone adding a new value will need to test their code. And this testing would not succeed without the range check above having got adjusted. >>> { >>> 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. >> > > Do you mean I should simplify this log message? Or remove completely? Remove. (And I think you generally need to reduce verbosity of your additions - please don't mix up what might be useful for your debugging with what will be useful once the code went in.) Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |