RE: [Xen-devel] [PATCH] Add memory hotadd to pvops dom0

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Konrad, thanks for your review. See below please.

>-----Original Message-----
>From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
>Sent: Friday, December 18, 2009 10:20 PM
>To: Jiang, Yunhong
>Cc: Jeremy Fitzhardinge; xen-devel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [Xen-devel] [PATCH] Add memory hotadd to pvops dom0
>
>On Fri, Dec 18, 2009 at 06:27:06PM +0800, Jiang, Yunhong wrote:
>> When memory hotadd event happen, a Xen hook will be called, to notify 
>> hypervisor
>of the new added memory.
>>
>> Because xen hypervisor will use the new memory to setup frametable/m2p table,
>so dom0 will always return success to acpi bios, and notify xen hypervisor 
>later.
>>
>> It add a hook in driver/acpi/acpi_memhotplug.c, but that change is quite 
>> small, not
>sure if it is acceptable. Other method is to provide a xen specific
>acpi_memory_device_driver, but I'm not sure if it worth to add so much 
>changes, to
>simply avoid two line hooks.
>>
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@xxxxxxxxx>
>> ---
>>  drivers/acpi/acpi_memhotplug.c    |    5 +
>>  drivers/xen/Makefile              |    1 +
>>  drivers/xen/xen_acpi_memhotplug.c |  197
>+++++++++++++++++++++++++++++++++++++
>>  include/xen/acpi.h                |    4 +
>>  include/xen/interface/platform.h  |   10 ++
>>  5 files changed, 217 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/xen/xen_acpi_memhotplug.c
>>
>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>> index 50e17c7..75e6b72 100644
>> --- a/drivers/acpi/acpi_memhotplug.c
>> +++ b/drivers/acpi/acpi_memhotplug.c
>> @@ -31,6 +31,8 @@
>>  #include <linux/types.h>
>>  #include <linux/memory_hotplug.h>
>>  #include <acpi/acpi_drivers.h>
>> +#include <acpi/acpi_numa.h>
>> +#include <xen/acpi.h>
>>
>>  #define ACPI_MEMORY_DEVICE_CLASS            "memory"
>>  #define ACPI_MEMORY_DEVICE_HID                      "PNP0C80"
>> @@ -215,6 +217,9 @@ static int acpi_memory_enable_device(struct
>acpi_memory_device *mem_device)
>>              return result;
>>      }
>>
>> +    if (xen_initial_domain())
>> +            return xen_add_memory(mem_device);
>> +
>>      node = acpi_get_node(mem_device->device->handle);
>>      /*
>>       * Tell the VM there is more memory here...
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 6325be1..fbacea7 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -19,6 +19,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)   += sys-hypervisor.o
>>  obj-$(CONFIG_XEN_S3)                        += acpi.o
>>  obj-$(CONFIG_XEN_MCE)                       += mce.o
>>  obj-$(CONFIG_ACPI_PROCESSOR_XEN)    += acpi_processor.o
>> +obj-$(CONFIG_ACPI_HOTPLUG_MEMORY)   += xen_acpi_memhotplug.o
>>
>>  xen-gntdev-y                                := gntdev.o
>>  xen-evtchn-y                                := evtchn.o
>> diff --git a/drivers/xen/xen_acpi_memhotplug.c
>b/drivers/xen/xen_acpi_memhotplug.c
>> new file mode 100644
>> index 0000000..17b936e
>> --- /dev/null
>> +++ b/drivers/xen/xen_acpi_memhotplug.c
>> @@ -0,0 +1,197 @@
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/types.h>
>> +#include <linux/memory_hotplug.h>
>> +#include <acpi/acpi_drivers.h>
>> +#include <xen/interface/platform.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/spinlock.h>
>> +#include <asm/xen/hypervisor.h>
>> +#include <asm/xen/hypercall.h>
>> +#include <xen/acpi.h>
>> +
>> +struct xen_hotmem_entry {
>> +    struct list_head hotmem_list;
>> +    uint64_t start;
>> +    uint64_t end;
>> +    uint32_t flags;
>> +    uint32_t pxm;
>> +};
>> +
>> +struct xen_hotmem_list {
>> +    struct list_head list;
>> +    int entry_nr;
>> +} xen_hotmem;
>> +
>> +DEFINE_SPINLOCK(xen_hotmem_lock);
>> +
>> +static int xen_hyper_addmem(struct xen_hotmem_entry *entry)
>> +{
>> +    int ret;
>> +
>> +    xen_platform_op_t op = {
>> +            .cmd            = XENPF_mem_hotadd,
>> +            .interface_version  = XENPF_INTERFACE_VERSION,
>> +    };
>> +    op.u.mem_add.spfn = entry->start >> PAGE_SHIFT;
>> +    op.u.mem_add.epfn = entry->end >> PAGE_SHIFT;
>> +    op.u.mem_add.flags = entry->flags;
>> +    op.u.mem_add.pxm = entry->pxm;
>> +
>> +    ret = HYPERVISOR_dom0_op(&op);
>> +    return ret;
>> +}
>> +static int xen_hotmem_free(struct xen_hotmem_entry *entry)
>> +{
>> +    list_del(&entry->hotmem_list);
>> +    kfree(entry);
>> +
>> +    return 0;
>> +}
>> +
>> +static void xen_hotmem_dpc(struct work_struct *work)
>> +{
>> +    struct list_head *elem, *tmp;
>> +    struct xen_hotmem_entry *entry;
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    spin_lock_irqsave(&xen_hotmem_lock, flags);
>> +    list_for_each_safe(elem, tmp, &xen_hotmem.list) {
>> +            entry = list_entry(elem, struct xen_hotmem_entry, hotmem_list);
>> +            ret = xen_hyper_addmem(entry);
>> +            if (ret)
>> +                    printk(KERN_WARNING "xen addmem failed with %x\n", ret);
>> +            xen_hotmem_free(entry);
>> +    }
>> +    spin_unlock_irqrestore(&xen_hotmem_lock, flags);
>> +}
>> +
>> +static DECLARE_WORK(xen_hotmem_work, xen_hotmem_dpc);
>> +
>> +static int xen_acpi_get_pxm(acpi_handle h)
>> +{
>> +    unsigned long long pxm;
>> +    acpi_status status;
>> +    acpi_handle handle;
>> +    acpi_handle phandle = h;
>> +
>> +    do {
>> +            handle = phandle;
>> +            status = acpi_evaluate_integer(handle, "_PXM", NULL, &pxm);
>> +            if (ACPI_SUCCESS(status))
>> +                    return pxm;
>> +            status = acpi_get_parent(handle, &phandle);
>> +    } while (ACPI_SUCCESS(status));
>> +    return -1;
>
>Better error return? -ENODEV?
>> +}
>> +
>> +static int add_memory_entry(int pxm, uint64_t start,
>> +                    uint64_t length, uint32_t flags)
>> +{
>> +    struct xen_hotmem_entry *entry;
>> +
>> +    if (pxm < 0 || !length)
>> +            return -1;
>
>-ENODEV?

Yes, will fix that, and for other error return commented below.

>
>
>> +
>> +    entry = kzalloc(sizeof(struct xen_hotmem_entry), GFP_ATOMIC);
>> +    if (!entry)
>> +            return -1;
>
>-ENOMEM?
>> +
>> +    INIT_LIST_HEAD(&entry->hotmem_list);
>> +    entry->start = start;
>> +    entry->end = start + length;
>> +    entry->flags = flags;
>> +    entry->pxm = pxm;
>> +
>> +    spin_lock(&xen_hotmem_lock);
>> +
>> +    list_add_tail(&entry->hotmem_list, &xen_hotmem.list);
>> +
>> +    spin_unlock(&xen_hotmem_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +int xen_add_memory(struct acpi_memory_device *mem_device)
>
>How about calling it 'xen_hotadd_memory' to keep in with the
>rest of the nomenclature?

Ok.

>
>> +{
>> +    int pxm, result;
>> +    int num_enabled = 0;
>> +    struct acpi_memory_info *info;
>> +
>> +    if (!mem_device)
>> +            return -EINVAL;
>> +
>> +    pxm = xen_acpi_get_pxm(mem_device->device->handle);
>> +
>> +    if (pxm < 0)
>> +            return -EINVAL;
>> +
>> +    /*
>> +     * Always return success to ACPI driver, and notify hypervisor later
>> +     * because hypervisor will utilize the memory in memory hotadd hypercal
>> +     */
>> +    list_for_each_entry(info, &mem_device->res_list, list) {
>
>Who deallocates this list?

It will be handled by acpi driver. Generally it is freed when the 
acpi_memory_device deallocated. 
Since acpi_memory_enable_device() didn't release it, I don't think I need care 
about it.

>
>> +            if (info->enabled) { /* just sanity check...*/
>> +                    num_enabled++;
>> +                    continue;
>> +            }
>> +            /*
>> +             * If the memory block size is zero, please ignore it.
>> +             * Don't try to do the following memory hotplug flowchart.
>> +             */
>> +            if (!info->length)
>> +                    continue;
>> +
>> +            result = add_memory_entry(pxm, info->start_addr,
>> +                                    info->length, 0);
>> +            if (result)
>> +                    continue;
>> +            info->enabled = 1;
>> +            num_enabled++;
>> +    }
>> +
>> +    if (!num_enabled)
>> +            return -EINVAL;
>> +
>> +    schedule_work(&xen_hotmem_work);
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(xen_add_memory);
>> +
>> +static int xen_memadd_init(void)
>> +{
>> +    if (!xen_initial_domain()) {
>> +            printk(KERN_ERR "xen_memadd is only for Xen dom0\n");
>
>Get rid of it. The kernel that can be used for DomU and Dom0 is the same.
>So this module could be very well compiled in the kernel -  and you
>would get this loaded under DomU and there is no need for this message.

Yes, thanks for remind.

>> +            return -1;
>
>-ENODEV
>
>> +    }
>> +
>> +    INIT_LIST_HEAD(&xen_hotmem.list);
>> +    xen_hotmem.entry_nr = 0;
>> +
>> +    return 0;
>> +}
>> +
>> +static void xen_memadd_exit(void)
>> +{
>> +    unsigned long flags;
>> +    struct list_head *elem, *tmp;
>> +    struct xen_hotmem_entry *entry;
>> +
>> +    spin_lock_irqsave(&xen_hotmem_lock, flags);
>> +    if (xen_hotmem.entry_nr) {
>> +            printk(KERN_WARNING "Still %x mem entries not added\n",
>> +              xen_hotmem.entry_nr);
>> +            list_for_each_safe(elem, tmp, &xen_hotmem.list) {
>> +                    entry = list_entry(elem, struct xen_hotmem_entry,
>> +                                    hotmem_list);
>> +                    xen_hotmem_free(entry);
>> +            }
>> +    }
>> +    spin_unlock_irqrestore(&xen_hotmem_lock, flags);
>> +}
>> +
>> +module_init(xen_memadd_init);
>
>Maybe call these 'xen_mem_hotadd_init' and 'xen_mem_hotadd_exit' to keep in
>style?

