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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/18 17:13, Jan Beulich wrote:
> > > > 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.
Okay.

> > > >            {
> > > >                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.)
Yes, I should follow this rule.

Thanks
Tiejun

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