Re: [PATCH V1 1/2] Xen acpi memory hotplug driver

From: Konrad Rzeszutek Wilk
Date: Wed Dec 05 2012 - 12:43:07 EST

On Fri, Nov 30, 2012 at 03:08:02AM +0000, Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 21, 2012 at 11:45:04AM +0000, Liu, Jinsong wrote:
> >>> From 630c65690c878255ce71e7c1172338ed08709273 Mon Sep 17 00:00:00
> >>> 2001
> >> From: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> >> Date: Tue, 20 Nov 2012 21:14:37 +0800
> >> Subject: [PATCH 1/2] Xen acpi memory hotplug driver
> >>
> >> Xen acpi memory hotplug consists of 2 logic components:
> >> Xen acpi memory hotplug driver and Xen hypercall.
> >>
> >> This patch implement Xen acpi memory hotplug driver. When running
> >> under xen platform, Xen driver will early occupy (so native driver
> >
> > How will it 'early occupy'? Can you spell it out here please?
>
> Sure, will add it like
> 'When running under xen platform, at booting stage xen memory hotplug driver will early occupy via subsys_initcall (earlier than native module_init), so xen driver will take effect and native driver will be blocked'.

OK.
>
> >
> >> will be blocked). When acpi memory notify OSPM, xen driver will take
> >> effect, adding related memory device and parsing memory information.
> >>
> >> Signed-off-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> >> ---
> >> drivers/xen/Kconfig | 11 +
> >> drivers/xen/Makefile | 1 +
> >> drivers/xen/xen-acpi-memhotplug.c | 383
> >> +++++++++++++++++++++++++++++++++++++ 3 files changed, 395
> >> insertions(+), 0 deletions(-) create mode 100644
> >> drivers/xen/xen-acpi-memhotplug.c
> >>
> >> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> >> index 126d8ce..abd0396 100644
> >> --- a/drivers/xen/Kconfig
> >> +++ b/drivers/xen/Kconfig
> >> @@ -206,4 +206,15 @@ config XEN_MCE_LOG
> >> Allow kernel fetching MCE error from Xen platform and
> >> converting it into Linux mcelog format for mcelog tools
> >>
> >> +config XEN_ACPI_MEMORY_HOTPLUG
> >> + bool "Xen ACPI memory hotplug"
> >
> > There should be a way to make this a module.
>
> I have some concerns to make it a module:
> 1. xen and native memhotplug driver both work as module, while we need early load xen driver.
> 2. if possible, a xen stub driver may solve load sequence issue, but it may involve other issues
> * if xen driver load then unload, native driver may have chance to load successfully;

The stub driver would still "occupy" the ACPI bus for the memory hotplug PnP, so
I think this would not be a problem.

> * if xen driver load --> unload --> load again, then it will lose hotplug notification during unload period;

Sure. But I think we can do it with this driver? After all the function of
it is to just tell the firmware to turn on/off sockets - and if we miss
one notification we won't take advantage of the power savings - but we
can do that later on.


> * if xen driver load --> unload --> load again, then it will re-add all memory devices, but the handle for 'booting memory device' and 'hotplug memory device' are different while we have no way to distinguish these 2 kind of devices.

Wouldn't the stub driver hold onto that?

>
> IMHO I think to make xen hotplug logic as module may involves unexpected result. Is there any obvious advantages of doing so? after all we have provided config choice to user. Thoughts?

Yes, it becomes a module - which is what we want.

