* [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children
@ 2015-01-26 22:29 Jake Oshins
2015-02-06 15:04 ` Vitaly Kuznetsov
0 siblings, 1 reply; 4+ messages in thread
From: Jake Oshins @ 2015-01-26 22:29 UTC (permalink / raw)
To: gregkh, kys, linux-kernel, devel, olaf, apw, vkuznets; +Cc: Jake Oshins
This set of changes finds the _CRS object in the ACPI namespace
that contains memory address space descriptors, intended to convey
to VMBus which ranges of memory-mapped I/O space are available for
child devices, and then builds a resource list that contains all
those ranges. Without this change, only some of the memory-mapped
I/O space will be available for child devices, and only in some
virtual BIOS configurations (Generation 2 VMs).
This patch has been updated with feedback from Vitaly Kuznetsov.
Cleanup is now driven by the acpi remove callback function.
Signed-off-by: Jake Oshins <jakeo@microsoft.com>
---
drivers/hv/vmbus_drv.c | 99 +++++++++++++++++++++++++++++++++------
drivers/video/fbdev/hyperv_fb.c | 2 +-
include/linux/hyperv.h | 2 +-
3 files changed, 86 insertions(+), 17 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4d6b269..ed618ac 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -43,10 +43,7 @@ static struct tasklet_struct msg_dpc;
static struct completion probe_event;
static int irq;
-struct resource hyperv_mmio = {
- .name = "hyperv mmio",
- .flags = IORESOURCE_MEM,
-};
+struct resource *hyperv_mmio;
EXPORT_SYMBOL_GPL(hyperv_mmio);
static int vmbus_exists(void)
@@ -849,30 +846,98 @@ void vmbus_device_unregister(struct hv_device *device_obj)
/*
- * VMBUS is an acpi enumerated device. Get the the information we
- * need from DSDT.
+ * VMBUS is an acpi enumerated device. Get the
+ * information we need from DSDT.
*/
static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
{
+ resource_size_t start = 0;
+ resource_size_t end = 0;
+ struct resource *new_res;
+ struct resource **old_res = &hyperv_mmio;
+
switch (res->type) {
case ACPI_RESOURCE_TYPE_IRQ:
irq = res->data.irq.interrupts[0];
+ return AE_OK;
+
+ /*
+ * "Address" descriptors are for bus windows. Ignore
+ * "memory" descriptors, which are for registers on
+ * devices.
+ */
+ case ACPI_RESOURCE_TYPE_ADDRESS32:
+ start = res->data.address32.minimum;
+ end = res->data.address32.maximum;
break;
case ACPI_RESOURCE_TYPE_ADDRESS64:
- hyperv_mmio.start = res->data.address64.minimum;
- hyperv_mmio.end = res->data.address64.maximum;
+ start = res->data.address64.minimum;
+ end = res->data.address64.maximum;
break;
+
+ default:
+ /* Unused resource type */
+ return AE_OK;
}
+ /*
+ * Ignore ranges that are below 1MB, as they're not
+ * necessary or useful here.
+ */
+ if (end < 0x100000)
+ return AE_OK;
+
+ new_res = kzalloc(sizeof(*new_res), GFP_ATOMIC);
+ if (!new_res)
+ return AE_NO_MEMORY;
+
+ new_res->name = "hyperv mmio";
+ new_res->flags = IORESOURCE_MEM;
+ new_res->start = start;
+ new_res->end = end;
+
+ do {
+ if (!*old_res) {
+ *old_res = new_res;
+ break;
+ }
+
+ if ((*old_res)->start > new_res->end) {
+ new_res->sibling = *old_res;
+ *old_res = new_res;
+ break;
+ }
+
+ old_res = &(*old_res)->sibling;
+
+ } while (1);
+
return AE_OK;
}
+static int vmbus_acpi_remove(struct acpi_device *device)
+{
+ struct resource *cur_res;
+ struct resource *next_res;
+
+ if (hyperv_mmio) {
+ release_resource(hyperv_mmio);
+ for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
+ next_res = cur_res->sibling;
+ kfree(cur_res);
+ }
+ }
+
+ return 0;
+}
+
static int vmbus_acpi_add(struct acpi_device *device)
{
acpi_status result;
int ret_val = -ENODEV;
+ struct acpi_device *ancestor;
hv_acpi_dev = device;
@@ -882,23 +947,26 @@ static int vmbus_acpi_add(struct acpi_device *device)
if (ACPI_FAILURE(result))
goto acpi_walk_err;
/*
- * The parent of the vmbus acpi device (Gen2 firmware) is the VMOD that
- * has the mmio ranges. Get that.
+ * Some ancestor of the vmbus acpi device (Gen1 or Gen2
+ * firmware) is the VMOD that has the mmio ranges. Get that.
*/
- if (device->parent) {
- result = acpi_walk_resources(device->parent->handle,
+ for (ancestor = device->parent; ancestor; ancestor = ancestor->parent) {
+ result = acpi_walk_resources(ancestor->handle,
METHOD_NAME__CRS,
vmbus_walk_resources, NULL);
if (ACPI_FAILURE(result))
- goto acpi_walk_err;
- if (hyperv_mmio.start && hyperv_mmio.end)
- request_resource(&iomem_resource, &hyperv_mmio);
+ continue;
+ if (hyperv_mmio) {
+ request_resource(&iomem_resource, hyperv_mmio);
+ break;
+ }
}
ret_val = 0;
acpi_walk_err:
complete(&probe_event);
+ vmbus_acpi_remove(device);
return ret_val;
}
@@ -914,6 +982,7 @@ static struct acpi_driver vmbus_acpi_driver = {
.ids = vmbus_acpi_device_ids,
.ops = {
.add = vmbus_acpi_add,
+ .remove = vmbus_acpi_remove,
},
};
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4254336..003c8f0 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -686,7 +686,7 @@ static int hvfb_getmem(struct fb_info *info)
par->mem.name = KBUILD_MODNAME;
par->mem.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
if (gen2vm) {
- ret = allocate_resource(&hyperv_mmio, &par->mem,
+ ret = allocate_resource(hyperv_mmio, &par->mem,
screen_fb_size,
0, -1,
screen_fb_size,
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 476c685..8903689 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1178,7 +1178,7 @@ int hv_vss_init(struct hv_util_service *);
void hv_vss_deinit(void);
void hv_vss_onchannelcallback(void *);
-extern struct resource hyperv_mmio;
+extern struct resource *hyperv_mmio;
/*
* Negotiated version with the Host.
--
1.7.4.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children
2015-01-26 22:29 [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children Jake Oshins
@ 2015-02-06 15:04 ` Vitaly Kuznetsov
2015-02-06 16:57 ` Jake Oshins
0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2015-02-06 15:04 UTC (permalink / raw)
To: Jake Oshins; +Cc: gregkh, kys, linux-kernel, devel, olaf, apw
Jake Oshins <jakeo@microsoft.com> writes:
> This set of changes finds the _CRS object in the ACPI namespace
> that contains memory address space descriptors, intended to convey
> to VMBus which ranges of memory-mapped I/O space are available for
> child devices, and then builds a resource list that contains all
> those ranges. Without this change, only some of the memory-mapped
> I/O space will be available for child devices, and only in some
> virtual BIOS configurations (Generation 2 VMs).
>
> This patch has been updated with feedback from Vitaly Kuznetsov.
> Cleanup is now driven by the acpi remove callback function.
Sorry for beeing late with this message but I'm seeing issues with this
commit. I added some debug to figure out what's going on and here is
what I see:
With Gen1 VM we end up doing request_resource for two ranges:
f8000000 - fffbffff
fe0000000 - fffefffff
request_resource() fails (as we already have PCI device at f8000000 I
suppose?) but we don't check the return value. release_resource on
module unload crashes the kernel:
[ 78.314344] BUG: unable to handle kernel NULL pointer dereference at
0000000000000030
[ 78.315021] IP: [<ffffffff8107fac5>] release_resource+0x25/0x90
[ 78.315021] PGD 78c67067 PUD 78c5a067 PMD 0
[ 78.315021] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 78.315021] Modules linked in: hv_vmbus(-)
...
If I'm not mistaken, before the change we didn't do any
request_resource() for Gen1 VMs at all.
With Gen2 VM we do request_resource for fe0000000 - fffffffff range
only, that means this commit doesn't change anything.
Can you please take a look? I'd like to help but I don't completely
understand the essense of the change wrt Gen1 VMs with PCI devices.
Thanks,
>
> Signed-off-by: Jake Oshins <jakeo@microsoft.com>
> ---
> drivers/hv/vmbus_drv.c | 99 +++++++++++++++++++++++++++++++++------
> drivers/video/fbdev/hyperv_fb.c | 2 +-
> include/linux/hyperv.h | 2 +-
> 3 files changed, 86 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4d6b269..ed618ac 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -43,10 +43,7 @@ static struct tasklet_struct msg_dpc;
> static struct completion probe_event;
> static int irq;
>
> -struct resource hyperv_mmio = {
> - .name = "hyperv mmio",
> - .flags = IORESOURCE_MEM,
> -};
> +struct resource *hyperv_mmio;
> EXPORT_SYMBOL_GPL(hyperv_mmio);
>
> static int vmbus_exists(void)
> @@ -849,30 +846,98 @@ void vmbus_device_unregister(struct hv_device *device_obj)
>
> /*
> - * VMBUS is an acpi enumerated device. Get the the information we
> - * need from DSDT.
> + * VMBUS is an acpi enumerated device. Get the
> + * information we need from DSDT.
> */
>
> static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
> {
> + resource_size_t start = 0;
> + resource_size_t end = 0;
> + struct resource *new_res;
> + struct resource **old_res = &hyperv_mmio;
> +
> switch (res->type) {
> case ACPI_RESOURCE_TYPE_IRQ:
> irq = res->data.irq.interrupts[0];
> + return AE_OK;
> +
> + /*
> + * "Address" descriptors are for bus windows. Ignore
> + * "memory" descriptors, which are for registers on
> + * devices.
> + */
> + case ACPI_RESOURCE_TYPE_ADDRESS32:
> + start = res->data.address32.minimum;
> + end = res->data.address32.maximum;
> break;
>
> case ACPI_RESOURCE_TYPE_ADDRESS64:
> - hyperv_mmio.start = res->data.address64.minimum;
> - hyperv_mmio.end = res->data.address64.maximum;
> + start = res->data.address64.minimum;
> + end = res->data.address64.maximum;
> break;
> +
> + default:
> + /* Unused resource type */
> + return AE_OK;
> }
>
> + /*
> + * Ignore ranges that are below 1MB, as they're not
> + * necessary or useful here.
> + */
> + if (end < 0x100000)
> + return AE_OK;
> +
> + new_res = kzalloc(sizeof(*new_res), GFP_ATOMIC);
> + if (!new_res)
> + return AE_NO_MEMORY;
> +
> + new_res->name = "hyperv mmio";
> + new_res->flags = IORESOURCE_MEM;
> + new_res->start = start;
> + new_res->end = end;
> +
> + do {
> + if (!*old_res) {
> + *old_res = new_res;
> + break;
> + }
> +
> + if ((*old_res)->start > new_res->end) {
> + new_res->sibling = *old_res;
> + *old_res = new_res;
> + break;
> + }
> +
> + old_res = &(*old_res)->sibling;
> +
> + } while (1);
> +
> return AE_OK;
> }
>
> +static int vmbus_acpi_remove(struct acpi_device *device)
> +{
> + struct resource *cur_res;
> + struct resource *next_res;
> +
> + if (hyperv_mmio) {
> + release_resource(hyperv_mmio);
> + for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
> + next_res = cur_res->sibling;
> + kfree(cur_res);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int vmbus_acpi_add(struct acpi_device *device)
> {
> acpi_status result;
> int ret_val = -ENODEV;
> + struct acpi_device *ancestor;
>
> hv_acpi_dev = device;
>
> @@ -882,23 +947,26 @@ static int vmbus_acpi_add(struct acpi_device *device)
> if (ACPI_FAILURE(result))
> goto acpi_walk_err;
> /*
> - * The parent of the vmbus acpi device (Gen2 firmware) is the VMOD that
> - * has the mmio ranges. Get that.
> + * Some ancestor of the vmbus acpi device (Gen1 or Gen2
> + * firmware) is the VMOD that has the mmio ranges. Get that.
> */
> - if (device->parent) {
> - result = acpi_walk_resources(device->parent->handle,
> + for (ancestor = device->parent; ancestor; ancestor = ancestor->parent) {
> + result = acpi_walk_resources(ancestor->handle,
> METHOD_NAME__CRS,
> vmbus_walk_resources, NULL);
>
> if (ACPI_FAILURE(result))
> - goto acpi_walk_err;
> - if (hyperv_mmio.start && hyperv_mmio.end)
> - request_resource(&iomem_resource, &hyperv_mmio);
> + continue;
> + if (hyperv_mmio) {
> + request_resource(&iomem_resource, hyperv_mmio);
> + break;
> + }
> }
> ret_val = 0;
>
> acpi_walk_err:
> complete(&probe_event);
> + vmbus_acpi_remove(device);
> return ret_val;
> }
>
> @@ -914,6 +982,7 @@ static struct acpi_driver vmbus_acpi_driver = {
> .ids = vmbus_acpi_device_ids,
> .ops = {
> .add = vmbus_acpi_add,
> + .remove = vmbus_acpi_remove,
> },
> };
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 4254336..003c8f0 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -686,7 +686,7 @@ static int hvfb_getmem(struct fb_info *info)
> par->mem.name = KBUILD_MODNAME;
> par->mem.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> if (gen2vm) {
> - ret = allocate_resource(&hyperv_mmio, &par->mem,
> + ret = allocate_resource(hyperv_mmio, &par->mem,
> screen_fb_size,
> 0, -1,
> screen_fb_size,
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 476c685..8903689 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1178,7 +1178,7 @@ int hv_vss_init(struct hv_util_service *);
> void hv_vss_deinit(void);
> void hv_vss_onchannelcallback(void *);
>
> -extern struct resource hyperv_mmio;
> +extern struct resource *hyperv_mmio;
>
> /*
> * Negotiated version with the Host.
--
Vitaly
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children
2015-02-06 15:04 ` Vitaly Kuznetsov
@ 2015-02-06 16:57 ` Jake Oshins
2015-02-07 4:57 ` Jake Oshins
0 siblings, 1 reply; 4+ messages in thread
From: Jake Oshins @ 2015-02-06 16:57 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: gregkh, KY Srinivasan, linux-kernel, devel, olaf, apw
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Friday, February 6, 2015 7:04 AM
> To: Jake Oshins
> Cc: gregkh@linuxfoundation.org; KY Srinivasan; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com
> Subject: Re: [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more
> than one MMIO range for children
>
> Jake Oshins <jakeo@microsoft.com> writes:
>
> > This set of changes finds the _CRS object in the ACPI namespace
> > that contains memory address space descriptors, intended to convey
> > to VMBus which ranges of memory-mapped I/O space are available for
> > child devices, and then builds a resource list that contains all
> > those ranges. Without this change, only some of the memory-mapped
> > I/O space will be available for child devices, and only in some
> > virtual BIOS configurations (Generation 2 VMs).
> >
> > This patch has been updated with feedback from Vitaly Kuznetsov.
> > Cleanup is now driven by the acpi remove callback function.
>
> Sorry for beeing late with this message but I'm seeing issues with this
> commit. I added some debug to figure out what's going on and here is
> what I see:
>
> With Gen1 VM we end up doing request_resource for two ranges:
> f8000000 - fffbffff
> fe0000000 - fffefffff
>
> request_resource() fails (as we already have PCI device at f8000000 I
> suppose?) but we don't check the return value. release_resource on
> module unload crashes the kernel:
> [ 78.314344] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000030
> [ 78.315021] IP: [<ffffffff8107fac5>] release_resource+0x25/0x90
> [ 78.315021] PGD 78c67067 PUD 78c5a067 PMD 0
> [ 78.315021] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
> [ 78.315021] Modules linked in: hv_vmbus(-)
> ...
> If I'm not mistaken, before the change we didn't do any
> request_resource() for Gen1 VMs at all.
>
> With Gen2 VM we do request_resource for fe0000000 - fffffffff range
> only, that means this commit doesn't change anything.
>
> Can you please take a look? I'd like to help but I don't completely
> understand the essense of the change wrt Gen1 VMs with PCI devices.
>
I'll take a look immediately.
Thanks,
Jake
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children
2015-02-06 16:57 ` Jake Oshins
@ 2015-02-07 4:57 ` Jake Oshins
0 siblings, 0 replies; 4+ messages in thread
From: Jake Oshins @ 2015-02-07 4:57 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: gregkh, KY Srinivasan, linux-kernel, devel, olaf, apw
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Friday, February 6, 2015 7:04 AM
> > To: Jake Oshins
> > Cc: gregkh@linuxfoundation.org; KY Srinivasan; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com
> > Subject: Re: [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more
> > than one MMIO range for children
<snip>
> >
> > Sorry for beeing late with this message but I'm seeing issues with this
> > commit. I added some debug to figure out what's going on and here is
> > what I see:
> >
> > With Gen1 VM we end up doing request_resource for two ranges:
> > f8000000 - fffbffff
> > fe0000000 - fffefffff
> >
> > request_resource() fails (as we already have PCI device at f8000000 I
> > suppose?) but we don't check the return value. release_resource on
> > module unload crashes the kernel:
> > [ 78.314344] BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000030
> > [ 78.315021] IP: [<ffffffff8107fac5>] release_resource+0x25/0x90
> > [ 78.315021] PGD 78c67067 PUD 78c5a067 PMD 0
> > [ 78.315021] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
> > [ 78.315021] Modules linked in: hv_vmbus(-)
> > ...
> > If I'm not mistaken, before the change we didn't do any
> > request_resource() for Gen1 VMs at all.
> >
> > With Gen2 VM we do request_resource for fe0000000 - fffffffff range
> > only, that means this commit doesn't change anything.
> >
> > Can you please take a look? I'd like to help but I don't completely
> > understand the essense of the change wrt Gen1 VMs with PCI devices.
> >
>
> I'll take a look immediately.
>
> Thanks,
> Jake
I looked at this and I realize that I need to fix this problem structurally. The intent is that VMBus-enumerated paravirtual "devices" suballocate from everything granted to the ACPI node above VMBus, in its _CRS object. But since PCI devices also have to suballocate from those ranges, I need a different approach. I'll work up a fix for this and send it around for review.
-- Jake Oshins
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-02-07 5:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-26 22:29 [PATCH v2 1/1] drivers:hv:vmbus drivers:hv:vmbus Allow for more than one MMIO range for children Jake Oshins
2015-02-06 15:04 ` Vitaly Kuznetsov
2015-02-06 16:57 ` Jake Oshins
2015-02-07 4:57 ` Jake Oshins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).