Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy

From: George Dunlap <george.dunlap@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>, Tiejun Chen <tiejun.chen@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	Ian Campbell <ian.campbell@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Yang Zhang <yang.z.zhang@intel.com>,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Subject: Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
Date: Tue, 14 Jul 2015 14:25:42 +0100	[thread overview]
Message-ID: <55A50DD6.4090104@eu.citrix.com> (raw)
In-Reply-To: <55A5128F0200007800090A59@mail.emea.novell.com>

On 07/14/2015 12:45 PM, Jan Beulich wrote:
>>>> On 14.07.15 at 13:30, <george.dunlap@eu.citrix.com> 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