Re: HVM support for e820_host (Was: Bug: Limitation of <=2GB RAM in domU persists with 4.3.0)

[Gordan Bobic <gordan@bobich.net>]

[-- Attachment #1: Type: text/plain, Size: 4296 bytes --]

 I have this at the point where it actually builds.
 Otherwise completely untested (will do that later today).

 Attached are:

 1) libxl patch
 Modified from the original patch to _not_ implicitly enable
 e820_host when PCI devices are passed.

 2) Mukesh's hypervisor e820 patch from here:
 http://lists.xen.org/archives/html/xen-devel/2013-05/msg01603.html
 Modified slightly to attempt to address Jan's comment on the same
 thread, and to adjust the diff line pointers to match against
 4.3.0 release code.

 3) A patch based on Konrad's earlier in this thread, with
 a few additions and changes to make it all compile.

 Some peer review would be most welcome - this is my first
 venture into Xen code, so please do assume that I have
 no idea what I'm doing at the moment. :)

 I added yet another E820MAX #define, this time to
 tools/firmware/hvmloader/e820.h

 If there is a better place to #include that via from
 e820.c, please point me in the right direction.

 Gordan

 On Wed, 04 Sep 2013 12:01:09 +0100, Gordan Bobic <gordan@bobich.net> 
 wrote:
> On Tue, 3 Sep 2013 17:08:33 -0400, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
>
>> Oh, and something like this probably should do it - not compile 
>> tested
>> in any way:
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 1fcaed0..7b38890 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3146,6 +3146,7 @@ static long hvm_memory_op(int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>      case XENMEM_machine_memory_map:
>>      case XENMEM_machphys_mapping:
>>          return -ENOSYS;
>> +    case XENMEM_memory_map:
>>      case XENMEM_decrease_reservation:
>>          rc = do_memory_op(cmd, arg);
>>          current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 
>> 1;
>
> This seems to work better. :)
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3142,10 +3142,10 @@ static long hvm_memory_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>
>      switch ( cmd & MEMOP_CMD_MASK )
>      {
> -    case XENMEM_memory_map:
>      case XENMEM_machine_memory_map:
>      case XENMEM_machphys_mapping:
>          return -ENOSYS;
> +    case XENMEM_memory_map:
>      case XENMEM_decrease_reservation:
>          rc = do_memory_op(cmd, arg);
>          current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 
> 1;
>
>
>> diff --git a/tools/firmware/hvmloader/e820.c
>> b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..86fb20a 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -68,16 +68,42 @@ void dump_e820_table(struct e820entry *e820,
>> unsigned int nr)
>>      }
>>  }
>>
>> +static const char *e820_names(int type)
>> +{
>> +    switch (type) {
>> +        case E820_RAM: return "RAM";
>> +        case E820_RESERVED: return "Reserved";
>> +        case E820_ACPI: return "ACPI";
>> +        case E820_NVS: return "ACPI NVS";
>> +        case E820_UNUSABLE: return "Unusable";
>> +        default: break;
>> +    }
>> +    return "Unknown";
>> +}
>
> To make this work I also added:
>
> --- tools/firmware/hvmloader/e820.h.orig	2013-09-04 
> 10:55:38.317275183 +0100
> +++ tools/firmware/hvmloader/e820.h	2013-09-04 10:56:14.374595809 
> +0100
> @@ -8,6 +8,7 @@
>  #define E820_RESERVED     2
>  #define E820_ACPI         3
>  #define E820_NVS          4
> +#define E820_UNUSBLE      5
>
>  struct e820entry {
>      uint64_t addr;
>
> It that OK?
>
>>  /* Create an E820 table based on memory parameters provided in 
>> hvm_info. */
>>  int build_e820_table(struct e820entry *e820,
>>                       unsigned int lowmem_reserved_base,
>>                       unsigned int bios_image_base)
>>  {
>>      unsigned int nr = 0;
>> +    struct xen_memory_map op;
>> +    struct e820entry map[E820MAX];
>> +    int rc;
>>
>>      if ( !lowmem_reserved_base )
>>              lowmem_reserved_base = 0xA0000;
>>
>> +    set_xen_guest_handle(op.buffer, map);
>> +
>> +    rc = hypercall_memory_op ( XENMEM_memory_op, &op);
>
> Where is XENMEM_memory_op defined?
> Should that be XENMEM_memory_map? Or maybe XENMEM_populate_physmap?
>
> Gordan
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[-- Attachment #2: xen-hvm-libxl-e820_host.patch --]
[-- Type: text/x-c, Size: 5502 bytes --]

--- xen-4.3.0/tools/libxl/libxl_create.c.orig	2013-09-03 14:26:47.478350269 +0100
+++ xen-4.3.0/tools/libxl/libxl_create.c	2013-09-03 14:45:26.710553063 +0100
@@ -208,6 +208,8 @@
 
     libxl_defbool_setdefault(&b_info->disable_migrate, false);
 
+    libxl_defbool_setdefault(&b_info->e820_host, false);
+
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
@@ -280,7 +282,6 @@
 
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        libxl_defbool_setdefault(&b_info->u.pv.e820_host, false);
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->shadow_memkb = 0;
         if (b_info->u.pv.slack_memkb == LIBXL_MEMKB_DEFAULT)
--- xen-4.3.0/tools/libxl/libxl_types.idl.orig	2013-09-03 14:16:48.462767589 +0100
+++ xen-4.3.0/tools/libxl/libxl_types.idl	2013-09-03 14:18:19.624028024 +0100
@@ -295,6 +295,8 @@
     ("irqs",             Array(uint32, "num_irqs")),
     ("iomem",            Array(libxl_iomem_range, "num_iomem")),
     ("claim_mode",	     libxl_defbool),
+    # Use host's E820 for PCI passthrough.
+    ("e820_host",        libxl_defbool),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
@@ -340,8 +342,6 @@
                                       ("cmdline", string),
                                       ("ramdisk", string),
                                       ("features", string, {'const': True}),
-                                      # Use host's E820 for PCI passthrough.
-                                      ("e820_host", libxl_defbool),
                                       ])),
                  ("invalid", Struct(None, [])),
                  ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
