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: Thu, 02 Jul 2015 14:50:05 +0800 Message-ID: <5594DF1D.3080803@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> <5593C544.2030009@intel.com> <5593FB94.50803@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: <5593FB94.50803@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 , Julien Grall , Tim Deegan , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , Jan Beulich , Yang Zhang , Stefano Stabellini , Ian Campbell List-Id: xen-devel@lists.xenproject.org > When you say "not tools", I take it to mean that you're not exposing > that option through the libxl interface? Yes. > > tools/libxc/xc_domain.c:xc_assign_dt_device() most certainly does pass > it in, and that's the level I'm talking about. Someone reviewing this > patch series needs to know, when xc or libxl set NO_RDM, what will be > the effect? The fact that libxc *shouldn't* set NO_RDM for PCI devices > doesn't mean it won't happen. > > Now looking at the end of the series and grepping for > "XEN_DOMCTL_DEV.*RDM", these values are *read and acted on* in exactly > two places: > > xen/arch/x86/mm/p2m.c: The whole point of this series; if the p2m is > occupied already, and flag == RDM_STRICT, return an error; otherwise > ignore it. > > xen/drivers/passthrough/device_tree.c: If flag != NO_RDM, return an error. > > So the meaning of the flags is: > For pci devices: > - RDM_RELAXED, NO_RDM: ignore conflicts in set_identity_p2m_entry() > - RDM_STRICT: error on conflicts in set_identity_p2m_entry() > for dt devices: > - Error if not NO_RDM Correct. > > It doesn't look to me like the NO_RDM setting actually adds any semantic > meaning. > > What I see in the list of references you gave is a request from the list > below is Julien saying this: > > "I would also add a XEN_DOMCTL_DEV_NO_RDM that would be use for non-PCI > assignment." > > It looks a bit like what you did is said, "Well Julien asked for a > NO_RDM setting, so here's a NO_RDM setting." Which while perhaps > understandable, doesn't make the value have any more usefulness. > > It seems to me that the real problem is that you had two values to begin > with, rather than actually having flags (as the name would imply). > > This what I would suggest. Make a single flag: > > #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 > #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) > > Then make the meaning of the flags as follows: > * for pci devices: > - RDM_RELAXED flag SET: ignore conflicts in set_identity_p2m_entry() > - RDM_RELAXED flag CLEAR: error on conflicts in set_identity_p2m_entry() No problem. > * for dt devices: > - Ignore this flag entirely But we still a flag to assign_device() like this, diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 5d3842a..a182487 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -52,7 +52,8 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev) goto fail; } - rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev)); + rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), + XEN_DOMCTL_DEV_RDM_RELAXED); if ( rc ) goto fail; Or rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), 0)? Thanks Tiejun