diff mbox series

[2/3] tools/init-xenstore-domain: support xenstore pvh stubdom

Message ID 20200923064541.19546-3-jgross@suse.com (mailing list archive)
State New, archived
Headers show
Series stubdom: add xenstore pvh stubdom support | expand

Commit Message

Jürgen Groß Sept. 23, 2020, 6:45 a.m. UTC 
Instead of creating the xenstore-stubdom domain first and parsing the
kernel later do it the other way round. This enables to probe for the
domain type supported by the xenstore-stubdom and to support both, pv
and pvh type stubdoms.

Try to parse the stubdom image first for PV support, if this fails use
HVM. Then create the domain with the appropriate type selected.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/init-xenstore-domain.c | 119 +++++++++++++++++++--------
 1 file changed, 85 insertions(+), 34 deletions(-)

Comments

Wei Liu Sept. 30, 2020, 3:46 p.m. UTC | #1 
On Wed, Sep 23, 2020 at 08:45:40AM +0200, Juergen Gross wrote:
> Instead of creating the xenstore-stubdom domain first and parsing the
> kernel later do it the other way round. This enables to probe for the
> domain type supported by the xenstore-stubdom and to support both, pv
> and pvh type stubdoms.
> 
> Try to parse the stubdom image first for PV support, if this fails use
> HVM. Then create the domain with the appropriate type selected.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
[...]
> +    dom->container_type = XC_DOM_HVM_CONTAINER;
> +    rv = xc_dom_parse_image(dom);
> +    if ( rv )
> +    {
> +        dom->container_type = XC_DOM_PV_CONTAINER;
> +        rv = xc_dom_parse_image(dom);
> +        if ( rv )
> +        {
> +            fprintf(stderr, "xc_dom_parse_image failed\n");
> +            goto err;
> +        }
> +    }
> +    else
> +    {
> +        config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
> +        config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
> +        dom->target_pages = mem_size >> XC_PAGE_SHIFT;
> +        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
> +        dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> +                          LAPIC_BASE_ADDRESS : mem_size;
> +        dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> +                           GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
> +        dom->mmio_start = LAPIC_BASE_ADDRESS;
> +        dom->max_vcpus = 1;
> +        e820[0].addr = 0;
> +        e820[0].size = dom->lowmem_end;
> +        e820[0].type = E820_RAM;
> +        e820[1].addr = LAPIC_BASE_ADDRESS;
> +        e820[1].size = dom->mmio_size;
> +        e820[1].type = E820_RESERVED;
> +        e820[2].addr = GB(4);
> +        e820[2].size = dom->highmem_end - GB(4);

Do you not want to check if highmem_end is larger than GB(4) before
putting in this region?

> +        e820[2].type = E820_RAM;
> +    }

This hardcoded e820 map doesn't seem very flexible, but we
control the guest kernel anyway so I think this should be fine.

The rest of this patch looks okay to me.