--- xen-4.3.0/tools/libxl/libxl_x86.c.orig	2013-09-03 14:26:36.093566315 +0100
+++ xen-4.3.0/tools/libxl/libxl_x86.c	2013-09-03 16:52:24.648701260 +0100
@@ -216,11 +216,8 @@
     struct e820entry map[E820MAX];
     libxl_domain_build_info *b_info;
 
-    if (d_config == NULL || d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
-        return ERROR_INVAL;
-
     b_info = &d_config->b_info;
-    if (!libxl_defbool_val(b_info->u.pv.e820_host))
+    if (!libxl_defbool_val(b_info->e820_host))
         return ERROR_INVAL;
 
     rc = xc_get_machine_memory_map(ctx->xch, map, E820MAX);
@@ -229,9 +226,15 @@
         return ERROR_FAIL;
     }
     nr = rc;
-    rc = e820_sanitize(ctx, map, &nr, b_info->target_memkb,
-                       (b_info->max_memkb - b_info->target_memkb) +
-                       b_info->u.pv.slack_memkb);
+    if (d_config == NULL || d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
+        rc = e820_sanitize(ctx, map, &nr, b_info->target_memkb,
+                           (b_info->max_memkb - b_info->target_memkb));
+    } else if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
+        rc = e820_sanitize(ctx, map, &nr, b_info->target_memkb,
+                           (b_info->max_memkb - b_info->target_memkb) +
+                           b_info->u.pv.slack_memkb);
+    }
+
     if (rc)
         return ERROR_FAIL;
 
@@ -296,8 +299,7 @@
         xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, NULL, 0, &shadow, 0, NULL);
     }
 