Sure.

>
>> +module_exit(xen_memadd_exit);
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/xen/acpi.h b/include/xen/acpi.h
>> index 3e7eab4..b15aa3e 100644
>> --- a/include/xen/acpi.h
>> +++ b/include/xen/acpi.h
>> @@ -41,6 +41,10 @@ int acpi_notify_hypervisor_state(u8 sleep_state,
>>  #define HOTPLUG_TYPE_ADD    0
>>  #define HOTPLUG_TYPE_REMOVE 1
>>
>> +#ifdef CONFIG_ACPI_HOTPLUG_MEMORY
>> +int xen_add_memory(struct acpi_memory_device *mem_device);
>> +#endif
>> +
>>  #if defined(CONFIG_ACPI_PROCESSOR_XEN) ||
>defined(CONFIG_ACPI_PROCESSOR_XEN_MODULE)
>>
>>  struct processor_cntl_xen_ops {
>> diff --git a/include/xen/interface/platform.h 
>> b/include/xen/interface/platform.h
>> index bff3a65..17ae622 100644
>> --- a/include/xen/interface/platform.h
>> +++ b/include/xen/interface/platform.h
>> @@ -344,6 +344,15 @@ struct xenpf_cpu_hotadd {
>>      uint32_t pxm;
>>  };
>>
>> +
>> +#define XENPF_mem_hotadd    59
>> +struct xenpf_mem_hotadd {
>> +    uint64_t spfn;
>> +    uint64_t epfn;
>> +    uint32_t pxm;
>> +    uint32_t flags;
>> +};
>> +
>>  struct xen_platform_op {
>>      uint32_t cmd;
>>      uint32_t interface_version; /* XENPF_INTERFACE_VERSION */
>> @@ -362,6 +371,7 @@ struct xen_platform_op {
>>              struct xenpf_pcpuinfo          pcpu_info;
>>              struct xenpf_cpu_ol            cpu_ol;
>>              struct xenpf_cpu_hotadd        cpu_add;
>> +            struct xenpf_mem_hotadd        mem_add;
>>              uint8_t                        pad[128];
>
>Should the size of the pad be decreased?

This is defined by xen hypervisor, and is intended for extenstion in future, I 
think.

I will update the patch and send it out next Monday.

--jyh

>>      } u;
>>  };
>> --
>> 1.5.4.2
>>
>>
>>
>
>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel