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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.07.15 at 13:30, <george.dunlap@xxxxxxxxxxxxx> 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

Jan


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