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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/01/2015 02:11 AM, Chen, Tiejun wrote:
> On 2015/6/30 22:20, George Dunlap wrote:
>> 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
> 
> Thanks.
> 
>> 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.
> 
> From my point of view, "NO" should be clear at certain point, right?

Well, I'm afraid it's not.

Looking through the entire series, it *appears* that "NO_RDM" is meant
to be passed for architectures like ARM DeviceTree, where it is known
that no RDM regions can exist.

But it might also mean "I expect this device not to have any RDM
regions".  And it's certainly not immediately obvious what the effective
difference would be when I choose it -- what happens if I pass NO_RDM
for PCI systems?  How is it different than passing STRICT?

And in any case, as I said, reviewers and future archaeologists should
be able to tell from the individual patch what is meant, not have to go
through the entire series and guess.

> If you want delve into the reason why we called it, you can refer to,
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01223.html
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01747.html
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01793.html

As I said, all the information needed to understand the patch needs to
be in the changelog.  Those messages will not be in the changelog, so
they are irrelevant to my main complaint.

>> 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,
> 
> /* XEN_DOMCTL_createdomain */
> struct xen_domctl_createdomain {
>     /* IN parameters */
>     uint32_t ssidref;
>     xen_domain_handle_t handle;
>  /* Is this an HVM guest (as opposed to a PVH or PV guest)? */
> #define _XEN_DOMCTL_CDF_hvm_guest     0
> #define XEN_DOMCTL_CDF_hvm_guest      (1U<<_XEN_DOMCTL_CDF_hvm_guest)
>  /* Use hardware-assisted paging if available? */
> #define _XEN_DOMCTL_CDF_hap           1
> #define XEN_DOMCTL_CDF_hap            (1U<<_XEN_DOMCTL_CDF_hap)
>  /* Should domain memory integrity be verifed by tboot during Sx? */
> #define _XEN_DOMCTL_CDF_s3_integrity  2
> #define XEN_DOMCTL_CDF_s3_integrity   (1U<<_XEN_DOMCTL_CDF_s3_integrity)
>  /* Disable out-of-sync shadow page tables? */
> #define _XEN_DOMCTL_CDF_oos_off       3
> #define XEN_DOMCTL_CDF_oos_off        (1U<<_XEN_DOMCTL_CDF_oos_off)
>  /* Is this a PVH guest (as opposed to an HVM or PV guest)? */
> #define _XEN_DOMCTL_CDF_pvh_guest     4
> #define XEN_DOMCTL_CDF_pvh_guest      (1U<<_XEN_DOMCTL_CDF_pvh_guest)
>     uint32_t flags;

Yes, this demonstrates my point.  Each of these is a single-bit boolean
value that takes up a single bit -- either on or off.  But here you have
three values -- NO_DRM, RELAXED, and STRICT, that take up two bits.  If
you add more flags like this, then all the code which says "if (flags >
N)" will need to be changed to mask out the higher bits.

 -George