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 16:48:47 +0800 Message-ID: <558285EF.6030701@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <558295280200007800086701@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 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. > >> { >> 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? Thanks Tiejun