Wei.
Jürgen Groß Oct. 1, 2020, 10:40 a.m. UTC | #2 
On 30.09.20 17:46, Wei Liu wrote:
> On Wed, Sep 23, 2020 at 08:45:40AM +0200, Juergen Gross wrote:
>> Instead of creating the xenstore-stubdom domain first and parsing the
>> kernel later do it the other way round. This enables to probe for the
>> domain type supported by the xenstore-stubdom and to support both, pv
>> and pvh type stubdoms.
>>
>> Try to parse the stubdom image first for PV support, if this fails use
>> HVM. Then create the domain with the appropriate type selected.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> [...]
>> +    dom->container_type = XC_DOM_HVM_CONTAINER;
>> +    rv = xc_dom_parse_image(dom);
>> +    if ( rv )
>> +    {
>> +        dom->container_type = XC_DOM_PV_CONTAINER;
>> +        rv = xc_dom_parse_image(dom);
>> +        if ( rv )
>> +        {
>> +            fprintf(stderr, "xc_dom_parse_image failed\n");
>> +            goto err;
>> +        }
>> +    }
>> +    else
>> +    {
>> +        config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
>> +        config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
>> +        dom->target_pages = mem_size >> XC_PAGE_SHIFT;
>> +        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
>> +        dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
>> +                          LAPIC_BASE_ADDRESS : mem_size;
>> +        dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
>> +                           GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
>> +        dom->mmio_start = LAPIC_BASE_ADDRESS;
>> +        dom->max_vcpus = 1;
>> +        e820[0].addr = 0;
>> +        e820[0].size = dom->lowmem_end;
>> +        e820[0].type = E820_RAM;
>> +        e820[1].addr = LAPIC_BASE_ADDRESS;
>> +        e820[1].size = dom->mmio_size;
>> +        e820[1].type = E820_RESERVED;
>> +        e820[2].addr = GB(4);
>> +        e820[2].size = dom->highmem_end - GB(4);
> 
> Do you not want to check if highmem_end is larger than GB(4) before
> putting in this region?

Oh, indeed I should.

> 
>> +        e820[2].type = E820_RAM;
>> +    }
> 
> This hardcoded e820 map doesn't seem very flexible, but we
> control the guest kernel anyway so I think this should be fine.
> 
> The rest of this patch looks okay to me.

Thanks.


