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

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

On 2015/7/1 18:02, George Dunlap wrote:
> 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?
Currently NO_RDM is just used and specific to non-x86 inside Xen, not 
tools. So actually we don't pass this. If someone want to extend this 
usage in the future he really should take into account.
> 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.
Sorry I'm missing this changelog.

> > > 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
Is this fine to you?

#define _XEN_DOMCTL_DEV_NO_RDM          0
#define XEN_DOMCTL_DEV_NO_RDM           (1U<<_XEN_DOMCTL_DEV_NO_RDM)
#define _XEN_DOMCTL_DEV_RDM_RELAXED     1
#define XEN_DOMCTL_DEV_RDM_RELAXED      (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED)
#define _XEN_DOMCTL_DEV_RDM_STRICT      2
#define XEN_DOMCTL_DEV_RDM_STRICT       (1U<<_XEN_DOMCTL_DEV_RDM_STRICT)


> 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.
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02950.html

Thanks
Tiejun

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