From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy Date: Tue, 30 Jun 2015 15:20:08 +0100 Message-ID: <5592A598.60001@eu.citrix.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55927C78.10301@intel.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: "Chen, Tiejun" 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 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