From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 1/6] xenbus: Support HVM backends Date: Mon, 19 Dec 2011 13:46:07 -0500 Message-ID: <4EEF866F.1030002@tycho.nsa.gov> 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> <20111219181655.GA3832@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20111219181655.GA3832@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Konrad Rzeszutek Wilk Cc: Konrad Rzeszutek Wilk , Xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On 12/19/2011 01:16 PM, Konrad Rzeszutek Wilk wrote: > 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? Not all that often - domain creation and destruction or device plug/unplug. So performance doesn't really matter. Is there a reason to prefer an _ops structure for this instead of direct calls?