-    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
-            libxl_defbool_val(d_config->b_info.u.pv.e820_host)) {
+    if (libxl_defbool_val(d_config->b_info.e820_host)) {
         ret = libxl__e820_alloc(gc, domid, d_config);
         if (ret) {
             LIBXL__LOG_ERRNO(gc->owner, LIBXL__LOG_ERROR,
--- xen-4.3.0/tools/libxl/xl_cmdimpl.c.orig	2013-09-03 14:26:54.524214804 +0100
+++ xen-4.3.0/tools/libxl/xl_cmdimpl.c	2013-09-03 14:47:11.811612562 +0100
@@ -1274,11 +1274,7 @@
     if (!xlu_cfg_get_long (config, "pci_permissive", &l, 0))
         pci_permissive = l;
 
-    /* To be reworked (automatically enabled) once the auto ballooning
-     * after guest starts is done (with PCI devices passed in). */
-    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
-        xlu_cfg_get_defbool(config, "e820_host", &b_info->u.pv.e820_host, 0);
-    }
+    xlu_cfg_get_defbool(config, "e820_host", &b_info->e820_host, 0);
 
     if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
         d_config->num_pcidevs = 0;
@@ -1297,7 +1293,7 @@
                 d_config->num_pcidevs++;
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
-            libxl_defbool_set(&b_info->u.pv.e820_host, true);
+            libxl_defbool_set(&b_info->e820_host, true);
     }
 
     switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
--- xen-4.3.0/tools/libxl/xl_sxp.c.orig	2013-09-03 14:25:37.839675572 +0100
+++ xen-4.3.0/tools/libxl/xl_sxp.c	2013-09-03 14:22:13.953561029 +0100
@@ -87,6 +87,10 @@
         }
     }
 
+    printf("\t(e820_host %s)\n",
+           libxl_defbool_to_string(b_info->e820_host));
+
+
     printf("\t(image\n");
     switch (c_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
@@ -150,8 +154,6 @@
         printf("\t\t\t(kernel %s)\n", b_info->u.pv.kernel);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk);
-        printf("\t\t\t(e820_host %s)\n",
-               libxl_defbool_to_string(b_info->u.pv.e820_host));
         printf("\t\t)\n");
         break;
     default:

[-- Attachment #3: xen-hvm-hypervisor-e820_host.patch --]
[-- Type: text/x-diff, Size: 3704 bytes --]

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b9711d2..8b04339 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -567,7 +567,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
         /* 64-bit PV guest by default. */
         d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
 
-        spin_lock_init(&d->arch.pv_domain.e820_lock);
+        spin_lock_init(&d->arch.e820_lock);
     }
 
     /* initialize default tsc behavior in case tools don't */
@@ -592,8 +592,8 @@ void arch_domain_destroy(struct domain *d)
 {
     if ( is_hvm_domain(d) )
         hvm_domain_destroy(d);
-    else
-        xfree(d->arch.pv_domain.e820);
+
+    xfree(d->arch.e820);
 
     free_domain_pirqs(d);
     if ( !is_idle_domain(d) )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 43eeddc..60f1a4f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4762,11 +4762,11 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         }
 
-        spin_lock(&d->arch.pv_domain.e820_lock);
-        xfree(d->arch.pv_domain.e820);
-        d->arch.pv_domain.e820 = e820;
-        d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
-        spin_unlock(&d->arch.pv_domain.e820_lock);
+        spin_lock(&d->arch.e820_lock);
+        xfree(d->arch.e820);
+        d->arch.e820 = e820;
+        d->arch.nr_e820 = fmap.map.nr_entries;
+        spin_unlock(&d->arch.e820_lock);
 
         rcu_unlock_domain(d);
         return rc;
@@ -4780,26 +4780,26 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&map, arg, 1) )
             return -EFAULT;
 
-        spin_lock(&d->arch.pv_domain.e820_lock);
+        spin_lock(&d->arch.e820_lock);
 
         /* Backwards compatibility. */
-        if ( (d->arch.pv_domain.nr_e820 == 0) ||
-             (d->arch.pv_domain.e820 == NULL) )
+        if ( (d->arch.nr_e820 == 0) ||
+             (d->arch.e820 == NULL) )
         {
-            spin_unlock(&d->arch.pv_domain.e820_lock);
+            spin_unlock(&d->arch.e820_lock);
             return -ENOSYS;
         }
 
-        map.nr_entries = min(map.nr_entries, d->arch.pv_domain.nr_e820);
-        if ( copy_to_guest(map.buffer, d->arch.pv_domain.e820,
+        map.nr_entries = min(map.nr_entries, d->arch.nr_e820);
+        if ( copy_to_guest(map.buffer, d->arch.e820,
                            map.nr_entries) ||
              __copy_to_guest(arg, &map, 1) )
         {
-            spin_unlock(&d->arch.pv_domain.e820_lock);
+            spin_unlock(&d->arch.e820_lock);
             return -EFAULT;
         }
 
-        spin_unlock(&d->arch.pv_domain.e820_lock);
+        spin_unlock(&d->arch.e820_lock);
         return 0;
     }
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 83fbe58..1d5783f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -234,11 +234,6 @@ struct pv_domain
 
     /* map_domain_page() mapping cache. */
     struct mapcache_domain mapcache;
-
-    /* Pseudophysical e820 map (XENMEM_memory_map).  */
-    spinlock_t e820_lock;
-    struct e820entry *e820;
-    unsigned int nr_e820;
 };
 
 struct arch_domain
