Re: [Xen-devel] [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 06/30/2015 12:24 PM, Chen, Tiejun wrote:
>>> +#define XEN_DOMCTL_DEV_NO_RDM           0
>>> +#define XEN_DOMCTL_DEV_RDM_RELAXED      1
>>> +#define XEN_DOMCTL_DEV_RDM_STRICT       2
>>> +    uint32_t  flag;   /* flag of assigned device */
>>
>> Normally flags would be bit fields, not values like this.
>>
>> Also, what's the distinction between RDM and RMRR, and is there a good
>> reason to use the first here rather than the second?
>>
>> It's also not clear to me what NO_RDM is meant to be for -- is it
>> meant to be an assertion that the caller expects the device to have no
>> RMRRs associated with it?
>>
> 
> All concerns what you're raising above just make me realized you're
> missing all background info and history changes. So I think if you
> really would like to review this series, at least you should take a look
> at our previous design and some basic change log, which are mentioned
> inside patch #00.

I did read #00, but I missed the RDM/RMRR thing.  I still don't see what
NO_RDM is for.

In any case, all the information needed to actually understand the code
needs to be checked into the tree, and patch 00 isn't going to be
checked in.  The choice about naming isn't important, but it should be
possible to look at the patch+changeset and figure out what NO_RDM is
supposed to be doing and why.

And finally, I have now looked through the patch history, and my initial
question was not covered: In the rest of domctl.h, "flags" is a bit
array of boolean values.  Here, at the moment, it is a tristate: 0, 1,
or 2.  There doesn't seem to be a plan for how to add in other flags --
are you going to have an "RDM_MASK" for bits 0-1, so bits 2-31 can be
used for something else?

This isn't super critical, since it is a domctl and we're allowed to
change it; but I think if we're going to be inconsistent we should at
least have consciously decided to do so for a reason.

 -George


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