From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Date: Tue, 14 Jul 2015 14:25:42 +0100 Message-ID: <55A50DD6.4090104@eu.citrix.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-4-git-send-email-tiejun.chen@intel.com> <55A35EFA.9030304@intel.com> <55A4E88E.9060502@eu.citrix.com> <55A4EA21.4020909@intel.com> <55A4F2D9.3070309@eu.citrix.com> <55A5128F0200007800090A59@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A5128F0200007800090A59@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 , Tiejun Chen Cc: Kevin Tian , Keir Fraser , Ian Campbell , Andrew Cooper , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Suravee Suthikulpanit , Yang Zhang , Aravind Gopalakrishnan List-Id: xen-devel@lists.xenproject.org On 07/14/2015 12:45 PM, Jan Beulich wrote: >>>> On 14.07.15 at 13:30, wrote: >> On 07/14/2015 11:53 AM, Chen, Tiejun wrote: >>>> The way this sort of thing is defined in the rest of domctl.h is like >>>> this: >>>> >>>> #define _XEN_DOMCTL_CDF_hvm_guest 0 >>>> #define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest) >>>> >>>> So the above should be >>>> >>>> #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 >>>> #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) >>>> >>>> And then your check in iommu_do_pci_domctl() would look like >>>> >>>> if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED) >>>> >>>> And if we end up adding any extra flags, we just | them into the above >>>> conditional, as is done in, for example, the XEN_DOMCTL_createdomain >>>> case in xen/common/domctl.c:do_domctl(). >>>> >>> >>> Seems Jan didn't like this way IIRC, so I hope Jan also can have a look >>> at this beforehand :) >> >> I think Jan thought that the MASK value you defined wasn't meant to be a >> single flag, but for all the flags; i.e., that if we added flags in bits >> 1 and 2, that MASK would become 0x7 rather than 0x1. And I agree that >> there's not much point to having such a mask defined in the public header. >> >> But what I'm doing above is making explicit what you have already; i.e., >> you just set XEN_DOMCTL_DEV_RDM_RELAXED to '1'; the reader has to sort >> of infer that the reason '1' is chosen is that it's setting bit 0. >> Doing it the way I suggest makes it more clear that this is meant to be >> a bitfield, and '0' has been allocated. >> >> Please correct me if I'm wrong, Jan. > > Indeed my primary objection was to what you describe. That said, > I'm not too happy with what you propose now either. Not only do > I view this (bit-pos,bit-mask) tuple as redundant unless one actually > needs both in certain places (which doesn't seem to be the case > here), but the naming also conflicts with the C standard (reserving > identifiers starting with underscore and an upper case letter). Just > like said in various other cases - we've got many examples of such > violations already, but I'd prefer not to make the situation worse. > > IOW I'd prefer to go with just > > #define XEN_DOMCTL_DEV_RDM_RELAXED 1 Very well. -George