[PATCH v3 0/6] xen/{net, blk}back support for running in HVM

[PATCH v3 0/6] xen/{net, blk}back support for running in HVM

[Daniel De Graaf]

Changes from v2:
 - Clarified comments and commit messages
 - Based on v3.2-rc3

Changes from v1:
 - Rebased on top of David's patches
 - Grant table wrapper functions used where appropriate

[PATCH 1/6] xenbus: Support HVM backends
[PATCH 2/6] xenbus: Use grant-table wrapper functions
[PATCH 3/6] xen/grant-table: Support mappings required by blkback
[PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers
[PATCH 5/6] xen/netback: Enable netback on HVM guests
[PATCH 6/6] xen/blkback: Enable blkback on HVM guests

[PATCH 1/6] xenbus: Support HVM backends

[Daniel De Graaf]

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 <dgdegra@tycho.nsa.gov>
---
 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 <linux/slab.h>
 #include <linux/types.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
+#include <xen/balloon.h>
 #include <xen/events.h>
 #include <xen/grant_table.h>
 #include <xen/xenbus.h>
 
+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);
+
 const char *xenbus_strstate(enum xenbus_state state)
 {
 	static const char *const name[] = {
@@ -420,21 +431,8 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)
 EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
 
 
-/**
- * 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)
+static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
+                                     int gnt_ref, void **vaddr)
 {
 	struct gnttab_map_grant_ref op = {
 		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
@@ -469,6 +467,64 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
 	*vaddr = area->addr;
 	return 0;
 }
+
+static int xenbus_map_ring_valloc_hvm(struct xenbus_device *dev,
+                                     int gnt_ref, void **vaddr)
+{
+	struct xenbus_map_node *node;
+	int err;
+	void *addr;
+
+	*vaddr = NULL;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	err = alloc_xenballooned_pages(1, &node->page, false /* lowmem */);
+	if (err)
+		goto out_err;
+
+	addr = pfn_to_kaddr(page_to_pfn(node->page));
+
+	err = xenbus_map_ring(dev, gnt_ref, &node->handle, addr);
+	if (err)
+		goto out_err;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
+
+	*vaddr = addr;
+	return 0;
+
+ out_err:
+	free_xenballooned_pages(1, &node->page);
+	kfree(node);
+	return err;
+}
+
+/**
+ * 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);
+}
 EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
 
 
@@ -510,20 +566,7 @@ int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
 }
 EXPORT_SYMBOL_GPL(xenbus_map_ring);
 
-
-/**
- * xenbus_unmap_ring_vfree
- * @dev: xenbus device
- * @vaddr: addr to unmap
- *
- * Based on Rusty Russell's skeleton driver's unmap_page.
- * Unmap a page of memory in this domain that was imported from another domain.
- * Use xenbus_unmap_ring_vfree if you mapped in your memory with
- * xenbus_map_ring_valloc (it will free the virtual address space).
- * Returns 0 on success and returns GNTST_* on error
- * (see xen/include/interface/grant_table.h).
- */
-int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
+static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
 {
 	struct vm_struct *area;
 	struct gnttab_unmap_grant_ref op = {
@@ -566,8 +609,60 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
 
 	return op.status;
 }
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
+static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
+{
+	int rv;
+	struct xenbus_map_node *node;
+	void *addr;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_for_each_entry(node, &xenbus_valloc_pages, next) {
+		addr = pfn_to_kaddr(page_to_pfn(node->page));
+		if (addr == vaddr) {
+			list_del(&node->next);
+			goto found;
+		}
+	}
+	node = NULL;
+ found:
+	spin_unlock(&xenbus_valloc_lock);
+
+	if (!node) {
+		xenbus_dev_error(dev, -ENOENT,
+				 "can't find mapped virtual address %p", vaddr);
+		return -ENOENT;
+	}
+
+	rv = xenbus_unmap_ring(dev, node->handle, addr);
+
+	if (!rv)
+		free_xenballooned_pages(1, &node->page);
+
+	kfree(node);
+	return rv;
+}
+
+/**
+ * xenbus_unmap_ring_vfree
+ * @dev: xenbus device
+ * @vaddr: addr to unmap
+ *
+ * Based on Rusty Russell's skeleton driver's unmap_page.
+ * Unmap a page of memory in this domain that was imported from another domain.
+ * Use xenbus_unmap_ring_vfree if you mapped in your memory with
+ * xenbus_map_ring_valloc (it will free the virtual address space).
+ * Returns 0 on success and returns GNTST_* on error
+ * (see xen/include/interface/grant_table.h).
+ */
+int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
+{
+	if (xen_pv_domain())
+		return xenbus_unmap_ring_vfree_pv(dev, vaddr);
+	else
+		return xenbus_unmap_ring_vfree_hvm(dev, vaddr);
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
 /**
  * xenbus_unmap_ring
-- 
1.7.7.4

Re: [PATCH 1/6] xenbus: Support HVM backends

[David Vrabel]

On 14/12/11 20:12, 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.

Perhaps rename the functions to xenbus_map_ring() and
xenbus_unmap_ring()?  I wouldn't respin the series just for this though.

Is there merit in having the pv and hvm variants, instead of just the
hvm variant?

David

Re: [PATCH 1/6] xenbus: Support HVM backends

[Daniel De Graaf]

On 12/15/2011 09:13 AM, David Vrabel wrote:
> On 14/12/11 20:12, 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.
> 
> Perhaps rename the functions to xenbus_map_ring() and
> xenbus_unmap_ring()?  I wouldn't respin the series just for this though.
> 
> Is there merit in having the pv and hvm variants, instead of just the
> hvm variant?
> 
> David
> 

There are already functions called xenbus_{map,unmap}_ring - they do the
actual mapping in the HVM case, with the _valloc versions just adding in
memory alloc/free.

The PV versions are slightly faster because they won't require additional
page table updates. Otherwise, I believe that the HVM variant should work
on both PV&HVM.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 1/6] xenbus: Support HVM backends

[Konrad Rzeszutek Wilk]

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 <dgdegra@tycho.nsa.gov>
> ---
>  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 <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/spinlock.h>
>  #include <linux/vmalloc.h>
>  #include <linux/export.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/page.h>
>  #include <xen/interface/xen.h>
>  #include <xen/interface/event_channel.h>
> +#include <xen/balloon.h>
>  #include <xen/events.h>
>  #include <xen/grant_table.h>
>  #include <xen/xenbus.h>
>  
> +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?

> +
>  const char *xenbus_strstate(enum xenbus_state state)
>  {
>  	static const char *const name[] = {
> @@ -420,21 +431,8 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)
>  EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
>  
>  
> -/**
> - * 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)
> +static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
> +                                     int gnt_ref, void **vaddr)
>  {
>  	struct gnttab_map_grant_ref op = {
>  		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
> @@ -469,6 +467,64 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
>  	*vaddr = area->addr;
>  	return 0;
>  }
> +
> +static int xenbus_map_ring_valloc_hvm(struct xenbus_device *dev,
> +                                     int gnt_ref, void **vaddr)
> +{
> +	struct xenbus_map_node *node;
> +	int err;
> +	void *addr;
> +
> +	*vaddr = NULL;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	err = alloc_xenballooned_pages(1, &node->page, false /* lowmem */);
> +	if (err)
> +		goto out_err;
> +
> +	addr = pfn_to_kaddr(page_to_pfn(node->page));
> +
> +	err = xenbus_map_ring(dev, gnt_ref, &node->handle, addr);
> +	if (err)
> +		goto out_err;
> +
> +	spin_lock(&xenbus_valloc_lock);
> +	list_add(&node->next, &xenbus_valloc_pages);
> +	spin_unlock(&xenbus_valloc_lock);
> +
> +	*vaddr = addr;
> +	return 0;
> +
> + out_err:
> +	free_xenballooned_pages(1, &node->page);
> +	kfree(node);
> +	return err;
> +}
> +
> +/**
> + * 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.

> +}
>  EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
>  
>  
> @@ -510,20 +566,7 @@ int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
>  }
>  EXPORT_SYMBOL_GPL(xenbus_map_ring);
>  
> -
> -/**
> - * xenbus_unmap_ring_vfree
> - * @dev: xenbus device
> - * @vaddr: addr to unmap
> - *
> - * Based on Rusty Russell's skeleton driver's unmap_page.
> - * Unmap a page of memory in this domain that was imported from another domain.
> - * Use xenbus_unmap_ring_vfree if you mapped in your memory with
> - * xenbus_map_ring_valloc (it will free the virtual address space).
> - * Returns 0 on success and returns GNTST_* on error
> - * (see xen/include/interface/grant_table.h).
> - */
> -int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
> +static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
>  {
>  	struct vm_struct *area;
>  	struct gnttab_unmap_grant_ref op = {
> @@ -566,8 +609,60 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
>  
>  	return op.status;
>  }
> -EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>  
> +static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
> +{
> +	int rv;
> +	struct xenbus_map_node *node;
> +	void *addr;
> +
> +	spin_lock(&xenbus_valloc_lock);
> +	list_for_each_entry(node, &xenbus_valloc_pages, next) {
> +		addr = pfn_to_kaddr(page_to_pfn(node->page));
> +		if (addr == vaddr) {
> +			list_del(&node->next);
> +			goto found;
> +		}
> +	}
> +	node = NULL;
> + found:
> +	spin_unlock(&xenbus_valloc_lock);
> +
> +	if (!node) {
> +		xenbus_dev_error(dev, -ENOENT,
> +				 "can't find mapped virtual address %p", vaddr);
> +		return -ENOENT;
> +	}
> +
> +	rv = xenbus_unmap_ring(dev, node->handle, addr);
> +
> +	if (!rv)
> +		free_xenballooned_pages(1, &node->page);

else
	WARN_ON("Leaking %p \n", vaddr);

> +
> +	kfree(node);
> +	return rv;
> +}
> +
> +/**
> + * xenbus_unmap_ring_vfree
> + * @dev: xenbus device
> + * @vaddr: addr to unmap
> + *
> + * Based on Rusty Russell's skeleton driver's unmap_page.
> + * Unmap a page of memory in this domain that was imported from another domain.
> + * Use xenbus_unmap_ring_vfree if you mapped in your memory with
> + * xenbus_map_ring_valloc (it will free the virtual address space).
> + * Returns 0 on success and returns GNTST_* on error

Well, that is not true anymore. You are also returning -ENOENT.

> + * (see xen/include/interface/grant_table.h).
> + */
> +int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
> +{
> +	if (xen_pv_domain())
> +		return xenbus_unmap_ring_vfree_pv(dev, vaddr);
> +	else
> +		return xenbus_unmap_ring_vfree_hvm(dev, vaddr);
> +}
> +EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>  
>  /**
>   * xenbus_unmap_ring
> -- 
> 1.7.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 1/6] xenbus: Support HVM backends

[Daniel De Graaf]

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<dgdegra@tycho.nsa.gov>
>> ---
>>   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<linux/slab.h>
>>   #include<linux/types.h>
>> +#include<linux/spinlock.h>
>>   #include<linux/vmalloc.h>
>>   #include<linux/export.h>
>>   #include<asm/xen/hypervisor.h>
>>   #include<asm/xen/page.h>
>>   #include<xen/interface/xen.h>
>>   #include<xen/interface/event_channel.h>
>> +#include<xen/balloon.h>
>>   #include<xen/events.h>
>>   #include<xen/grant_table.h>
>>   #include<xen/xenbus.h>
>>
>> +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.

[...]
>> +static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
>> +{
>> +	int rv;
>> +	struct xenbus_map_node *node;
>> +	void *addr;
>> +
>> +	spin_lock(&xenbus_valloc_lock);
>> +	list_for_each_entry(node,&xenbus_valloc_pages, next) {
>> +		addr = pfn_to_kaddr(page_to_pfn(node->page));
>> +		if (addr == vaddr) {
>> +			list_del(&node->next);
>> +			goto found;
>> +		}
>> +	}
>> +	node = NULL;
>> + found:
>> +	spin_unlock(&xenbus_valloc_lock);
>> +
>> +	if (!node) {
>> +		xenbus_dev_error(dev, -ENOENT,
>> +				 "can't find mapped virtual address %p", vaddr);
>> +		return -ENOENT;
>> +	}
>> +
>> +	rv = xenbus_unmap_ring(dev, node->handle, addr);
>> +
>> +	if (!rv)
>> +		free_xenballooned_pages(1,&node->page);
>
> else
> 	WARN_ON("Leaking %p \n", vaddr);

We do already get a warning from the xenbus_dev_error inside xenbus_unmap_ring
which is similar to the error path in the _pv version; added anyway since
triggering either of these indicates hypervisor/kernel state desynchronization.

>> +
>> +	kfree(node);
>> +	return rv;
>> +}
>> +
>> +/**
>> + * xenbus_unmap_ring_vfree
>> + * @dev: xenbus device
>> + * @vaddr: addr to unmap
>> + *
>> + * Based on Rusty Russell's skeleton driver's unmap_page.
>> + * Unmap a page of memory in this domain that was imported from another domain.
>> + * Use xenbus_unmap_ring_vfree if you mapped in your memory with
>> + * xenbus_map_ring_valloc (it will free the virtual address space).
>> + * Returns 0 on success and returns GNTST_* on error
>
> Well, that is not true anymore. You are also returning -ENOENT.

Will fix to return GNTST_bad_virt_addr like the PV version.

>> + * (see xen/include/interface/grant_table.h).
>> + */
>> +int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
>> +{
>> +	if (xen_pv_domain())
>> +		return xenbus_unmap_ring_vfree_pv(dev, vaddr);
>> +	else
>> +		return xenbus_unmap_ring_vfree_hvm(dev, vaddr);
>> +}
>> +EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>>
>>   /**
>>    * xenbus_unmap_ring
>> --
>> 1.7.7.4
>>

[PATCH 1/6 v2] xenbus: Support HVM backends

[Daniel De Graaf]

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. This also removes the need to use vmlist_lock
on PV by tracking the allocated xenbus rings.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/xenbus/xenbus_client.c |  209 +++++++++++++++++++++++++++---------
 1 files changed, 160 insertions(+), 49 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 1906125..367decf 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -32,16 +32,30 @@
 
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
+#include <xen/balloon.h>
 #include <xen/events.h>
 #include <xen/grant_table.h>
 #include <xen/xenbus.h>
 
+struct xenbus_map_node {
+	struct list_head next;
+	union {
+		struct vm_struct *area; /* PV */
+		struct page *page;     /* HVM */
+	};
+	grant_handle_t handle;
+};
+
+static DEFINE_SPINLOCK(xenbus_valloc_lock);
+static LIST_HEAD(xenbus_valloc_pages);
+
 const char *xenbus_strstate(enum xenbus_state state)
 {
 	static const char *const name[] = {
@@ -420,35 +434,29 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)
 EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
 
 
-/**
- * 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)
+static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
+				     int gnt_ref, void **vaddr)
 {
 	struct gnttab_map_grant_ref op = {
 		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
 		.ref   = gnt_ref,
 		.dom   = dev->otherend_id,
 	};
+	struct xenbus_map_node *node;
 	struct vm_struct *area;
 	pte_t *pte;
 
 	*vaddr = NULL;
 
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
 	area = alloc_vm_area(PAGE_SIZE, &pte);
-	if (!area)
+	if (!area) {
+		kfree(node);
 		return -ENOMEM;
+	}
 
 	op.host_addr = arbitrary_virt_to_machine(pte).maddr;
 
@@ -457,18 +465,81 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
 
 	if (op.status != GNTST_okay) {
 		free_vm_area(area);
+		kfree(node);
 		xenbus_dev_fatal(dev, op.status,
 				 "mapping in shared page %d from domain %d",
 				 gnt_ref, dev->otherend_id);
 		return op.status;
 	}
 
-	/* Stuff the handle in an unused field */
-	area->phys_addr = (unsigned long)op.handle;
+	node->handle = op.handle;
+	node->area = area;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
 
 	*vaddr = area->addr;
 	return 0;
 }