Juergen
Jürgen Groß Oct. 7, 2020, 6:54 a.m. UTC | #3 
On 30.09.20 17:46, Wei Liu wrote:
> On Wed, Sep 23, 2020 at 08:45:40AM +0200, Juergen Gross wrote:
>> Instead of creating the xenstore-stubdom domain first and parsing the
>> kernel later do it the other way round. This enables to probe for the
>> domain type supported by the xenstore-stubdom and to support both, pv
>> and pvh type stubdoms.
>>
>> Try to parse the stubdom image first for PV support, if this fails use
>> HVM. Then create the domain with the appropriate type selected.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> [...]
>> +    dom->container_type = XC_DOM_HVM_CONTAINER;
>> +    rv = xc_dom_parse_image(dom);
>> +    if ( rv )
>> +    {
>> +        dom->container_type = XC_DOM_PV_CONTAINER;
>> +        rv = xc_dom_parse_image(dom);
>> +        if ( rv )
>> +        {
>> +            fprintf(stderr, "xc_dom_parse_image failed\n");
>> +            goto err;
>> +        }
>> +    }
>> +    else
>> +    {
>> +        config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
>> +        config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
>> +        dom->target_pages = mem_size >> XC_PAGE_SHIFT;
>> +        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
>> +        dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
>> +                          LAPIC_BASE_ADDRESS : mem_size;
>> +        dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
>> +                           GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
>> +        dom->mmio_start = LAPIC_BASE_ADDRESS;
>> +        dom->max_vcpus = 1;
>> +        e820[0].addr = 0;
>> +        e820[0].size = dom->lowmem_end;
>> +        e820[0].type = E820_RAM;
>> +        e820[1].addr = LAPIC_BASE_ADDRESS;
>> +        e820[1].size = dom->mmio_size;
>> +        e820[1].type = E820_RESERVED;
>> +        e820[2].addr = GB(4);
>> +        e820[2].size = dom->highmem_end - GB(4);
> 
> Do you not want to check if highmem_end is larger than GB(4) before
> putting in this region?

Oh, just realized: further down I'm setting the guest's map with either
2 or 3 entries depending on dom->highmem_end value.

So I think this is fine.


Juergen
Wei Liu Oct. 7, 2020, 10:34 a.m. UTC | #4 
On Wed, Oct 07, 2020 at 08:54:43AM +0200, Jürgen Groß wrote:
> On 30.09.20 17:46, Wei Liu wrote:
> > On Wed, Sep 23, 2020 at 08:45:40AM +0200, Juergen Gross wrote:
> > > Instead of creating the xenstore-stubdom domain first and parsing the
> > > kernel later do it the other way round. This enables to probe for the
> > > domain type supported by the xenstore-stubdom and to support both, pv
> > > and pvh type stubdoms.
> > > 
> > > Try to parse the stubdom image first for PV support, if this fails use
> > > HVM. Then create the domain with the appropriate type selected.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > [...]
> > > +    dom->container_type = XC_DOM_HVM_CONTAINER;
> > > +    rv = xc_dom_parse_image(dom);
> > > +    if ( rv )
> > > +    {
> > > +        dom->container_type = XC_DOM_PV_CONTAINER;
> > > +        rv = xc_dom_parse_image(dom);
> > > +        if ( rv )
> > > +        {
> > > +            fprintf(stderr, "xc_dom_parse_image failed\n");
> > > +            goto err;
> > > +        }
> > > +    }
> > > +    else
> > > +    {
> > > +        config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
> > > +        config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
> > > +        dom->target_pages = mem_size >> XC_PAGE_SHIFT;
> > > +        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
> > > +        dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> > > +                          LAPIC_BASE_ADDRESS : mem_size;
> > > +        dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> > > +                           GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
> > > +        dom->mmio_start = LAPIC_BASE_ADDRESS;
> > > +        dom->max_vcpus = 1;
> > > +        e820[0].addr = 0;
> > > +        e820[0].size = dom->lowmem_end;
> > > +        e820[0].type = E820_RAM;
> > > +        e820[1].addr = LAPIC_BASE_ADDRESS;
> > > +        e820[1].size = dom->mmio_size;
> > > +        e820[1].type = E820_RESERVED;
> > > +        e820[2].addr = GB(4);
> > > +        e820[2].size = dom->highmem_end - GB(4);
> > 
> > Do you not want to check if highmem_end is larger than GB(4) before
> > putting in this region?
> 
> Oh, just realized: further down I'm setting the guest's map with either
> 2 or 3 entries depending on dom->highmem_end value.
> 
> So I think this is fine.
> 

Fair enough.

Acked-by: Wei Liu <wl@xen.org>

Wei.

> 
> Juergen
>
diff mbox series

Patch

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 22c4be6a3f..c174357c0e 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -18,6 +18,10 @@ 
 #include "init-dom-json.h"
 #include "_paths.h"
 
+#define LAPIC_BASE_ADDRESS  0xfee00000UL
+#define MB(x)               ((uint64_t)x << 20)
+#define GB(x)               ((uint64_t)x << 30)
+
 static uint32_t domid = ~0;
 static char *kernel;
 static char *ramdisk;
@@ -69,6 +73,8 @@  static int build(xc_interface *xch)
     int rv, xs_fd;
     struct xc_dom_image *dom = NULL;
     int limit_kb = (maxmem ? : (memory + 1)) * 1024;
+    uint64_t mem_size = MB(memory);
+    struct e820entry e820[3];
     struct xen_domctl_createdomain config = {
         .ssidref = SECINITSID_DOMU,
         .flags = XEN_DOMCTL_CDF_xs_domain,
@@ -101,6 +107,66 @@  static int build(xc_interface *xch)
         }
     }
 
+    dom = xc_dom_allocate(xch, NULL, NULL);
+    if ( !dom )
+    {
+        fprintf(stderr, "xc_dom_allocate failed\n");
+        rv = -1;
+        goto err;
+    }
+
+    rv = xc_dom_kernel_file(dom, kernel);
+    if ( rv )
+    {
+        fprintf(stderr, "xc_dom_kernel_file failed\n");
+        goto err;
+    }
+
+    if ( ramdisk )
+    {
+        rv = xc_dom_module_file(dom, ramdisk, NULL);
+        if ( rv )
+        {
+            fprintf(stderr, "xc_dom_module_file failed\n");
+            goto err;
+        }
+    }
+
+    dom->container_type = XC_DOM_HVM_CONTAINER;
+    rv = xc_dom_parse_image(dom);
+    if ( rv )
+    {
+        dom->container_type = XC_DOM_PV_CONTAINER;
+        rv = xc_dom_parse_image(dom);
+        if ( rv )
+        {
+            fprintf(stderr, "xc_dom_parse_image failed\n");
+            goto err;
+        }
+    }
+    else
+    {
+        config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
+        config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
+        dom->target_pages = mem_size >> XC_PAGE_SHIFT;
+        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
+        dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
+                          LAPIC_BASE_ADDRESS : mem_size;
+        dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
+                           GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
+        dom->mmio_start = LAPIC_BASE_ADDRESS;
+        dom->max_vcpus = 1;
+        e820[0].addr = 0;
+        e820[0].size = dom->lowmem_end;
+        e820[0].type = E820_RAM;
+        e820[1].addr = LAPIC_BASE_ADDRESS;
+        e820[1].size = dom->mmio_size;
+        e820[1].type = E820_RESERVED;
+        e820[2].addr = GB(4);
+        e820[2].size = dom->highmem_end - GB(4);
+        e820[2].type = E820_RAM;
+    }
+
     rv = xc_domain_create(xch, &domid, &config);
     if ( rv )
     {
@@ -125,11 +191,15 @@  static int build(xc_interface *xch)
         fprintf(stderr, "xc_evtchn_alloc_unbound failed\n");
         goto err;
     }
-    rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
-    if ( rv )
+
+    if ( dom->container_type == XC_DOM_PV_CONTAINER )
     {
-        fprintf(stderr, "xc_domain_set_memmap_limit failed\n");
-        goto err;
+        rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
+        if ( rv )
+        {
+            fprintf(stderr, "xc_domain_set_memmap_limit failed\n");
+            goto err;
+        }
     }
 
     rv = ioctl(xs_fd, IOCTL_XENBUS_BACKEND_SETUP, domid);
@@ -144,45 +214,16 @@  static int build(xc_interface *xch)
     else
         snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
-    dom = xc_dom_allocate(xch, cmdline, NULL);
-    if ( !dom )
-    {
-        fprintf(stderr, "xc_dom_allocate failed\n");
-        goto err;
-    }
-    dom->container_type = XC_DOM_PV_CONTAINER;
+    dom->cmdline = xc_dom_strdup(dom, cmdline);
     dom->xenstore_domid = domid;
     dom->console_evtchn = console_evtchn;
 
-    rv = xc_dom_kernel_file(dom, kernel);
-    if ( rv )
-    {
-        fprintf(stderr, "xc_dom_kernel_file failed\n");
-        goto err;
-    }
-
-    if ( ramdisk )
-    {
-        rv = xc_dom_module_file(dom, ramdisk, NULL);
-        if ( rv )
-        {
-            fprintf(stderr, "xc_dom_module_file failed\n");
-            goto err;
-        }
-    }
-
     rv = xc_dom_boot_xen_init(dom, xch, domid);
     if ( rv )
     {
         fprintf(stderr, "xc_dom_boot_xen_init failed\n");
         goto err;
     }
-    rv = xc_dom_parse_image(dom);
-    if ( rv )
-    {
-        fprintf(stderr, "xc_dom_parse_image failed\n");
-        goto err;
-    }
     rv = xc_dom_mem_init(dom, memory);
     if ( rv )
     {
@@ -195,6 +236,16 @@  static int build(xc_interface *xch)
         fprintf(stderr, "xc_dom_boot_mem_init failed\n");
         goto err;
     }
+    if ( dom->container_type == XC_DOM_HVM_CONTAINER )
+    {
+        rv = xc_domain_set_memory_map(xch, domid, e820,
+                                      dom->highmem_end ? 3 : 2);
+        if ( rv )
+        {
+            fprintf(stderr, "xc_domain_set_memory_map failed\n");
+            goto err;
+        }
+    }
     rv = xc_dom_build_image(dom);
     if ( rv )
     {