[PATCH 0/5] hv: drivers: Ensure that bridge windows don't overlap

[PATCH 0/5] hv: drivers: Ensure that bridge windows don't overlap

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

Hyper-V VMs expose paravirtual drivers through a mechanism called
VMBus, which is managed by hv_vmbus.ko.  For each paravirtual service
instance, this driver exposes a new child device.  Some of these child
devices need memory address space, into which Hyper-V will map things
like the virtual video framebuffer.  This memory-mapped address space
is chosen by the guest OS, not the hypervisor.

This is difficult to map onto the Linux pnp layer, as the code in the
pnp layer to choose MMIO space keys off of bus type and it doesn't know
anything about VMBus.  The maintainers of the pnp layer have asked that
we not offer patches to it that make it understand VMBus, but that we
rather find ways of using the code in its current state.  So hv_vmbus.ko
exports a function, vmbus_allocate_mmio() for choosing the address space
for any child driver that needs this facility.

The recently introduced PCI front-end driver for Hyper-V VMs
(pci-hyperv.ko) uses vmbus_allocate_mmio() for choosing both the region
of memory into which PCI configuration space can be mapped and the
region of memory into which real PCI Express devices which are passed
through to the VM should occupy.  The regions allocated are made to look
like root PCI bus bridge windows to the PCI driver, reusing all the code
in the PCI driver for the rest of the PCI device management.

The problem is that these bridge windows are marked in such a way that
devices can still allocate from the memory space spanned by them, and
this means that if two different PCI buses are created in the VM, each
with devices under them, they may allocate the same memory space, leading
to PCI Base Address Registers which overlap.

This patch series fixes the problem by tracking allocations to child
devices in a separate resource tree, marking them such that the bridge
windows can't overlap.  The main memory resource tree, iomem_resource,
contains resources properly marked as bridge windows, allowing their
children to overlap with them.

Jake Oshins (5):
  hv: Make a function to free mmio regions through vmbus
  hv: Lock access to hyperv_mmio resource tree
  hv: Use new vmbus_mmio_free() from client drivers.
  hv: Reverse order of resources in hyperv_mmio
  hv: Track allocations of children of hv_vmbus in private resource tree

 drivers/hv/vmbus_drv.c          | 56 +++++++++++++++++++++++++++++++++++------
 drivers/pci/host/pci-hyperv.c   | 14 +++++------
 drivers/video/fbdev/hyperv_fb.c |  4 +--
 include/linux/hyperv.h          |  2 +-
 4 files changed, 59 insertions(+), 17 deletions(-)

--
1.9.1

[PATCH 1/5] hv: Make a function to free mmio regions through vmbus

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

This patch introduces a function that reverses everything
done by vmbus_allocate_mmio().  Existing code just called
release_mem_region().  Future patches in this series
require a more complex sequence of actions, so this function
is introduced to wrap those actions.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 15 +++++++++++++++
 include/linux/hyperv.h |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 063e5f5..69393ff 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1221,6 +1221,21 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);
 
 /**
+ * vmbus_free_mmio() - Free a memory-mapped I/O range.
+ * @start:		Base address of region to release.
+ * @size:		Size of the range to be allocated
+ *
+ * This function releases anything requested by
+ * vmbus_mmio_allocate().
+ */
+void vmbus_free_mmio(resource_size_t start, resource_size_t size)
+{
+	release_mem_region(start, size);
+
+}
+EXPORT_SYMBOL_GPL(vmbus_free_mmio);
+
+/**
  * vmbus_cpu_number_to_vp_number() - Map CPU to VP.
  * @cpu_number: CPU number in Linux terms
  *
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a32704d..f3e0c71 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1091,7 +1091,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			resource_size_t min, resource_size_t max,
 			resource_size_t size, resource_size_t align,
 			bool fb_overlap_ok);
-
+void vmbus_free_mmio(resource_size_t start, resource_size_t size);
 int vmbus_cpu_number_to_vp_number(int cpu_number);
 u64 hv_do_hypercall(u64 control, void *input, void *output);
 
-- 
1.9.1

[PATCH 2/5] hv: Lock access to hyperv_mmio resource tree

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

In existing code, this tree of resources is created
in single-threaded code and never modified after it is
created, and thus needs no locking.  This patch introduces
a semaphore for tree access, as other patches in this
series introduce run-time modifications of this resource
tree which can happen on multiple threads.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 69393ff..1da18e1 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -103,6 +103,7 @@ static struct notifier_block hyperv_panic_block = {
 };
 
 struct resource *hyperv_mmio;
+DEFINE_SEMAPHORE(hyperv_mmio_lock);
 
 static int vmbus_exists(void)
 {
@@ -1173,7 +1174,10 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 	resource_size_t range_min, range_max, start, local_min, local_max;
 	const char *dev_n = dev_name(&device_obj->device);
 	u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size << 1);
-	int i;
+	int i, retval;
+
+	retval = -ENXIO;
+	down(&hyperv_mmio_lock);
 
 	for (iter = hyperv_mmio; iter; iter = iter->sibling) {
 		if ((iter->start >= max) || (iter->end <= min))
@@ -1210,13 +1214,17 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			for (; start + size - 1 <= local_max; start += align) {
 				*new = request_mem_region_exclusive(start, size,
 								    dev_n);
-				if (*new)
-					return 0;
+				if (*new) {
+					retval = 0;
+					goto exit;
+				}
 			}
 		}
 	}
 
-	return -ENXIO;
+exit:
+	up(&hyperv_mmio_lock);
+	return retval;
 }
 EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);
 
-- 
1.9.1

[PATCH 3/5] hv: Use new vmbus_mmio_free() from client drivers.

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

This patch modifies all the callers of vmbus_mmio_allocate()
to call vmbus_mmio_free() instead of release_mem_region().

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
 drivers/pci/host/pci-hyperv.c   | 14 +++++++-------
 drivers/video/fbdev/hyperv_fb.c |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 6f77d52..177bbec 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -2034,14 +2034,14 @@ static void hv_pci_free_bridge_windows(struct hv_pcibus_device *hbus)
 
 	if (hbus->low_mmio_space && hbus->low_mmio_res) {
 		hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
-		release_mem_region(hbus->low_mmio_res->start,
-				   resource_size(hbus->low_mmio_res));
+		vmbus_free_mmio(hbus->low_mmio_res->start,
+				resource_size(hbus->low_mmio_res));
 	}
 
 	if (hbus->high_mmio_space && hbus->high_mmio_res) {
 		hbus->high_mmio_res->flags |= IORESOURCE_BUSY;
-		release_mem_region(hbus->high_mmio_res->start,
-				   resource_size(hbus->high_mmio_res));
+		vmbus_free_mmio(hbus->high_mmio_res->start,
+				resource_size(hbus->high_mmio_res));
 	}
 }
 
@@ -2119,8 +2119,8 @@ static int hv_pci_allocate_bridge_windows(struct hv_pcibus_device *hbus)
 
 release_low_mmio:
 	if (hbus->low_mmio_res) {
-		release_mem_region(hbus->low_mmio_res->start,
-				   resource_size(hbus->low_mmio_res));
+		vmbus_free_mmio(hbus->low_mmio_res->start,
+				resource_size(hbus->low_mmio_res));
 	}
 
 	return ret;
@@ -2163,7 +2163,7 @@ static int hv_allocate_config_window(struct hv_pcibus_device *hbus)
 
 static void hv_free_config_window(struct hv_pcibus_device *hbus)
 {
-	release_mem_region(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
+	vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
 }
 
 /**
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index e2451bd..2fd49b2 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 err3:
 	iounmap(fb_virt);
 err2:
-	release_mem_region(par->mem->start, screen_fb_size);
+	vmbus_free_mmio(par->mem->start, screen_fb_size);
 	par->mem = NULL;
 err1:
 	if (!gen2vm)
@@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)
 	struct hvfb_par *par = info->par;
 
 	iounmap(info->screen_base);
-	release_mem_region(par->mem->start, screen_fb_size);
+	vmbus_free_mmio(par->mem->start, screen_fb_size);
 	par->mem = NULL;
 }
 
-- 
1.9.1

[PATCH 4/5] hv: Reverse order of resources in hyperv_mmio

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

A patch later in this series allocates child nodes
in this resource tree.  For that to work, this tree
needs to be sorted in ascending order.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 1da18e1..b090548 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1090,7 +1090,6 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 	new_res->end = end;
 
 	/*
-	 * Stick ranges from higher in address space at the front of the list.
 	 * If two ranges are adjacent, merge them.
 	 */
 	do {
@@ -1111,7 +1110,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 			break;
 		}
 
-		if ((*old_res)->end < new_res->start) {
+		if ((*old_res)->start > new_res->end) {
 			new_res->sibling = *old_res;
 			if (prev_res)
 				(*prev_res)->sibling = new_res;
-- 
1.9.1

[PATCH 5/5] hv: Track allocations of children of hv_vmbus in private resource tree

[jakeo]

From: Jake Oshins <jakeo@microsoft.com>

This patch changes vmbus_allocate_mmio() and vmbus_free_mmio() so
that when child paravirtual devices allocate memory-mapped I/O
space, they allocate it privately from a resource tree pointed
at by hyperv_mmio and also by the public resource tree
iomem_resource.  This allows the region to be marked as "busy"
in the private tree, but a "bridge window" in the public tree,
guaranteeing that no two bridge windows will overlap each other
but while also allowing the PCI device children of the bridge
windows to overlap that window.

One might conclude that this belongs in the pnp layer, rather
than in this driver.  Rafael Wysocki, the maintainter of the
pnp layer, has previously asked that we not modify the pnp layer
as it is considered deprecated.  This patch is thus essentially
a workaround.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index b090548..2a7eb3f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1169,7 +1169,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			resource_size_t size, resource_size_t align,
 			bool fb_overlap_ok)
 {
-	struct resource *iter;
+	struct resource *iter, *shadow;
 	resource_size_t range_min, range_max, start, local_min, local_max;
 	const char *dev_n = dev_name(&device_obj->device);
 	u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size << 1);
@@ -1211,12 +1211,22 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 
 			start = (local_min + align - 1) & ~(align - 1);
 			for (; start + size - 1 <= local_max; start += align) {
+				shadow = __request_region(iter, start,
+							  size,
+							  NULL,
+							  IORESOURCE_BUSY);
+				if (!shadow)
+					continue;
+
 				*new = request_mem_region_exclusive(start, size,
 								    dev_n);
 				if (*new) {
+					shadow->name = (char*)*new;
 					retval = 0;
 					goto exit;
 				}
+
+				__release_region(iter, start, size);
 			}
 		}
 	}
@@ -1237,7 +1247,17 @@ EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);
  */
 void vmbus_free_mmio(resource_size_t start, resource_size_t size)
 {
+	struct resource *iter;
+
+	down(&hyperv_mmio_lock);
+	for (iter = hyperv_mmio; iter; iter = iter->sibling) {
+		if ((iter->start >= start + size) || (iter->end <= start))
+			continue;
+
+		__release_region(iter, start, size);
+	}
 	release_mem_region(start, size);
+	up(&hyperv_mmio_lock);
 
 }
 EXPORT_SYMBOL_GPL(vmbus_free_mmio);
-- 
1.9.1

RE: [PATCH 5/5] hv: Track allocations of children of hv_vmbus in private resource tree

[KY Srinivasan]