+
+static int xenbus_map_ring_valloc_hvm(struct xenbus_device *dev,
+				      int gnt_ref, void **vaddr)
+{
+	struct xenbus_map_node *node;
+	int err;
+	void *addr;
+
+	*vaddr = NULL;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	err = alloc_xenballooned_pages(1, &node->page, false /* lowmem */);
+	if (err)
+		goto out_err;
+
+	addr = pfn_to_kaddr(page_to_pfn(node->page));
+
+	err = xenbus_map_ring(dev, gnt_ref, &node->handle, addr);
+	if (err)
+		goto out_err;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
+
+	*vaddr = addr;
+	return 0;
+
+ out_err:
+	free_xenballooned_pages(1, &node->page);
+	kfree(node);
+	return err;
+}
+
+/**
+ * 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);
+}
 EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
 
 
@@ -510,47 +581,32 @@ int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
 }
 EXPORT_SYMBOL_GPL(xenbus_map_ring);
 
-
-/**
- * xenbus_unmap_ring_vfree
- * @dev: xenbus device
- * @vaddr: addr to unmap
- *
- * Based on Rusty Russell's skeleton driver's unmap_page.
- * Unmap a page of memory in this domain that was imported from another domain.
- * Use xenbus_unmap_ring_vfree if you mapped in your memory with
- * xenbus_map_ring_valloc (it will free the virtual address space).
- * Returns 0 on success and returns GNTST_* on error
- * (see xen/include/interface/grant_table.h).
- */
-int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
+static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
 {
-	struct vm_struct *area;
+	struct xenbus_map_node *node;
 	struct gnttab_unmap_grant_ref op = {
 		.host_addr = (unsigned long)vaddr,
 	};
 	unsigned int level;
 
-	/* It'd be nice if linux/vmalloc.h provided a find_vm_area(void *addr)
-	 * method so that we don't have to muck with vmalloc internals here.
-	 * We could force the user to hang on to their struct vm_struct from
-	 * xenbus_map_ring_valloc, but these 6 lines considerably simplify
-	 * this API.
-	 */
-	read_lock(&vmlist_lock);
-	for (area = vmlist; area != NULL; area = area->next) {
-		if (area->addr == vaddr)
-			break;
+	spin_lock(&xenbus_valloc_lock);
+	list_for_each_entry(node, &xenbus_valloc_pages, next) {
+		if (node->area->addr == vaddr) {
+			list_del(&node->next);
+			goto found;
+		}
 	}
-	read_unlock(&vmlist_lock);
+	node = NULL;
+ found:
+	spin_unlock(&xenbus_valloc_lock);
 
-	if (!area) {
+	if (!node) {
 		xenbus_dev_error(dev, -ENOENT,
 				 "can't find mapped virtual address %p", vaddr);
 		return GNTST_bad_virt_addr;
 	}
 
-	op.handle = (grant_handle_t)area->phys_addr;
+	op.handle = node->handle;
 	op.host_addr = arbitrary_virt_to_machine(
 		lookup_address((unsigned long)vaddr, &level)).maddr;
 
@@ -558,16 +614,71 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
 		BUG();
 
 	if (op.status == GNTST_okay)
-		free_vm_area(area);
+		free_vm_area(node->area);
 	else
 		xenbus_dev_error(dev, op.status,
 				 "unmapping page at handle %d error %d",
-				 (int16_t)area->phys_addr, op.status);
+				 node->handle, op.status);
 
+	kfree(node);
 	return op.status;
 }
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
+static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
+{
+	int rv;
+	struct xenbus_map_node *node;
+	void *addr;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_for_each_entry(node, &xenbus_valloc_pages, next) {
+		addr = pfn_to_kaddr(page_to_pfn(node->page));
+		if (addr == vaddr) {
+			list_del(&node->next);
+			goto found;
+		}
+	}
+	node = NULL;
+ found:
+	spin_unlock(&xenbus_valloc_lock);
+
+	if (!node) {
+		xenbus_dev_error(dev, -ENOENT,
+				 "can't find mapped virtual address %p", vaddr);
+		return GNTST_bad_virt_addr;
+	}
+
+	rv = xenbus_unmap_ring(dev, node->handle, addr);
+
+	if (!rv)
+		free_xenballooned_pages(1, &node->page);
+	else
+		WARN(1, "Leaking %p\n", vaddr);
+
+	kfree(node);
+	return rv;
+}
+
+/**
+ * xenbus_unmap_ring_vfree
+ * @dev: xenbus device
+ * @vaddr: addr to unmap
+ *
+ * Based on Rusty Russell's skeleton driver's unmap_page.
+ * Unmap a page of memory in this domain that was imported from another domain.
+ * Use xenbus_unmap_ring_vfree if you mapped in your memory with
+ * xenbus_map_ring_valloc (it will free the virtual address space).
+ * Returns 0 on success and returns GNTST_* on error
+ * (see xen/include/interface/grant_table.h).
+ */
+int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
+{
+	if (xen_pv_domain())
+		return xenbus_unmap_ring_vfree_pv(dev, vaddr);
+	else
+		return xenbus_unmap_ring_vfree_hvm(dev, vaddr);
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
 /**
  * xenbus_unmap_ring
-- 
1.7.7.4

Re: [PATCH 1/6] xenbus: Support HVM backends

[Konrad Rzeszutek Wilk]

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<dgdegra@tycho.nsa.gov>
> >>---
> >>  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<linux/slab.h>
> >>  #include<linux/types.h>
> >>+#include<linux/spinlock.h>
> >>  #include<linux/vmalloc.h>
> >>  #include<linux/export.h>
> >>  #include<asm/xen/hypervisor.h>
> >>  #include<asm/xen/page.h>
> >>  #include<xen/interface/xen.h>
> >>  #include<xen/interface/event_channel.h>
> >>+#include<xen/balloon.h>
> >>  #include<xen/events.h>
> >>  #include<xen/grant_table.h>
> >>  #include<xen/xenbus.h>
> >>
> >>+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?

Re: [PATCH 1/6] xenbus: Support HVM backends

[Daniel De Graaf]

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<dgdegra@tycho.nsa.gov>
>>>> ---
>>>>   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<linux/slab.h>
>>>>   #include<linux/types.h>
>>>> +#include<linux/spinlock.h>
>>>>   #include<linux/vmalloc.h>
>>>>   #include<linux/export.h>
>>>>   #include<asm/xen/hypervisor.h>
>>>>   #include<asm/xen/page.h>
>>>>   #include<xen/interface/xen.h>
>>>>   #include<xen/interface/event_channel.h>
>>>> +#include<xen/balloon.h>
>>>>   #include<xen/events.h>
>>>>   #include<xen/grant_table.h>
>>>>   #include<xen/xenbus.h>
>>>>
>>>> +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?

Re: [PATCH 1/6] xenbus: Support HVM backends

[Konrad Rzeszutek Wilk]

> >>>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?

Looks cleaner and fits the bill of where the rest of the Linux kernel is
going with using func structs for most everything.

[PATCH 1/6 v3] xenbus: Support HVM backends

[Daniel De Graaf]

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.  This also removes the need to use vmlist_lock
on PV by tracking the allocated xenbus rings.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/xenbus/xenbus_client.c |  175 +++++++++++++++++++++++++++++++-----
 drivers/xen/xenbus/xenbus_probe.c  |    2 +
 drivers/xen/xenbus/xenbus_probe.h  |    2 +
 3 files changed, 158 insertions(+), 21 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 1906125..0b3369d 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -32,16 +32,39 @@
 
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
+#include <xen/balloon.h>
 #include <xen/events.h>
 #include <xen/grant_table.h>
 #include <xen/xenbus.h>
 
+#include "xenbus_probe.h"
+
+struct xenbus_map_node {
+	struct list_head next;
+	union {
+		struct vm_struct *area; /* PV */
+		struct page *page;     /* HVM */
+	};
+	grant_handle_t handle;
+};
+
+static DEFINE_SPINLOCK(xenbus_valloc_lock);
+static LIST_HEAD(xenbus_valloc_pages);
+
+struct xenbus_ring_ops {
+	int (*map)(struct xenbus_device *dev, int gnt, void **vaddr);
+	int (*unmap)(struct xenbus_device *dev, void *vaddr);
+};
+
+static const struct xenbus_ring_ops *ring_ops __read_mostly;
+
 const char *xenbus_strstate(enum xenbus_state state)
 {
 	static const char *const name[] = {
@@ -436,19 +459,33 @@ EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
  */
 int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
 {
+	return ring_ops->map(dev, gnt_ref, vaddr);
+}
+EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
+
+static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
+				     int gnt_ref, void **vaddr)
+{
 	struct gnttab_map_grant_ref op = {
 		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
 		.ref   = gnt_ref,
 		.dom   = dev->otherend_id,
 	};
+	struct xenbus_map_node *node;
 	struct vm_struct *area;
 	pte_t *pte;
 
 	*vaddr = NULL;
 
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
 	area = alloc_vm_area(PAGE_SIZE, &pte);
-	if (!area)
+	if (!area) {
+		kfree(node);
 		return -ENOMEM;
+	}
 
 	op.host_addr = arbitrary_virt_to_machine(pte).maddr;
 
@@ -457,19 +494,59 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
 
 	if (op.status != GNTST_okay) {
 		free_vm_area(area);
+		kfree(node);
 		xenbus_dev_fatal(dev, op.status,
 				 "mapping in shared page %d from domain %d",
 				 gnt_ref, dev->otherend_id);
 		return op.status;
 	}
 
-	/* Stuff the handle in an unused field */
-	area->phys_addr = (unsigned long)op.handle;
+	node->handle = op.handle;
+	node->area = area;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
 
 	*vaddr = area->addr;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
+
+static int xenbus_map_ring_valloc_hvm(struct xenbus_device *dev,
+				      int gnt_ref, void **vaddr)
+{
+	struct xenbus_map_node *node;
+	int err;
+	void *addr;
+
+	*vaddr = NULL;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	err = alloc_xenballooned_pages(1, &node->page, false /* lowmem */);
+	if (err)
+		goto out_err;
+
+	addr = pfn_to_kaddr(page_to_pfn(node->page));
+
+	err = xenbus_map_ring(dev, gnt_ref, &node->handle, addr);
+	if (err)
+		goto out_err;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
+
+	*vaddr = addr;
+	return 0;
+
+ out_err:
+	free_xenballooned_pages(1, &node->page);
+	kfree(node);
+	return err;
+}
 
 
 /**
@@ -525,32 +602,36 @@ EXPORT_SYMBOL_GPL(xenbus_map_ring);
  */
 int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
 {
-	struct vm_struct *area;
+	return ring_ops->unmap(dev, vaddr);
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
+
+static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
+{
+	struct xenbus_map_node *node;
 	struct gnttab_unmap_grant_ref op = {
 		.host_addr = (unsigned long)vaddr,
 	};
 	unsigned int level;
 
-	/* It'd be nice if linux/vmalloc.h provided a find_vm_area(void *addr)
-	 * method so that we don't have to muck with vmalloc internals here.
-	 * We could force the user to hang on to their struct vm_struct from
-	 * xenbus_map_ring_valloc, but these 6 lines considerably simplify
-	 * this API.
-	 */
-	read_lock(&vmlist_lock);
-	for (area = vmlist; area != NULL; area = area->next) {
-		if (area->addr == vaddr)
-			break;
+	spin_lock(&xenbus_valloc_lock);
+	list_for_each_entry(node, &xenbus_valloc_pages, next) {
+		if (node->area->addr == vaddr) {
+			list_del(&node->next);
+			goto found;
+		}
 	}
-	read_unlock(&vmlist_lock);
+	node = NULL;
+ found:
+	spin_unlock(&xenbus_valloc_lock);
 
-	if (!area) {
+	if (!node) {
 		xenbus_dev_error(dev, -ENOENT,
 				 "can't find mapped virtual address %p", vaddr);
 		return GNTST_bad_virt_addr;
 	}
 
-	op.handle = (grant_handle_t)area->phys_addr;
+	op.handle = node->handle;
 	op.host_addr = arbitrary_virt_to_machine(
 		lookup_address((unsigned long)vaddr, &level)).maddr;
 
@@ -558,16 +639,50 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
 		BUG();
 
 	if (op.status == GNTST_okay)
-		free_vm_area(area);
+		free_vm_area(node->area);
 	else
 		xenbus_dev_error(dev, op.status,
 				 "unmapping page at handle %d error %d",
-				 (int16_t)area->phys_addr, op.status);
+				 node->handle, op.status);
 
+	kfree(node);
 	return op.status;
 }
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
+static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
+{
+	int rv;
+	struct xenbus_map_node *node;
+	void *addr;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_for_each_entry(node, &xenbus_valloc_pages, next) {
+		addr = pfn_to_kaddr(page_to_pfn(node->page));
+		if (addr == vaddr) {
+			list_del(&node->next);
+			goto found;
+		}
+	}
+	node = NULL;
+ found:
+	spin_unlock(&xenbus_valloc_lock);
+
+	if (!node) {
+		xenbus_dev_error(dev, -ENOENT,
+				 "can't find mapped virtual address %p", vaddr);
+		return GNTST_bad_virt_addr;
+	}
+
+	rv = xenbus_unmap_ring(dev, node->handle, addr);
+
+	if (!rv)
+		free_xenballooned_pages(1, &node->page);
+	else
+		WARN(1, "Leaking %p\n", vaddr);
+
+	kfree(node);
+	return rv;
+}
 
 /**
  * xenbus_unmap_ring
@@ -617,3 +732,21 @@ enum xenbus_state xenbus_read_driver_state(const char *path)
 	return result;
 }
 EXPORT_SYMBOL_GPL(xenbus_read_driver_state);
+
+static const struct xenbus_ring_ops ring_ops_pv = {
+	.map = xenbus_map_ring_valloc_pv,
+	.unmap = xenbus_unmap_ring_vfree_pv,
+};
+
+static const struct xenbus_ring_ops ring_ops_hvm = {
+	.map = xenbus_map_ring_valloc_hvm,
+	.unmap = xenbus_unmap_ring_vfree_hvm,
+};
+
+void __init xenbus_ring_ops_init(void)
+{
+	if (xen_pv_domain())
+		ring_ops = &ring_ops_pv;
+	else
+		ring_ops = &ring_ops_hvm;
+}
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 1b178c6..1c05b25 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -730,6 +730,8 @@ static int __init xenbus_init(void)
 	if (!xen_domain())
 		return -ENODEV;
 
+	xenbus_ring_ops_init();
+
 	if (xen_hvm_domain()) {
 		uint64_t v = 0;
 		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
diff --git a/drivers/xen/xenbus/xenbus_probe.h b/drivers/xen/xenbus/xenbus_probe.h
index 9b1de4e..460d784 100644
--- a/drivers/xen/xenbus/xenbus_probe.h
+++ b/drivers/xen/xenbus/xenbus_probe.h
@@ -76,4 +76,6 @@ extern void xenbus_otherend_changed(struct xenbus_watch *watch,
 extern int xenbus_read_otherend_details(struct xenbus_device *xendev,
 					char *id_node, char *path_node);
 
+void xenbus_ring_ops_init(void);
+
 #endif
-- 
1.7.7.4

[PATCH 2/6] xenbus: Use grant-table wrapper functions

[Daniel De Graaf]

For xenbus_{map,unmap}_ring to work on HVM, the grant table operations
must be set up using the gnttab_set_{map,unmap}_op functions instead of
directly populating the fields of gnttab_map_grant_ref. These functions
simply populate the structure on paravirtualized Xen; however, on HVM
they must call __pa() on vaddr when populating op->host_addr because the
hypervisor cannot directly interpret guest-virtual addresses.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/xenbus/xenbus_client.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index ecd695d..8a23f1a 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -545,12 +545,10 @@ EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
 int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
 		    grant_handle_t *handle, void *vaddr)
 {
-	struct gnttab_map_grant_ref op = {
-		.host_addr = (unsigned long)vaddr,
-		.flags     = GNTMAP_host_map,
-		.ref       = gnt_ref,
-		.dom       = dev->otherend_id,
-	};
+	struct gnttab_map_grant_ref op;
+
+	gnttab_set_map_op(&op, (phys_addr_t)vaddr, GNTMAP_host_map, gnt_ref,
+	                  dev->otherend_id);
 
 	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
 		BUG();
@@ -677,10 +675,9 @@ EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 int xenbus_unmap_ring(struct xenbus_device *dev,
 		      grant_handle_t handle, void *vaddr)
 {
-	struct gnttab_unmap_grant_ref op = {
-		.host_addr = (unsigned long)vaddr,
-		.handle    = handle,
-	};
+	struct gnttab_unmap_grant_ref op;
+
+	gnttab_set_unmap_op(&op, (phys_addr_t)vaddr, GNTMAP_host_map, handle);
 
 	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
 		BUG();
-- 
1.7.7.4

Re: [PATCH 2/6] xenbus: Use grant-table wrapper functions

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2011 at 03:12:10PM -0500, Daniel De Graaf wrote:
> For xenbus_{map,unmap}_ring to work on HVM, the grant table operations
> must be set up using the gnttab_set_{map,unmap}_op functions instead of
> directly populating the fields of gnttab_map_grant_ref. These functions
> simply populate the structure on paravirtualized Xen; however, on HVM
> they must call __pa() on vaddr when populating op->host_addr because the
> hypervisor cannot directly interpret guest-virtual addresses.

OK, this is a nice cleanup too.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/xenbus/xenbus_client.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ecd695d..8a23f1a 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -545,12 +545,10 @@ EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
>  int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
>  		    grant_handle_t *handle, void *vaddr)
>  {
> -	struct gnttab_map_grant_ref op = {
> -		.host_addr = (unsigned long)vaddr,
> -		.flags     = GNTMAP_host_map,
> -		.ref       = gnt_ref,
> -		.dom       = dev->otherend_id,
> -	};
> +	struct gnttab_map_grant_ref op;
> +
> +	gnttab_set_map_op(&op, (phys_addr_t)vaddr, GNTMAP_host_map, gnt_ref,
> +	                  dev->otherend_id);
>  
>  	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
>  		BUG();
> @@ -677,10 +675,9 @@ EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>  int xenbus_unmap_ring(struct xenbus_device *dev,
>  		      grant_handle_t handle, void *vaddr)
>  {
> -	struct gnttab_unmap_grant_ref op = {
> -		.host_addr = (unsigned long)vaddr,
> -		.handle    = handle,
> -	};
> +	struct gnttab_unmap_grant_ref op;
> +
> +	gnttab_set_unmap_op(&op, (phys_addr_t)vaddr, GNTMAP_host_map, handle);

>  
>  	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
>  		BUG();
> -- 
> 1.7.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 3/6] xen/grant-table: Support mappings required by blkback

[Daniel De Graaf]

Add support for mappings without GNTMAP_contains_pte. This was not
supported because the unmap operation assumed that this flag was being
used; adding a parameter to the unmap operation to allow the PTE
clearing to be disabled is sufficient to make unmap capable of
supporting either mapping type.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntdev.c      |    3 ++-
 drivers/xen/grant-table.c |   23 ++++-------------------
 include/xen/grant_table.h |    2 +-
 3 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index afca14d..65bff07 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -312,7 +312,8 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 		}
 	}
 
-	err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages + offset, pages);
+	err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages + offset,
+	                        pages, true);
 	if (err)
 		return err;
 
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index bf1c094..a02d139 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -472,24 +472,9 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 				(map_ops[i].host_addr & ~PAGE_MASK));
 			mfn = pte_mfn(*pte);
 		} else {
-			/* If you really wanted to do this:
-			 * mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
-			 *
-			 * The reason we do not implement it is b/c on the
-			 * unmap path (gnttab_unmap_refs) we have no means of
-			 * checking whether the page is !GNTMAP_contains_pte.
-			 *
-			 * That is without some extra data-structure to carry
-			 * the struct page, bool clear_pte, and list_head next
-			 * tuples and deal with allocation/delallocation, etc.
-			 *
-			 * The users of this API set the GNTMAP_contains_pte
-			 * flag so lets just return not supported until it
-			 * becomes neccessary to implement.
-			 */
-			return -EOPNOTSUPP;
+			mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
 		}
-		ret = m2p_add_override(mfn, pages[i], &kmap_ops[i]);
+		ret = m2p_add_override(mfn, pages[i], kmap_ops ? &kmap_ops[i] : NULL);
 		if (ret)
 			return ret;
 	}
@@ -499,7 +484,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 EXPORT_SYMBOL_GPL(gnttab_map_refs);
 
 int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
-		struct page **pages, unsigned int count)
+		struct page **pages, unsigned int count, bool clear_pte)
 {
 	int i, ret;
 
@@ -511,7 +496,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 		return ret;
 
 	for (i = 0; i < count; i++) {
-		ret = m2p_remove_override(pages[i], true /* clear the PTE */);
+		ret = m2p_remove_override(pages[i], clear_pte);
 		if (ret)
 			return ret;
 	}
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 11e2dfc..37da54d 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -158,6 +158,6 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 			struct gnttab_map_grant_ref *kmap_ops,
 		    struct page **pages, unsigned int count);
 int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