>
> >
> >
> >> + depends on XEN_DOM0 && X86_64 && ACPI
> >> + default n
> >> + help
> >> + This is Xen acpi memory hotplug.
> > ^^^^ -> ACPI
> >
> >> +
> >> + Currently Xen only support acpi memory hot-add. If you want
> > ^^^^-> ACPI
> >
> >> + to hot-add memory at runtime (the hot-added memory cannot be
> >> + removed until machine stop), select Y here, otherwise select N. +
> >> endmenu
> >> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> >> index 7435470..c339eb4 100644
> >> --- a/drivers/xen/Makefile
> >> +++ b/drivers/xen/Makefile
> >> @@ -30,6 +30,7 @@ obj-$(CONFIG_XEN_MCE_LOG) += mcelog.o
> >> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/
> >> obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o
> >> obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o
> >> +obj-$(CONFIG_XEN_ACPI_MEMORY_HOTPLUG) += xen-acpi-memhotplug.o
> >> xen-evtchn-y := evtchn.o xen-gntdev-y := gntdev.o
> >> xen-gntalloc-y := gntalloc.o
> >> diff --git a/drivers/xen/xen-acpi-memhotplug.c
> >> b/drivers/xen/xen-acpi-memhotplug.c new file mode 100644 index
> >> 0000000..f0c7990 --- /dev/null
> >> +++ b/drivers/xen/xen-acpi-memhotplug.c
> >> @@ -0,0 +1,383 @@
> >> +/*
> >> + * Copyright (C) 2012 Intel Corporation
> >> + * Author: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> >> + * Author: Jiang Yunhong <yunhong.jiang@xxxxxxxxx> + *
> >> + * This program is free software; you can redistribute it and/or
> >> modify + * it under the terms of the GNU General Public License as
> >> published by + * the Free Software Foundation; either version 2 of
> >> the License, or (at + * your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> but + * WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE
> >> or + * NON INFRINGEMENT. See the GNU General Public License for
> >> more + * details. + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/init.h>
> >> +#include <linux/types.h>
> >> +#include <acpi/acpi_drivers.h>
> >> +
> >> +#define ACPI_MEMORY_DEVICE_CLASS "memory"
> >> +#define ACPI_MEMORY_DEVICE_HID "PNP0C80"
> >> +#define ACPI_MEMORY_DEVICE_NAME "Hotplug Mem Device"
> >
> > Weird tabs?
> >
>
> It ported from native and seems right tabs? will double check.
>
> >> +
> >> +#undef PREFIX
> >
> > Why the #undef ?
> >> +#define PREFIX "ACPI:memory_hp:"
> >
> >
> > Not "ACPI:memory_xen:" ?
>
> OK, how about more detailed "ACPI:xen_memory_hotplug:"?

Sure.
>
> >
> >
> >> +
> >> +static int acpi_memory_device_add(struct acpi_device *device);
> >> +static int acpi_memory_device_remove(struct acpi_device *device,
> >> int type); + +static const struct acpi_device_id memory_device_ids[]
> >> = { + {ACPI_MEMORY_DEVICE_HID, 0},
> >> + {"", 0},
> >> +};
> >> +
> >> +static struct acpi_driver acpi_memory_device_driver = {
> >> + .name = "acpi_memhotplug",
> >
> > Not 'xen_acpi_memhotplug' ?
>
> No, here driver name (same as native driver name) used to block native driver loading.

Then you need a comment in the file explaining that.

>
> >
> >> + .class = ACPI_MEMORY_DEVICE_CLASS,
> >> + .ids = memory_device_ids,
> >> + .ops = {
> >> + .add = acpi_memory_device_add,
> >> + .remove = acpi_memory_device_remove,
> >
> > Just for sake of clarity I would prefix those with 'xen_'.
>
> OK.
>
> >
> >> + },
> >> +};
> >> +
> >> +struct acpi_memory_info {
> >> + struct list_head list;
> >> + u64 start_addr; /* Memory Range start physical addr */
> >> + u64 length; /* Memory Range length */
> >> + unsigned short caching; /* memory cache attribute */
> >> + unsigned short write_protect; /* memory read/write attribute */
> >
> > Can't the write_protect by a bit field like the 'enabled'? So
> > unsigned int write_protect:1;
> > ?
>
> Seems not good, write_protect copied from an acpi buffer (byte3) getting from _CRS evaluation.

Ah, pls put a comment in there as well why that cannot be done.

>
> >> + unsigned int enabled:1;
> >> +};
> >> +
> >> +struct acpi_memory_device {
> >> + struct acpi_device *device;
> >> + struct list_head res_list;
> >> +};
> >> +
> >> +static int acpi_hotmem_initialized;
> >
> > Just make it a bool and also use __read_mostly please.
>
> OK.
>
> >
> >> +
> >> +
> >> +int xen_acpi_memory_enable_device(struct acpi_memory_device
> >> *mem_device) +{ + return 0;
> >> +}
> >
> > Why even have this function if it does not do anything?
>
> Not a nop, it implemented at patch 2/2.

Yup, saw it in the next patch.
>
> >
> >> +
> >> +static acpi_status
> >> +acpi_memory_get_resource(struct acpi_resource *resource, void
> >> *context) +{ + struct acpi_memory_device *mem_device = context;
> >> + struct acpi_resource_address64 address64;
> >> + struct acpi_memory_info *info, *new;
> >> + acpi_status status;
> >> +
> >> + status = acpi_resource_to_address64(resource, &address64); + if
> >> (ACPI_FAILURE(status) || + (address64.resource_type !=
> >> ACPI_MEMORY_RANGE)) + return AE_OK; +
> >> + list_for_each_entry(info, &mem_device->res_list, list) {
> >> + /* Can we combine the resource range information? */
> >
> > I don't know? Is this is a future TODO?
>
> I'm also not quite sure, this comments ported from native side.

OK, pls find out. Perhaps this comment is stale.

>
> >
> >> + if ((info->caching == address64.info.mem.caching) &&
> >> + (info->write_protect == address64.info.mem.write_protect) &&
> >> + (info->start_addr + info->length == address64.minimum)) {
> >> + info->length += address64.address_length;
> >> + return AE_OK;
> >> + }
> >> + }
> >> +
> >> + new = kzalloc(sizeof(struct acpi_memory_info), GFP_KERNEL); + if
> >> (!new) + return AE_ERROR;
> >> +
> >> + INIT_LIST_HEAD(&new->list);
> >> + new->caching = address64.info.mem.caching;
> >> + new->write_protect = address64.info.mem.write_protect;
> >> + new->start_addr = address64.minimum;
> >> + new->length = address64.address_length;
> >> + list_add_tail(&new->list, &mem_device->res_list); +
> >> + return AE_OK;
> >> +}
> >> +
> >> +static int
> >> +acpi_memory_get_device_resources(struct acpi_memory_device
> >> *mem_device) +{ + acpi_status status;
> >> + struct acpi_memory_info *info, *n;
> >> +
> >> + if (!list_empty(&mem_device->res_list))
> >> + return 0;
> >> +
> >> + status = acpi_walk_resources(mem_device->device->handle,
> >> + METHOD_NAME__CRS, acpi_memory_get_resource, mem_device); +
> >> + if (ACPI_FAILURE(status)) {
> >> + list_for_each_entry_safe(info, n, &mem_device->res_list, list)
> >> + kfree(info); + INIT_LIST_HEAD(&mem_device->res_list);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +acpi_memory_get_device(acpi_handle handle,
> >> + struct acpi_memory_device **mem_device)
> >> +{
> >> + acpi_status status;
> >> + acpi_handle phandle;
> >> + struct acpi_device *device = NULL;
> >> + struct acpi_device *pdevice = NULL;
> >> + int result;
> >> +
> >> + if (!acpi_bus_get_device(handle, &device) && device) + goto end;
> >> +
> >> + status = acpi_get_parent(handle, &phandle);
> >> + if (ACPI_FAILURE(status)) {
> >> + pr_warn(PREFIX "Cannot find acpi parent\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Get the parent device */
> >> + result = acpi_bus_get_device(phandle, &pdevice);
> >> + if (result) {
> >> + pr_warn(PREFIX "Cannot get acpi bus device\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /*
> >> + * Now add the notified device. This creates the acpi_device
> >> + * and invokes .add function
> >> + */
> >> + result = acpi_bus_add(&device, pdevice, handle,
> >> ACPI_BUS_TYPE_DEVICE); + if (result) { + pr_warn(PREFIX "Cannot add
> >> acpi bus\n"); + return -EINVAL;
> >> + }
> >> +
> >> +end:
> >> + *mem_device = acpi_driver_data(device);
> >> + if (!(*mem_device)) {
> >> + pr_err(PREFIX "Driver data not found\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int acpi_memory_check_device(struct acpi_memory_device
> >> *mem_device) +{ + unsigned long long current_status;
> >> +
> >> + /* Get device present/absent information from the _STA */
> >> + if (ACPI_FAILURE(acpi_evaluate_integer(mem_device->device->handle,
> >> + "_STA", NULL, &current_status)))
> >> + return -ENODEV;
> >> + /*
> >> + * Check for device status. Device should be
> >> + * present/enabled/functioning.
> >> + */
> >> + if (!((current_status & ACPI_STA_DEVICE_PRESENT)
> >> + && (current_status & ACPI_STA_DEVICE_ENABLED)
> >> + && (current_status & ACPI_STA_DEVICE_FUNCTIONING)))
> >> + return -ENODEV; +
> >> + return 0;
> >> +}
> >> +
> >> +static int acpi_memory_disable_device(struct acpi_memory_device
> >> *mem_device) +{ + pr_warn(PREFIX "Xen does not support memory
> >> hotremove\n"); + + return -ENOSYS;
> >> +}
> >> +
> >> +static void acpi_memory_device_notify(acpi_handle handle, u32
> >> event, void *data) +{ + struct acpi_memory_device *mem_device;
> >> + struct acpi_device *device;
> >> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */ +
> >> + switch (event) {
> >> + case ACPI_NOTIFY_BUS_CHECK:
> >> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >> + "\nReceived BUS CHECK notification for device\n")); + /* Fall
> >> Through */ + case ACPI_NOTIFY_DEVICE_CHECK:
> >> + if (event == ACPI_NOTIFY_DEVICE_CHECK)
> >> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >> + "\nReceived DEVICE CHECK notification for device\n")); +
> >> + if (acpi_memory_get_device(handle, &mem_device)) {
> >> + pr_err(PREFIX "Cannot find driver data\n");
> >> + break;
> >> + }
> >> +
> >> + ost_code = ACPI_OST_SC_SUCCESS;
> >> + break;
> >> +
> >> + case ACPI_NOTIFY_EJECT_REQUEST:
> >> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >> + "\nReceived EJECT REQUEST notification for device\n")); +
> >> + if (acpi_bus_get_device(handle, &device)) {
> >> + pr_err(PREFIX "Device doesn't exist\n");
> >> + break;
> >> + }
> >> + mem_device = acpi_driver_data(device);
> >> + if (!mem_device) {
> >> + pr_err(PREFIX "Driver Data is NULL\n");
> >> + break;
> >> + }
> >> +
> >> + /*
> >> + * TBD: implement acpi_memory_disable_device and invoke
> >> + * acpi_bus_remove if Xen support hotremove in the future + */
> >> + acpi_memory_disable_device(mem_device);
> >> + break;
> >> +
> >> + default:
> >> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >> + "Unsupported event [0x%x]\n", event));
> >> + /* non-hotplug event; possibly handled by other handler */
> >> + return; + }
> >> +
> >> + /* Inform firmware that the hotplug operation has completed */
> >> + (void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
> >
> >
> > Hm, even if we failed? Say for the ACPI_NOTIFY_EJECT_REQUEST ?
>
> OK, let's remove this the comments 'Inform firmware that the hotplug operation has completed'
> For ACPI_NOTIFY_EJECT_REQUEST, it in fact inform firmware 'ACPI_OST_SC_NON_SPECIFIC_FAILURE'.
>
> >
> >> + return;
> >> +}
> >> +
> >> +static int acpi_memory_device_add(struct acpi_device *device) +{
> >> + int result;
> >> + struct acpi_memory_device *mem_device = NULL;
> >> +
> >> +
> >> + if (!device)
> >> + return -EINVAL;
> >> +
> >> + mem_device = kzalloc(sizeof(struct acpi_memory_device),
> >> GFP_KERNEL); + if (!mem_device) + return -ENOMEM;
> >> +
> >> + INIT_LIST_HEAD(&mem_device->res_list);
> >> + mem_device->device = device;
> >> + sprintf(acpi_device_name(device), "%s", ACPI_MEMORY_DEVICE_NAME);
> >> + sprintf(acpi_device_class(device), "%s", ACPI_MEMORY_DEVICE_CLASS);
> >> + device->driver_data = mem_device;
> >> +
> >> + /* Get the range from the _CRS */
> >> + result = acpi_memory_get_device_resources(mem_device); + if
> >> (result) { + kfree(mem_device);
> >> + return result;
> >> + }
> >> +
> >> + /*
> >> + * Early boot code has recognized memory area by EFI/E820.
> >> + * If DSDT shows these memory devices on boot, hotplug is not
> >> necessary + * for them. So, it just returns until completion of
> >> this driver's + * start up. + */
> >> + if (!acpi_hotmem_initialized)
> >> + return 0;
> >> +
> >> + if (!acpi_memory_check_device(mem_device))
> >> + result = xen_acpi_memory_enable_device(mem_device);
> >
> > This is a nop. Could you just do:
> > result = 0;
> > ?
>
> It implemented at patch 2/2.
>
> Thanks,
> Jinsong
>
> >
> >> +
> >> + return result;
> >> +}
> >> +
> >> +static int acpi_memory_device_remove(struct acpi_device *device,
> >> int type) +{ + struct acpi_memory_device *mem_device = NULL;
> >> +
> >> + if (!device || !acpi_driver_data(device))
> >> + return -EINVAL;
> >> +
> >> + mem_device = acpi_driver_data(device);
> >> + kfree(mem_device);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/*
> >> + * Helper function to check for memory device
> >> + */
> >> +static acpi_status is_memory_device(acpi_handle handle) +{
> >> + char *hardware_id;
> >> + acpi_status status;
> >> + struct acpi_device_info *info;
> >> +
> >> + status = acpi_get_object_info(handle, &info);
> >> + if (ACPI_FAILURE(status))
> >> + return status;
> >> +
> >> + if (!(info->valid & ACPI_VALID_HID)) {
> >> + kfree(info);
> >> + return AE_ERROR;
> >> + }
> >> +
> >> + hardware_id = info->hardware_id.string;
> >> + if ((hardware_id == NULL) ||
> >> + (strcmp(hardware_id, ACPI_MEMORY_DEVICE_HID))) + status =
> >> AE_ERROR; +
> >> + kfree(info);
> >> + return status;
> >> +}
> >> +
> >> +static acpi_status
> >> +acpi_memory_register_notify_handler(acpi_handle handle,
> >> + u32 level, void *ctxt, void **retv)
> >> +{
> >> + acpi_status status;
> >> +
> >> + status = is_memory_device(handle);
> >> + if (ACPI_FAILURE(status))
> >> + return AE_OK; /* continue */
> >> +
> >> + status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> >> + acpi_memory_device_notify, NULL);
> >> + /* continue */
> >> + return AE_OK;
> >> +}
> >> +
> >> +static int __init xen_acpi_memory_device_init(void) +{
> >> + int result;
> >> + acpi_status status;
> >> +
> >> + /* only dom0 is responsible for xen acpi memory hotplug */ + if
> >> (!xen_initial_domain()) + return -ENODEV;
> >> +
> >> + result = acpi_bus_register_driver(&acpi_memory_device_driver);
> >> + if (result < 0) + return -ENODEV;
> >> +
> >> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> >> + ACPI_UINT32_MAX, +
> >> acpi_memory_register_notify_handler, NULL, + NULL, NULL); +
> >> + if (ACPI_FAILURE(status)) {
> >> + pr_warn(PREFIX "walk_namespace failed\n");
> >> + acpi_bus_unregister_driver(&acpi_memory_device_driver); + return
> >> -ENODEV; + }
> >> +
> >> + acpi_hotmem_initialized = 1;
> >> + return 0;
> >> +}
> >> +subsys_initcall(xen_acpi_memory_device_init);
> >> --
> >> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/