> -----Original Message-----
> From: jakeo@microsoft.com [mailto:jakeo@microsoft.com]
> Sent: Wednesday, February 24, 2016 1:24 PM
> To: linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; KY Srinivasan
> <kys@microsoft.com>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; Haiyang Zhang <haiyangz@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>
> Cc: Jake Oshins <jakeo@microsoft.com>
> Subject: [PATCH 5/5] hv: Track allocations of children of hv_vmbus in private
> resource tree
> 
> From: Jake Oshins <jakeo@microsoft.com>
> 
> This patch changes vmbus_allocate_mmio() and vmbus_free_mmio() so
> that when child paravirtual devices allocate memory-mapped I/O
> space, they allocate it privately from a resource tree pointed
> at by hyperv_mmio and also by the public resource tree
> iomem_resource.  This allows the region to be marked as "busy"
> in the private tree, but a "bridge window" in the public tree,
> guaranteeing that no two bridge windows will overlap each other
> but while also allowing the PCI device children of the bridge
> windows to overlap that window.
> 
> One might conclude that this belongs in the pnp layer, rather
> than in this driver.  Rafael Wysocki, the maintainter of the
> pnp layer, has previously asked that we not modify the pnp layer
> as it is considered deprecated.  This patch is thus essentially
> a workaround.
> 
> Signed-off-by: Jake Oshins <jakeo@microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index b090548..2a7eb3f 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1169,7 +1169,7 @@ int vmbus_allocate_mmio(struct resource **new,
> struct hv_device *device_obj,
>  			resource_size_t size, resource_size_t align,
>  			bool fb_overlap_ok)
>  {
> -	struct resource *iter;
> +	struct resource *iter, *shadow;
>  	resource_size_t range_min, range_max, start, local_min, local_max;
>  	const char *dev_n = dev_name(&device_obj->device);
>  	u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size << 1);
> @@ -1211,12 +1211,22 @@ int vmbus_allocate_mmio(struct resource
> **new, struct hv_device *device_obj,
> 
>  			start = (local_min + align - 1) & ~(align - 1);
>  			for (; start + size - 1 <= local_max; start += align) {
> +				shadow = __request_region(iter, start,
> +							  size,
> +							  NULL,
> +							  IORESOURCE_BUSY);
> +				if (!shadow)
> +					continue;
> +
>  				*new =
> request_mem_region_exclusive(start, size,
>  								    dev_n);
>  				if (*new) {
> +					shadow->name = (char*)*new;

Why are you not correctly setting the name field in the shadow structure?

Regards,

K. Y

RE: [PATCH 5/5] hv: Track allocations of children of hv_vmbus in private resource tree

[Jake Oshins]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Friday, February 26, 2016 5:09 PM
> To: Jake Oshins <jakeo@microsoft.com>; linux-pci@vger.kernel.org;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; Haiyang Zhang <haiyangz@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>
> Cc: Jake Oshins <jakeo@microsoft.com>
> Subject: RE: [PATCH 5/5] hv: Track allocations of children of hv_vmbus in
> private resource tree
> 
> > -----Original Message-----
> > From: jakeo@microsoft.com [mailto:jakeo@microsoft.com]
> > Sent: Wednesday, February 24, 2016 1:24 PM
> > To: linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; KY Srinivasan
> > <kys@microsoft.com>; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > vkuznets@redhat.com; Haiyang Zhang <haiyangz@microsoft.com>;
> Hadden
> > Hoppert <haddenh@microsoft.com>
> > Cc: Jake Oshins <jakeo@microsoft.com>
> > Subject: [PATCH 5/5] hv: Track allocations of children of hv_vmbus in
> private
> > resource tree
> >
> > From: Jake Oshins <jakeo@microsoft.com>
> >
> > This patch changes vmbus_allocate_mmio() and vmbus_free_mmio() so
> > that when child paravirtual devices allocate memory-mapped I/O
> > space, they allocate it privately from a resource tree pointed
> > at by hyperv_mmio and also by the public resource tree
> > iomem_resource.  This allows the region to be marked as "busy"
> > in the private tree, but a "bridge window" in the public tree,
> > guaranteeing that no two bridge windows will overlap each other
> > but while also allowing the PCI device children of the bridge
> > windows to overlap that window.
> >
> > One might conclude that this belongs in the pnp layer, rather
> > than in this driver.  Rafael Wysocki, the maintainter of the
> > pnp layer, has previously asked that we not modify the pnp layer
> > as it is considered deprecated.  This patch is thus essentially
> > a workaround.
> >
> > Signed-off-by: Jake Oshins <jakeo@microsoft.com>
> > ---
> >  drivers/hv/vmbus_drv.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index b090548..2a7eb3f 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1169,7 +1169,7 @@ int vmbus_allocate_mmio(struct resource
> **new,
> > struct hv_device *device_obj,
> >  			resource_size_t size, resource_size_t align,
> >  			bool fb_overlap_ok)
> >  {
> > -	struct resource *iter;
> > +	struct resource *iter, *shadow;
> >  	resource_size_t range_min, range_max, start, local_min, local_max;
> >  	const char *dev_n = dev_name(&device_obj->device);
> >  	u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size << 1);
> > @@ -1211,12 +1211,22 @@ int vmbus_allocate_mmio(struct resource
> > **new, struct hv_device *device_obj,
> >
> >  			start = (local_min + align - 1) & ~(align - 1);
> >  			for (; start + size - 1 <= local_max; start += align) {
> > +				shadow = __request_region(iter, start,
> > +							  size,
> > +							  NULL,
> > +							  IORESOURCE_BUSY);
> > +				if (!shadow)
> > +					continue;
> > +
> >  				*new =
> > request_mem_region_exclusive(start, size,
> >  								    dev_n);
> >  				if (*new) {
> > +					shadow->name = (char*)*new;
> 
> Why are you not correctly setting the name field in the shadow structure?
> 
> Regards,
> 
> K. Y

Nothing looks at the name fields in the shadow resource tree.  So it seemed like that pointer could point to anything.  I figured by making it point to the resource claim from the iomem_resource that might be useful in debugging someday.  If you'd rather see something different here, it doesn't make much difference to me.

Thanks for the review,
Jake Oshins