-		      struct page **pages, unsigned int count);
+		      struct page **pages, unsigned int count, bool clear_pte);
 
 #endif /* __ASM_GNTTAB_H__ */
-- 
1.7.7.4

[PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers

[Daniel De Graaf]

Now that the grant table hypercall wrappers support mappings without
GNTMAP_contains_pte, they should be used instead of invoking the
hypercalls manually.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/block/xen-blkback/blkback.c |   29 ++++-------------------------
 1 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 15ec4db..1e256dc 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -324,6 +324,7 @@ struct seg_buf {
 static void xen_blkbk_unmap(struct pending_req *req)
 {
 	struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+	struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	unsigned int i, invcount = 0;
 	grant_handle_t handle;
 	int ret;
@@ -335,25 +336,12 @@ static void xen_blkbk_unmap(struct pending_req *req)
 		gnttab_set_unmap_op(&unmap[invcount], vaddr(req, i),
 				    GNTMAP_host_map, handle);
 		pending_handle(req, i) = BLKBACK_INVALID_HANDLE;
+		pages[invcount] = virt_to_page(vaddr(req, i));
 		invcount++;
 	}
 
-	ret = HYPERVISOR_grant_table_op(
-		GNTTABOP_unmap_grant_ref, unmap, invcount);
+	ret = gnttab_unmap_refs(unmap, pages, invcount, false);
 	BUG_ON(ret);
-	/*
-	 * Note, we use invcount, so nr->pages, so we can't index
-	 * using vaddr(req, i).
-	 */
-	for (i = 0; i < invcount; i++) {
-		ret = m2p_remove_override(
-			virt_to_page(unmap[i].host_addr), false);
-		if (ret) {
-			pr_alert(DRV_PFX "Failed to remove M2P override for %lx\n",
-				 (unsigned long)unmap[i].host_addr);
-			continue;
-		}
-	}
 }
 
 static int xen_blkbk_map(struct blkif_request *req,
@@ -381,7 +369,7 @@ static int xen_blkbk_map(struct blkif_request *req,
 				  pending_req->blkif->domid);
 	}
 
-	ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map, nseg);
+	ret = gnttab_map_refs(map, NULL, &blkbk->pending_page(pending_req, 0), nseg);
 	BUG_ON(ret);
 
 	/*
@@ -401,15 +389,6 @@ static int xen_blkbk_map(struct blkif_request *req,
 		if (ret)
 			continue;
 
-		ret = m2p_add_override(PFN_DOWN(map[i].dev_bus_addr),
-			blkbk->pending_page(pending_req, i), NULL);
-		if (ret) {
-			pr_alert(DRV_PFX "Failed to install M2P override for %lx (ret: %d)\n",
-				 (unsigned long)map[i].dev_bus_addr, ret);
-			/* We could switch over to GNTTABOP_copy */
-			continue;
-		}
-
 		seg[i].buf  = map[i].dev_bus_addr |
 			(req->u.rw.seg[i].first_sect << 9);
 	}
