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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 07/14/2015 12:45 PM, Jan Beulich wrote:
>>>> 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

Very well.

 -George

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