Date:	Fri, 06 Feb 2015 16:04:03 +0100
From:	Vitaly Kuznetsov <vkuznets@...hat.com>
To:	Jake Oshins <jakeo@...rosoft.com>
Cc:	gregkh@...uxfoundation.org, kys@...rosoft.com,
	linux-kernel@...r.kernel.org, devel@...uxdriverproject.org,
	olaf@...fle.de, apw@...onical.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@...rosoft.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@...rosoft.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists