From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 1/6] xenbus: Support HVM backends Date: Mon, 19 Dec 2011 13:16:55 -0500 Message-ID: <20111219181655.GA3832@phenom.dumpdata.com> References: <1323893534-436-1-git-send-email-dgdegra@tycho.nsa.gov> <1323893534-436-2-git-send-email-dgdegra@tycho.nsa.gov> <20111216195620.GB26802@andromeda.dapyr.net> <4EEF7993.2060301@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4EEF7993.2060301@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Daniel De Graaf Cc: Konrad Rzeszutek Wilk , Xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Mon, Dec 19, 2011 at 12:51:15PM -0500, Daniel De Graaf wrote: > On 12/16/2011 02:56 PM, Konrad Rzeszutek Wilk wrote: > >On Wed, Dec 14, 2011 at 03:12:09PM -0500, Daniel De Graaf wrote: > >>Add HVM implementations of xenbus_(map,unmap)_ring_v(alloc,free) so > >>that ring mappings can be done without using GNTMAP_contains_pte which > >>is not supported on HVM. > >> > >>Signed-off-by: Daniel De Graaf > >>--- > >> drivers/xen/xenbus/xenbus_client.c | 155 +++++++++++++++++++++++++++++------- > >> 1 files changed, 125 insertions(+), 30 deletions(-) > >> > >>diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c > >>index 1906125..ecd695d 100644 > >>--- a/drivers/xen/xenbus/xenbus_client.c > >>+++ b/drivers/xen/xenbus/xenbus_client.c > >>@@ -32,16 +32,27 @@ > >> > >> #include > >> #include > >>+#include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >>+#include > >> #include > >> #include > >> #include > >> > >>+struct xenbus_map_node { > >>+ struct list_head next; > >>+ struct page *page; > >>+ grant_handle_t handle; > >>+}; > >>+ > >>+static DEFINE_SPINLOCK(xenbus_valloc_lock); > >>+static LIST_HEAD(xenbus_valloc_pages); > > > >Could we use this for what the PV version of xenbus_unmap_ring_vfree > >does? (where it uses the vmlist_lock to look for the appropiate vaddr). > > > >Could the vmlist_lock be removed and instead we can use this structure > >for both PV and HVM? > > Yes, the next version will do this. > > [...] > >>+ > >>+/** > >>+ * xenbus_map_ring_valloc > >>+ * @dev: xenbus device > >>+ * @gnt_ref: grant reference > >>+ * @vaddr: pointer to address to be filled out by mapping > >>+ * > >>+ * Based on Rusty Russell's skeleton driver's map_page. > >>+ * Map a page of memory into this domain from another domain's grant table. > >>+ * xenbus_map_ring_valloc allocates a page of virtual address space, maps the > >>+ * page to that address, and sets *vaddr to that address. > >>+ * Returns 0 on success, and GNTST_* (see xen/include/interface/grant_table.h) > >>+ * or -ENOMEM on error. If an error is returned, device will switch to > >>+ * XenbusStateClosing and the error message will be saved in XenStore. > >>+ */ > >>+int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr) > >>+{ > >>+ if (xen_pv_domain()) > >>+ return xenbus_map_ring_valloc_pv(dev, gnt_ref, vaddr); > >>+ else > >>+ return xenbus_map_ring_valloc_hvm(dev, gnt_ref, vaddr); > > > >This could be done in a similar way which Annie's granttable v2 patches are done. > > > >Meaning define something like: > > > >struct xenbus_ring_ops { > > int (*map)(struct xenbus_device *dev, int gnt, void *vaddr); > > ... > > > >And then define two variants of it (PV and HVM): > > > >struct xenbus_ring_ops pv_ring_ops = { > > .map = xenbus_map_ring_valloc_pv; > > .. > >} > > > >have a static to which either one will be assigned: > > > >static struct xenbus_ring_ops *ring_ops; > > > >And in the init function: > > > >void __init xenbus_ring_ops_init(void) > >{ > > if (xen_hvm_domain) > > ring_ops = hvm_ring_ops; > > else > > ... > > > >And then xenbus_init() calls the xenbus_rings_ops_init(). > > > >Then these 'xenbus_map_ring_valloc' end up just using the > >ring_ops->map call. > > Is the reason for doing this just to avoid overhead? The overhead from > an indirect function call is much higher than from an integer comparison > (which is what xen_pv_domain resolves to). In this case, the _pv and _hvm > functions are both inlined into the dispatch function. Do we care about that? How often are these calls made?