WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] Add memory hotadd to pvops dom0
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Fri, 18 Dec 2009 09:19:52 -0500
Cc: Jeremy Fitzhardinge <jeremy@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 18 Dec 2009 06:30:14 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <C8EDE645B81E5141A8C6B2F73FD9265105AE092FE1@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <C8EDE645B81E5141A8C6B2F73FD9265105AE092FE1@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.19 (2009-01-05)
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?


> +
> +     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?

> +{
> +     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?

> +             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.

> +             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?

> +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?
>       } 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