@@ -313,6 +308,11 @@ struct arch_domain
                                 (possibly other cases in the future */
     uint64_t vtsc_kerncount; /* for hvm, counts all vtsc */
     uint64_t vtsc_usercount; /* not used for hvm */
+
+    /* Pseudophysical e820 map (XENMEM_memory_map).  */
+    spinlock_t e820_lock;
+    struct e820entry *e820;
+    unsigned int nr_e820;
 } __cacheline_aligned;
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
-- 
1.7.2.3

[-- Attachment #4: xen-hvm-loader-e820_host.patch --]
[-- Type: text/x-diff, Size: 3164 bytes --]

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1fcaed0..7b38890 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3142,10 +3142,10 @@ static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     switch ( cmd & MEMOP_CMD_MASK )
     {
-    case XENMEM_memory_map:
     case XENMEM_machine_memory_map:
     case XENMEM_machphys_mapping:
         return -ENOSYS;
+    case XENMEM_memory_map:
     case XENMEM_decrease_reservation:
         rc = do_memory_op(cmd, arg);
         current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
@@ -3216,10 +3217,10 @@ static long hvm_memory_op_compat32(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     switch ( cmd & MEMOP_CMD_MASK )
     {
-    case XENMEM_memory_map:
     case XENMEM_machine_memory_map:
     case XENMEM_machphys_mapping:
         return -ENOSYS;
+    case XENMEM_memory_map:
     case XENMEM_decrease_reservation:
         rc = compat_memory_op(cmd, arg);
         current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
diff --git a/tools/firmware/hvmloader/e820.h.orig b/tools/firmware/hvmloader/e820.h
index b2ead7f..2fa700d 100644
--- a/tools/firmware/hvmloader/e820.h.orig
+++ b/tools/firmware/hvmloader/e820.h
@@ -8,6 +8,9 @@
 #define E820_RESERVED     2
 #define E820_ACPI         3
 #define E820_NVS          4
+#define E820_UNUSABLE     5
+
+#define E820MAX         128
 
 struct e820entry {
     uint64_t addr;
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 2e05e93..86fb20a 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -22,6 +22,9 @@
 
 #include "config.h"
 #include "util.h"
+#include "hypercall.h"
+#include <xen/memory.h>
+#include <errno.h>
 
 void dump_e820_table(struct e820entry *e820, unsigned int nr)
 {
@@ -68,16 +68,42 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
     }
 }
 
+static const char *e820_names(int type)
+{
+    switch (type) {
+        case E820_RAM: return "RAM";
+        case E820_RESERVED: return "Reserved";
+        case E820_ACPI: return "ACPI";
+        case E820_NVS: return "ACPI NVS";
+        case E820_UNUSABLE: return "Unusable";
+        default: break;
+    }
+    return "Unknown";
+}
+
+
 /* Create an E820 table based on memory parameters provided in hvm_info. */
 int build_e820_table(struct e820entry *e820,
                      unsigned int lowmem_reserved_base,
                      unsigned int bios_image_base)
 {
     unsigned int nr = 0;
+    struct xen_memory_map op;
+    struct e820entry map[E820MAX];
+    int rc;
 
     if ( !lowmem_reserved_base )
             lowmem_reserved_base = 0xA0000;
 
+    set_xen_guest_handle(op.buffer, map);
+
+    rc = hypercall_memory_op ( XENMEM_memory_map, &op);
+    if ( rc != -ENOSYS) { /* It works!? */
+        int i;
+        for ( i = 0; i < op.nr_entries; i++ )
+            printf("    %llx -> %llx %s\n", map[i].addr >> 12,
+                   (map[i].addr + map[i].size) >> 12, e820_names(map[i].type));
+    }
     /* Lowmem must be at least 512K to keep Windows happy) */
     ASSERT ( lowmem_reserved_base > 512<<10 );

[-- Attachment #5: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel