From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 04/16] xen/passthrough: extend hypercall to support rdm reservation policy Date: Thu, 18 Jun 2015 17:31:00 +0800 Message-ID: <55828FD4.4080302@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-5-git-send-email-tiejun.chen@intel.com> <558163F70200007800086058@mail.emea.novell.com> <55826FDE.7030501@intel.com> <558295280200007800086701@mail.emea.novell.com> <558285EF.6030701@intel.com> <5582A7E902000078000867EF@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5582A7E902000078000867EF@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: yang.z.zhang@intel.com, andrew.cooper3@citrix.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2015/6/18 17:13, Jan Beulich wrote: >>>> On 18.06.15 at 10:48, wrote: >> On 2015/6/18 15:53, Jan Beulich wrote: >>>>>> On 18.06.15 at 09:14, wrote: >>>> On 2015/6/17 18:11, Jan Beulich wrote: >>>>>>>> On 11.06.15 at 03:15, 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