-- 
1.7.7.4

Re: [PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2011 at 03:12:12PM -0500, Daniel De Graaf wrote:
> Now that the grant table hypercall wrappers support mappings without
> GNTMAP_contains_pte, they should be used instead of invoking the
> hypercalls manually.

Nice.. and it removes a bunch of duplicate code.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/block/xen-blkback/blkback.c |   29 ++++-------------------------
>  1 files changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 15ec4db..1e256dc 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -324,6 +324,7 @@ struct seg_buf {
>  static void xen_blkbk_unmap(struct pending_req *req)
>  {
>  	struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> +	struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>  	unsigned int i, invcount = 0;
>  	grant_handle_t handle;
>  	int ret;
> @@ -335,25 +336,12 @@ static void xen_blkbk_unmap(struct pending_req *req)
>  		gnttab_set_unmap_op(&unmap[invcount], vaddr(req, i),
>  				    GNTMAP_host_map, handle);
>  		pending_handle(req, i) = BLKBACK_INVALID_HANDLE;
> +		pages[invcount] = virt_to_page(vaddr(req, i));
>  		invcount++;
>  	}
>  
> -	ret = HYPERVISOR_grant_table_op(
> -		GNTTABOP_unmap_grant_ref, unmap, invcount);
> +	ret = gnttab_unmap_refs(unmap, pages, invcount, false);
>  	BUG_ON(ret);
> -	/*
> -	 * Note, we use invcount, so nr->pages, so we can't index
> -	 * using vaddr(req, i).
> -	 */
> -	for (i = 0; i < invcount; i++) {
> -		ret = m2p_remove_override(
> -			virt_to_page(unmap[i].host_addr), false);
> -		if (ret) {
> -			pr_alert(DRV_PFX "Failed to remove M2P override for %lx\n",
> -				 (unsigned long)unmap[i].host_addr);
> -			continue;
> -		}
> -	}
>  }
>  
>  static int xen_blkbk_map(struct blkif_request *req,
> @@ -381,7 +369,7 @@ static int xen_blkbk_map(struct blkif_request *req,
>  				  pending_req->blkif->domid);
>  	}
>  
> -	ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map, nseg);
> +	ret = gnttab_map_refs(map, NULL, &blkbk->pending_page(pending_req, 0), nseg);
>  	BUG_ON(ret);
>  
>  	/*
> @@ -401,15 +389,6 @@ static int xen_blkbk_map(struct blkif_request *req,
>  		if (ret)
>  			continue;
>  
> -		ret = m2p_add_override(PFN_DOWN(map[i].dev_bus_addr),
> -			blkbk->pending_page(pending_req, i), NULL);
> -		if (ret) {
> -			pr_alert(DRV_PFX "Failed to install M2P override for %lx (ret: %d)\n",
> -				 (unsigned long)map[i].dev_bus_addr, ret);
> -			/* We could switch over to GNTTABOP_copy */
> -			continue;
> -		}
> -
>  		seg[i].buf  = map[i].dev_bus_addr |
>  			(req->u.rw.seg[i].first_sect << 9);
>  	}
> -- 
> 1.7.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2011 at 03:12:12PM -0500, Daniel De Graaf wrote:
> Now that the grant table hypercall wrappers support mappings without
> GNTMAP_contains_pte, they should be used instead of invoking the
> hypercalls manually.

