From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy Date: Wed, 01 Jul 2015 18:47:32 +0800 Message-ID: <5593C544.2030009@intel.com> References: <1435053450-25131-1-git-send-email-tiejun.chen@intel.com> <1435053450-25131-5-git-send-email-tiejun.chen@intel.com> <55927C78.10301@intel.com> <5592A598.60001@eu.citrix.com> <55933E4D.4030405@intel.com> <5593BAAB.20801@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5593BAAB.20801@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Kevin Tian , Keir Fraser , Suravee Suthikulpanit , Andrew Cooper , Tim Deegan , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , Jan Beulich , Yang Zhang , Stefano Stabellini , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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