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

["Chen, Tiejun" <tiejun.chen@intel.com>]

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?

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

>
> 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;

> 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

Its very possible to introduce some new flags to address some 
complicated cases like live migration, and this is really what we want 
to step next.

> used for something else?

I think already we have a comment here,

uint32_t  flag;   /* flag of assigned device */

This clarifies explicitly this viable is dedicated to be as a flag.

>
> 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.
>

Just see above, if I'm wrong please correct me.

Thanks
Tiejun