Hey Jens,

Same deal with this patch as with the other one I sent. Part of a
patchset, and this particular code moves the mucking around to the
existing library that does the mucking around.

It can be applied seperatly so I can also just move this patch
to my "stable/for-jens.3.3" patch series if that would make it easier
for you?

Thanks!
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/block/xen-blkback/blkback.c |   29 ++++-------------------------
>  1 files changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 15ec4db..1e256dc 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -324,6 +324,7 @@ struct seg_buf {
>  static void xen_blkbk_unmap(struct pending_req *req)
>  {
>  	struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> +	struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>  	unsigned int i, invcount = 0;
>  	grant_handle_t handle;
>  	int ret;
> @@ -335,25 +336,12 @@ static void xen_blkbk_unmap(struct pending_req *req)
>  		gnttab_set_unmap_op(&unmap[invcount], vaddr(req, i),
>  				    GNTMAP_host_map, handle);
>  		pending_handle(req, i) = BLKBACK_INVALID_HANDLE;
> +		pages[invcount] = virt_to_page(vaddr(req, i));
>  		invcount++;
>  	}
>  
> -	ret = HYPERVISOR_grant_table_op(
> -		GNTTABOP_unmap_grant_ref, unmap, invcount);
> +	ret = gnttab_unmap_refs(unmap, pages, invcount, false);
>  	BUG_ON(ret);
> -	/*
> -	 * Note, we use invcount, so nr->pages, so we can't index
> -	 * using vaddr(req, i).
> -	 */
> -	for (i = 0; i < invcount; i++) {
> -		ret = m2p_remove_override(
> -			virt_to_page(unmap[i].host_addr), false);
> -		if (ret) {
> -			pr_alert(DRV_PFX "Failed to remove M2P override for %lx\n",
> -				 (unsigned long)unmap[i].host_addr);
> -			continue;
> -		}
> -	}
>  }
>  
>  static int xen_blkbk_map(struct blkif_request *req,
> @@ -381,7 +369,7 @@ static int xen_blkbk_map(struct blkif_request *req,
>  				  pending_req->blkif->domid);
>  	}
>  
> -	ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map, nseg);
> +	ret = gnttab_map_refs(map, NULL, &blkbk->pending_page(pending_req, 0), nseg);
>  	BUG_ON(ret);
>  
>  	/*
> @@ -401,15 +389,6 @@ static int xen_blkbk_map(struct blkif_request *req,
>  		if (ret)
>  			continue;
>  
> -		ret = m2p_add_override(PFN_DOWN(map[i].dev_bus_addr),
> -			blkbk->pending_page(pending_req, i), NULL);
> -		if (ret) {
> -			pr_alert(DRV_PFX "Failed to install M2P override for %lx (ret: %d)\n",
> -				 (unsigned long)map[i].dev_bus_addr, ret);
> -			/* We could switch over to GNTTABOP_copy */
> -			continue;
> -		}
> -
>  		seg[i].buf  = map[i].dev_bus_addr |
>  			(req->u.rw.seg[i].first_sect << 9);
>  	}
> -- 
> 1.7.7.4

[PATCH 5/6] xen/netback: Enable netback on HVM guests

[Daniel De Graaf]

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/net/xen-netback/netback.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 0cb594c..951c713 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1638,7 +1638,7 @@ static int __init netback_init(void)
 	int rc = 0;
 	int group;
 
-	if (!xen_pv_domain())
+	if (!xen_domain())
 		return -ENODEV;
 
 	xen_netbk_group_nr = num_online_cpus();
-- 
1.7.7.4

Re: [PATCH 5/6] xen/netback: Enable netback on HVM guests

[Ian Campbell]

On Wed, 2011-12-14 at 20:12 +0000, Daniel De Graaf wrote:
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(which I've said before, please always retain Acks etc across postings
so people don't waste their time re-reviewing things.)

We previously (http://marc.info/?l=xen-devel&m=131944893321681&w=2)
agreed with Dave Miller that this could go via Konrad's tree which could
have been very usefully noted here also.

> ---
>  drivers/net/xen-netback/netback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 0cb594c..951c713 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1638,7 +1638,7 @@ static int __init netback_init(void)
>  	int rc = 0;
>  	int group;
>  
> -	if (!xen_pv_domain())
> +	if (!xen_domain())
>  		return -ENODEV;
>  
>  	xen_netbk_group_nr = num_online_cpus();

[PATCH 6/6] xen/blkback: Enable blkback on HVM guests

[Daniel De Graaf]

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/block/xen-blkback/blkback.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 1e256dc..fbffdf0 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -823,7 +823,7 @@ static int __init xen_blkif_init(void)
 	int i, mmap_pages;
 	int rc = 0;
 
-	if (!xen_pv_domain())
+	if (!xen_domain())
 		return -ENODEV;
 
 	blkbk = kzalloc(sizeof(struct xen_blkbk), GFP_KERNEL);
-- 
1.7.7.4

Re: [PATCH 6/6] xen/blkback: Enable blkback on HVM guests

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2011 at 03:12:14PM -0500, Daniel De Graaf wrote:
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Jens,

This patch is part of a series and while I can break this off to my
"stable/for-jens-3.3" branch (which I should send soonish), I was wondering
whether you would be OK just Ack-ing this so I can carry it/submit this
patch to Linus directly?

Thanks!
> ---
>  drivers/block/xen-blkback/blkback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 1e256dc..fbffdf0 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -823,7 +823,7 @@ static int __init xen_blkif_init(void)
>  	int i, mmap_pages;
>  	int rc = 0;
>  
> -	if (!xen_pv_domain())
> +	if (!xen_domain())
>  		return -ENODEV;
>  
>  	blkbk = kzalloc(sizeof(struct xen_blkbk), GFP_KERNEL);
> -- 
> 1.7.7.4

Re: [PATCH v3 0/6] xen/{net, blk}back support for running in HVM

[Konrad Rzeszutek Wilk]

> [PATCH 1/6] xenbus: Support HVM backends
> [PATCH 2/6] xenbus: Use grant-table wrapper functions
> [PATCH 3/6] xen/grant-table: Support mappings required by blkback
> [PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers
> [PATCH 5/6] xen/netback: Enable netback on HVM guests
> [PATCH 6/6] xen/blkback: Enable blkback on HVM guests

2-6 look good to me.

The #1 just needs adressing some of the things I pointed out.

Re: [PATCH v3 0/6] xen/{net, blk}back support for running in HVM

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2011 at 03:12:08PM -0500, Daniel De Graaf wrote:
> Changes from v2:
>  - Clarified comments and commit messages
>  - Based on v3.2-rc3
> 
> Changes from v1:
>  - Rebased on top of David's patches
>  - Grant table wrapper functions used where appropriate
> 
> [PATCH 1/6] xenbus: Support HVM backends
> [PATCH 2/6] xenbus: Use grant-table wrapper functions
> [PATCH 3/6] xen/grant-table: Support mappings required by blkback
> [PATCH 4/6] xen/blkback: use grant-table.c hypercall wrappers
> [PATCH 5/6] xen/netback: Enable netback on HVM guests
> [PATCH 6/6] xen/blkback: Enable blkback on HVM guests

Applied all of them in my #testing branch. Will move them next week
to #linux-next pending testing, Ack from Jens for #4 and #6.