[PATCH v3 0/5] HVMlite: DomU fixes and a Dom0 preparatory patch

[PATCH v3 0/5] HVMlite: DomU fixes and a Dom0 preparatory patch

[Roger Pau Monne]

Hello, 

The series is sorted in the following way:
 - Patch 1/5 is a preparatory patch for Dom0 HVMlite support.
 - Patch 4/5 is a fix from a fallout introduced by the HVMlite series, which 
   inadvertently disabled the emulated PIT that was added to PV(H) guests.
 - Patches 2, 3 and 5 expand the fields of the libxl_domain_build_info 
   structure in order to store whether emulated devices inside of Xen are 
   enabled or not.
 - NOTE: patch 4/5 and 5/5 should be committed together. I haven't squashed 
   them together in order to ease the review, but I can do so in the next 
   iteration if requested.

The full series can also be found in my git tree:

http://xenbits.xen.org/gitweb/?p=people/royger/xen.git;a=shortlog;h=refs/heads/hvmlite_fixes_v3

I hope this will help review of patch 1/5, which is a significant rework, 
and will probably be better reviewed by looking at the resulting code.

Thanks, Roger.

[PATCH v3 1/5] libelf: rewrite symtab/strtab loading for Dom0

[Roger Pau Monne]

Current implementation of elf_load_bsdsyms is broken when loading inside of
a HVM guest, because it assumes elf_memcpy_safe is able to write into guest
memory space, which it is not.

Take the oportunity to do some cleanup and properly document how
elf_{parse/load}_bsdsyms works. The new implementation uses elf_load_image
when dealing with data that needs to be copied to the guest memory space.
Also reduce the number of section headers copied to the minimum necessary.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/libelf/libelf-loader.c | 213 ++++++++++++++++++++++++++++----------
 1 file changed, 158 insertions(+), 55 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 6f42bea..9552d4c 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -153,7 +153,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
 {
     uint64_t sz;
     ELF_HANDLE_DECL(elf_shdr) shdr;
-    unsigned i, type;
+    unsigned int i;
 
     if ( !ELF_HANDLE_VALID(elf->sym_tab) )
         return;
@@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
     sz = sizeof(uint32_t);
 
     /* Space for the elf and elf section headers */
-    sz += (elf_uval(elf, elf->ehdr, e_ehsize) +
-           elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize));
+    sz += elf_uval(elf, elf->ehdr, e_ehsize) +
+          3 * elf_uval(elf, elf->ehdr, e_shentsize);
     sz = elf_round_up(elf, sz);
 
-    /* Space for the symbol and string tables. */
+    /* Space for the symbol and string table. */
     for ( i = 0; i < elf_shdr_count(elf); i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
             /* input has an insane section header count field */
             break;
-        type = elf_uval(elf, shdr, sh_type);
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
-            sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
+
+        if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB )
+            continue;
+
+        sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
+        shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link));
+
+        if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+            /* input has an insane section header count field */
+            break;
+
+        if ( elf_uval(elf, shdr, sh_type) != SHT_STRTAB )
+            /* Invalid symtab -> strtab link */
+            break;
+
+        sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
     }
 
     elf->bsd_symtab_pstart = pstart;
@@ -186,13 +199,31 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
 
 static void elf_load_bsdsyms(struct elf_binary *elf)
 {
-    ELF_HANDLE_DECL(elf_ehdr) sym_ehdr;
-    unsigned long sz;
-    elf_ptrval maxva;
-    elf_ptrval symbase;
-    elf_ptrval symtab_addr;
-    ELF_HANDLE_DECL(elf_shdr) shdr;
-    unsigned i, type;
+    /*
+     * Header that is placed at the end of the kernel and allows
+     * the OS to find where the symtab and strtab have been loaded.
+     * It mimics a valid ELF file header, although it only contains
+     * a symtab and a strtab section.
+     *
+     * NB: according to the ELF spec there's only ONE symtab per ELF
+     * file, and accordingly we will only load the corresponding
+     * strtab, so we only need three section headers in our fake ELF
+     * header (first section header is always a dummy).
+     */
+    struct __packed {
+        elf_ehdr header;
+        elf_shdr section[3];
+    } symbol_header;
+
+    ELF_HANDLE_DECL(elf_ehdr) header_handle;
+    unsigned long shdr_size;
+    uint32_t symsize;
+    ELF_HANDLE_DECL(elf_shdr) section_handle;
+    ELF_HANDLE_DECL(elf_shdr) image_handle;
+    unsigned int i, link;
+    elf_ptrval header_base;
+    elf_ptrval symtab_base;
+    elf_ptrval strtab_base;
 
     if ( !elf->bsd_symtab_pstart )
         return;
@@ -200,64 +231,136 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
 #define elf_hdr_elm(_elf, _hdr, _elm, _val)     \
 do {                                            \
     if ( elf_64bit(_elf) )                      \
-        elf_store_field(_elf, _hdr, e64._elm, _val);  \
+        (_hdr).e64._elm = _val;                 \
     else                                        \
-        elf_store_field(_elf, _hdr, e32._elm, _val);  \
+        (_hdr).e32._elm = _val;                 \
 } while ( 0 )
 
-    symbase = elf_get_ptr(elf, elf->bsd_symtab_pstart);
-    symtab_addr = maxva = symbase + sizeof(uint32_t);
+#define SYMTAB_INDEX    1
+#define STRTAB_INDEX    2
 
-    /* Set up Elf header. */
-    sym_ehdr = ELF_MAKE_HANDLE(elf_ehdr, symtab_addr);
-    sz = elf_uval(elf, elf->ehdr, e_ehsize);
-    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz);
-    maxva += sz; /* no round up */
+    /* Allow elf_memcpy_safe to write to symbol_header. */
+    elf->caller_xdest_base = &symbol_header;
+    elf->caller_xdest_size = sizeof(symbol_header);
 
-    elf_hdr_elm(elf, sym_ehdr, e_phoff, 0);
-    elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize));
-    elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0);
-    elf_hdr_elm(elf, sym_ehdr, e_phnum, 0);
-
-    /* Copy Elf section headers. */
-    shdr = ELF_MAKE_HANDLE(elf_shdr, maxva);
-    sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize);
-    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr),
-                    ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
-                    sz);
-    maxva = elf_round_up(elf, (unsigned long)maxva + sz);
+    /*
+     * Calculate the position of the various elements in GUEST MEMORY SPACE.
+     * This addresses MUST only be used with elf_load_image.
+     *
+     * NB: strtab_base cannot be calculated at this point because we don't
+     * know the size of the symtab yet, and the strtab will be placed after it.
+     */
+    header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + sizeof(uint32_t);
+    symtab_base = elf_round_up(elf, header_base + sizeof(symbol_header));
+
+    /* Fill the ELF header, copied from the original ELF header. */
+    header_handle = ELF_MAKE_HANDLE(elf_ehdr,
+                                    ELF_REALPTR2PTRVAL(&symbol_header.header));
+    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle),
+                    ELF_HANDLE_PTRVAL(elf->ehdr),
+                    elf_uval(elf, elf->ehdr, e_ehsize));
+
+    /* Set the offset to the shdr array. */
+    elf_hdr_elm(elf, symbol_header.header, e_shoff,
+                offsetof(typeof(symbol_header), section));
+
+    /* Set the right number of section headers. */
+    elf_hdr_elm(elf, symbol_header.header, e_shnum, 3);
+
+    /* Clear a couple of fields we don't use. */
+    elf_hdr_elm(elf, symbol_header.header, e_phoff, 0);
+    elf_hdr_elm(elf, symbol_header.header, e_phentsize, 0);
+    elf_hdr_elm(elf, symbol_header.header, e_phnum, 0);
+
+    /* Zero the dummy section. */
+    section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                     ELF_REALPTR2PTRVAL(&symbol_header.section[SHN_UNDEF]));
+    shdr_size = elf_uval(elf, elf->ehdr, e_shentsize);
+    elf_memset_safe(elf, ELF_HANDLE_PTRVAL(section_handle), 0, shdr_size);
 
+    /*
+     * Find the actual symtab and strtab in the ELF.
+     *
+     * The symtab section header is going to reside in section[SYMTAB_INDEX],
+     * while the corresponding strtab is going to be placed in
+     * section[STRTAB_INDEX]. sh_offset is mangled so it points to the offset
+     * where the sections are actually loaded (relative to the ELF header
+     * location).
+     */
+    section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                     ELF_REALPTR2PTRVAL(&symbol_header.section[SYMTAB_INDEX]));
     for ( i = 0; i < elf_shdr_count(elf); i++ )
     {
-        elf_ptrval old_shdr_p;
-        elf_ptrval new_shdr_p;
 
-        type = elf_uval(elf, shdr, sh_type);
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
+        image_handle = elf_shdr_by_index(elf, i);
+        if ( elf_uval(elf, image_handle, sh_type) != SHT_SYMTAB )
+            continue;
+
+        elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle),
+                        ELF_HANDLE_PTRVAL(image_handle),
+                        shdr_size);
+
+        link = elf_uval(elf, section_handle, sh_link);
+        if ( link == SHN_UNDEF )
         {
-             elf_msg(elf, "%s: shdr %i at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", __func__, i,
-                     elf_section_start(elf, shdr), maxva);
-             sz = elf_uval(elf, shdr, sh_size);
-             elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz);
-             /* Mangled to be based on ELF header location. */
-             elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
-             maxva = elf_round_up(elf, (unsigned long)maxva + sz);
+            elf_mark_broken(elf, "bad link in symtab");
+            break;
         }
-        old_shdr_p = ELF_HANDLE_PTRVAL(shdr);
-        new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize);
-        if ( new_shdr_p <= old_shdr_p ) /* wrapped or stuck */
+
+        /* Load symtab into guest memory. */
+        elf_load_image(elf, symtab_base, elf_section_start(elf, section_handle),
+                       elf_uval(elf, section_handle, sh_size),
+                       elf_uval(elf, section_handle, sh_size));
+        elf_hdr_elm(elf, symbol_header.section[SYMTAB_INDEX], sh_offset,
+                    symtab_base - header_base);
+        elf_hdr_elm(elf, symbol_header.section[SYMTAB_INDEX], sh_link,
+                    STRTAB_INDEX);
+
+        /* Calculate the guest address where strtab is loaded. */
+        strtab_base = elf_round_up(elf, symtab_base +
+                                   elf_uval(elf, section_handle, sh_size));
+
+        /* Load strtab section header. */
+        section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                    ELF_REALPTR2PTRVAL(&symbol_header.section[STRTAB_INDEX]));
+        elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle),
+                        ELF_HANDLE_PTRVAL(elf_shdr_by_index(elf, link)),
+                        shdr_size);
+
+        if ( elf_uval(elf, section_handle, sh_type) != SHT_STRTAB )
         {
-            elf_mark_broken(elf, "bad section header length");
+            elf_mark_broken(elf, "strtab not found");
             break;
         }
-        if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */
-            break;
-        shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p);
+
+        /* Load strtab into guest memory. */
+        elf_load_image(elf, strtab_base, elf_section_start(elf, section_handle),
+                       elf_uval(elf, section_handle, sh_size),
+                       elf_uval(elf, section_handle, sh_size));
+        elf_hdr_elm(elf, symbol_header.section[STRTAB_INDEX], sh_offset,
+                    strtab_base - header_base);
+
+        /* Store the whole size (including headers and loaded sections). */
+        symsize = strtab_base + elf_uval(elf, section_handle, sh_size) -
+                  header_base;
+        break;
     }
 
-    /* Write down the actual sym size. */
-    elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr);
+    /* Load the total size at symtab_pstart. */
+    elf_load_image(elf, elf_get_ptr(elf, elf->bsd_symtab_pstart),
+                   ELF_REALPTR2PTRVAL(&symsize), sizeof(symsize),
+                   sizeof(symsize));
+
+    /* Load the headers. */
+    elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&symbol_header),
+                   sizeof(symbol_header), sizeof(symbol_header));
+
+    /* Remove permissions from elf_memcpy_safe. */
+    elf->caller_xdest_base = NULL;
+    elf->caller_xdest_size = 0;
 
+#undef SYMTAB_INDEX
+#undef STRTAB_INDEX
 #undef elf_ehdr_elm
 }
 
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v3 2/5] libxl: introduce LIBXL_VGA_INTERFACE_TYPE_UNDEF

[Roger Pau Monne]

And use it as the default value for the VGA kind. This allows libxl to set
it to the default value later on when the domain type is known. For HVM
guests the default value is LIBXL_VGA_INTERFACE_TYPE_CIRRUS while for
HVMlite the default value is LIBXL_VGA_INTERFACE_TYPE_NONE.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_create.c  | 8 ++++++--
 tools/libxl/libxl_dm.c      | 6 ++++++
 tools/libxl/libxl_types.idl | 3 ++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e491d83..61a4001 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -206,8 +206,12 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         if (b_info->u.hvm.mmio_hole_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->u.hvm.mmio_hole_memkb = 0;
 
-        if (!b_info->u.hvm.vga.kind)
-            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_UNDEF) {
+            if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)
+                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_NONE;
+            else
+                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+        }
 
         if (!b_info->u.hvm.hdtype)
             b_info->u.hvm.hdtype = LIBXL_HDTYPE_IDE;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a088d71..9aa0cc8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -531,6 +531,9 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
             break;
         case LIBXL_VGA_INTERFACE_TYPE_QXL:
             break;
+        default:
+            LOG(ERROR, "Invalid emulated video card specified");
+            return ERROR_INVAL;
         }
 
         if (b_info->u.hvm.boot) {
@@ -970,6 +973,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 GCSPRINTF("qxl-vga,vram_size_mb=%"PRIu64",ram_size_mb=%"PRIu64,
                 (b_info->video_memkb/2/1024), (b_info->video_memkb/2/1024) ) );
             break;
+        default:
+            LOG(ERROR, "Invalid emulated video card specified");
+            return ERROR_INVAL;
         }
 
         if (b_info->u.hvm.boot) {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9ad7eba..92c95e5 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -204,11 +204,12 @@ libxl_shutdown_reason = Enumeration("shutdown_reason", [
     ], init_val = "LIBXL_SHUTDOWN_REASON_UNKNOWN")
 
 libxl_vga_interface_type = Enumeration("vga_interface_type", [
+    (0, "UNDEF"),
     (1, "CIRRUS"),
     (2, "STD"),
     (3, "NONE"),
     (4, "QXL"),
-    ], init_val = "LIBXL_VGA_INTERFACE_TYPE_CIRRUS")
+    ], init_val = "LIBXL_VGA_INTERFACE_TYPE_UNDEF")
 
 libxl_vendor_device = Enumeration("vendor_device", [
     (0, "NONE"),
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v3 2/5] libxl: introduce LIBXL_VGA_INTERFACE_TYPE_UNDEF

[Ian Campbell]

On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> And use it as the default value for the VGA kind. This allows libxl to
> set
> it to the default value later on when the domain type is known. For HVM
> guests the default value is LIBXL_VGA_INTERFACE_TYPE_CIRRUS while for
> HVMlite the default value is LIBXL_VGA_INTERFACE_TYPE_NONE.

Most enums in lixl_type.idl call this value "UNKNOWN" rather than "UNDEF".
We should be consistent.

You will also need to add the usual #define LIBXL_HAVE to libxl.h.

Everything else looks ok to me.

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_create.c  | 8 ++++++--
>  tools/libxl/libxl_dm.c      | 6 ++++++
>  tools/libxl/libxl_types.idl | 3 ++-
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e491d83..61a4001 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -206,8 +206,12 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
>          if (b_info->u.hvm.mmio_hole_memkb == LIBXL_MEMKB_DEFAULT)
>              b_info->u.hvm.mmio_hole_memkb = 0;
>  
> -        if (!b_info->u.hvm.vga.kind)
> -            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_UNDEF) {
> +            if (b_info->device_model_version ==
> LIBXL_DEVICE_MODEL_VERSION_NONE)
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_NONE;
> +            else
> +                b_info->u.hvm.vga.kind =
> LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +        }
>  
>          if (!b_info->u.hvm.hdtype)
>              b_info->u.hvm.hdtype = LIBXL_HDTYPE_IDE;
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a088d71..9aa0cc8 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -531,6 +531,9 @@ static int
> libxl__build_device_model_args_old(libxl__gc *gc,
>              break;
>          case LIBXL_VGA_INTERFACE_TYPE_QXL:
>              break;
> +        default:
> +            LOG(ERROR, "Invalid emulated video card specified");
> +            return ERROR_INVAL;
>          }
>  
>          if (b_info->u.hvm.boot) {
> @@ -970,6 +973,9 @@ static int
> libxl__build_device_model_args_new(libxl__gc *gc,
>                  GCSPRINTF("qxl-
> vga,vram_size_mb=%"PRIu64",ram_size_mb=%"PRIu64,
>                  (b_info->video_memkb/2/1024), (b_info-
> >video_memkb/2/1024) ) );
>              break;
> +        default:
> +            LOG(ERROR, "Invalid emulated video card specified");
> +            return ERROR_INVAL;
>          }
>  
>          if (b_info->u.hvm.boot) {
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9ad7eba..92c95e5 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -204,11 +204,12 @@ libxl_shutdown_reason =
> Enumeration("shutdown_reason", [
>      ], init_val = "LIBXL_SHUTDOWN_REASON_UNKNOWN")
>  
>  libxl_vga_interface_type = Enumeration("vga_interface_type", [
> +    (0, "UNDEF"),
>      (1, "CIRRUS"),
>      (2, "STD"),
>      (3, "NONE"),
>      (4, "QXL"),
> -    ], init_val = "LIBXL_VGA_INTERFACE_TYPE_CIRRUS")
> +    ], init_val = "LIBXL_VGA_INTERFACE_TYPE_UNDEF")
>  
>  libxl_vendor_device = Enumeration("vendor_device", [
>      (0, "NONE"),
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 3/5] libxl: initialise the build info before calling prepare_config

[Roger Pau Monne]

libxl__arch_domain_prepare_config has access to the libxl_domain_build_info
struct, so make sure it's properly initialised.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.
---
 tools/libxl/libxl_create.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 61a4001..ba4c9e8 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -892,6 +892,12 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
+    if (ret) {
+        LOG(ERROR, "Unable to set domain build info defaults");
+        goto error_out;
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOG(ERROR, "cannot make domain: %d", ret);
@@ -903,12 +909,6 @@ static void initiate_domain_create(libxl__egc *egc,
     dcs->guest_domid = domid;
     dcs->dmss.dm.guest_domid = 0; /* means we haven't spawned */
 
-    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
-    if (ret) {
-        LOG(ERROR, "Unable to set domain build info defaults");
-        goto error_out;
-    }
-
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
         (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
          libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v3 3/5] libxl: initialise the build info before calling prepare_config

[Ian Campbell]

On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> libxl__arch_domain_prepare_config has access to the
> libxl_domain_build_info
> struct, so make sure it's properly initialised.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.

I think this is worth moving into the actual commit message. Plus it would
be useful to clarify that while prepare_config has access to b_info it
doesn't touch it right now (but presumably you are about to make it do so).

If it does touch it then that is currently a bug which should be mentioned
in the commit message and tagged for backport etc.

I suspect the reason for the ordering today is that domain_make is intended
to consume create_info, not build_info. However that distinction seems to
me to be an artefact of a much older API structure which doesn't seem to
make much sense now (but we are stuck with it :-()

> ---
>  tools/libxl/libxl_create.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 61a4001..ba4c9e8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -892,6 +892,12 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
> +    if (ret) {
> +        LOG(ERROR, "Unable to set domain build info defaults");
> +        goto error_out;
> +    }
> +
>      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
>      if (ret) {
>          LOG(ERROR, "cannot make domain: %d", ret);
> @@ -903,12 +909,6 @@ static void initiate_domain_create(libxl__egc *egc,
>      dcs->guest_domid = domid;
>      dcs->dmss.dm.guest_domid = 0; /* means we haven't spawned */
>  
> -    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
> -    if (ret) {
> -        LOG(ERROR, "Unable to set domain build info defaults");
> -        goto error_out;
> -    }
> -
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
>           libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {

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

Re: [PATCH v3 3/5] libxl: initialise the build info before calling prepare_config

[Roger Pau Monné]

El 20/01/16 a les 13.46, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>> libxl__arch_domain_prepare_config has access to the
>> libxl_domain_build_info
>> struct, so make sure it's properly initialised.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>> NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.
> 
> I think this is worth moving into the actual commit message. Plus it would
> be useful to clarify that while prepare_config has access to b_info it
> doesn't touch it right now (but presumably you are about to make it do so).

Right, see 5/5.

> If it does touch it then that is currently a bug which should be mentioned
> in the commit message and tagged for backport etc.

No, it doesn't touch it ATM, so not a bug.

> I suspect the reason for the ordering today is that domain_make is intended
> to consume create_info, not build_info. However that distinction seems to
> me to be an artefact of a much older API structure which doesn't seem to
> make much sense now (but we are stuck with it :-()

That's right, this create_info vs build_info split seems quite
arbitrary, but I don't see myself shaving that yak right now (also
changing this is going to be a pain regarding API compatibility).

Roger.


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

Re: [PATCH v3 3/5] libxl: initialise the build info before calling prepare_config

[Ian Campbell]

On Wed, 2016-01-20 at 16:32 +0100, Roger Pau Monné wrote:
> > I suspect the reason for the ordering today is that domain_make is intended
> > to consume create_info, not build_info. However that distinction seems to
> > me to be an artefact of a much older API structure which doesn't seem to
> > make much sense now (but we are stuck with it :-()
> 
> That's right, this create_info vs build_info split seems quite
> arbitrary, but I don't see myself shaving that yak right now (also
> changing this is going to be a pain regarding API compatibility).

Yeah, we can't change that in any sane way I don't think.

Ian.

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

[PATCH v3 4/5] x86/PV: allow PV guests to have an emulated PIT

[Roger Pau Monne]

This fixes the fallout from the HVMlite series, that removed the emulated
PIT from PV(H) guests. Also, this patch forces the hardware domain to
always have an emulated PIT, regardless of whether the toolstack specified
one or not.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Force the emulated PIT to always be enabled for the hardware domain.
 - Change indentation of the valid set of emulation bitmaps check.
---
 xen/arch/x86/domain.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e70c125..78df5ae 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -542,8 +542,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                    d->domain_id, config->emulation_flags);
             return -EINVAL;
         }
-        if ( config->emulation_flags != 0 &&
-             (!is_hvm_domain(d) || config->emulation_flags != XEN_X86_EMU_ALL) )
+        if ( is_hardware_domain(d) )
+            config->emulation_flags |= XEN_X86_EMU_PIT;
+        if ( !is_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_PIT) :
+                                 (config->emulation_flags != XEN_X86_EMU_ALL &&
+                                  config->emulation_flags != 0))
         {
             printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
                    "with the current selection of emulators: %#x\n",
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v3 4/5] x86/PV: allow PV guests to have an emulated PIT

[Jan Beulich]

>>> On 20.01.16 at 12:57, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -542,8 +542,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                     d->domain_id, config->emulation_flags);
>              return -EINVAL;
>          }
> -        if ( config->emulation_flags != 0 &&
> -             (!is_hvm_domain(d) || config->emulation_flags != XEN_X86_EMU_ALL) )
> +        if ( is_hardware_domain(d) )
> +            config->emulation_flags |= XEN_X86_EMU_PIT;
> +        if ( !is_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_PIT) :
> +                                 (config->emulation_flags != XEN_X86_EMU_ALL &&
> +                                  config->emulation_flags != 0))
>          {
>              printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
>                     "with the current selection of emulators: %#x\n",

While you mentioned in the cover letter that this needs to be
committed together with patch 5, having them as separate
commits would still break bisectabaility afaict. Would it really
be so unreasonable to allow PV(H) DomU-s to not have an
emulated PIT, depending on (a future) tool stack gathered
per-domain setting? Converting the check above accordingly
would - afaict - at once break that dependency on patch 5.

Jan

[PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monne]

Allow enabling or disabling emulated devices from the libxl domain
configuration file. For HVM guests with a device model all the emulated
devices are enabled. For HVM guests without a device model no devices are
enabled by default, although they can be enabled using the options provided.
The arbiter of whether a combination is posible or not is always Xen, libxl
doesn't do any kind of check.

This set of options is also propagated inside of the libxl migration record
as part of the contents of the libxl_domain_build_info struct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5       | 39 +++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h         | 11 +++++++++++
 tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
 tools/libxl/libxl_types.idl |  6 ++++++
 tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
 tools/libxl/xl_cmdimpl.c    |  7 +++++++
 6 files changed, 111 insertions(+), 6 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 8899f75..46d4529 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1762,6 +1762,45 @@ See F<docs/misc/pci-device-reservations.txt> for more information.
 
 =back
 
+=head3 HVM without a device model options
+
+This options can be used to change the set of emulated devices provided
+to guests without a device model. Note that Xen might not support all
+possible combinations. By default HVM guests without a device model
+don't have any of them enabled.
+
+=over 4
+
+=item B<lapic=BOOLEAN>
+
+Enables or disables the Local APIC.
+
+=item B<ioapic=BOOLEAN>
+
+Enables or disables the IO APIC.
+
+=item B<rtc=BOOLEAN>
+
+Enables or disables the RTC.
+
+=item B<power_management=BOOLEAN>
+
+Enables or disables the ACPI power management timer and control interfaces.
+
+=item B<pic=BOOLEAN>
+
+Enables or disables the PIC.
+
+=item B<pit=BOOLEAN>
+
+Enables or disables the PIT.
+
+=item B<hpet=BOOLEAN>
+
+Enables or disables the HPET.
+
+=back
+
 =head2 Device-Model Options
 
 The following options control the selection of the device-model.  This
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7114491..fc4ff1d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -876,6 +876,17 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  */
 #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1
 
+/*
+ * LIBXL_HAVE_EMULATED_DEVS_OPTIONS
+ *
+ * In the case that LIBXL_HAVE_EMULATED_DEVS_OPTIONS is set libxl
+ * allows enabling or disabling emulated devices for HVM guests
+ * without a device model. The following fields are added to the
+ * hvm structure inside of libxl_domain_build_info: lapic, ioapic,
+ * rtc, power_management, pic, pit.
+ */
+#define LIBXL_HAVE_EMULATED_DEVS_OPTIONS 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ba4c9e8..8a76a6b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -295,13 +295,32 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4,            true);
         libxl_defbool_setdefault(&b_info->u.hvm.nx,                 true);
         libxl_defbool_setdefault(&b_info->u.hvm.viridian,           false);
-        libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
         libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
         libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
         libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
         libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
 
+        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
+            b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)
+        {
+            libxl_defbool_setdefault(&b_info->u.hvm.hpet,               false);
+            libxl_defbool_setdefault(&b_info->u.hvm.lapic,              false);
+            libxl_defbool_setdefault(&b_info->u.hvm.ioapic,             false);
+            libxl_defbool_setdefault(&b_info->u.hvm.rtc,                false);
+            libxl_defbool_setdefault(&b_info->u.hvm.power_management,   false);
+            libxl_defbool_setdefault(&b_info->u.hvm.pic,                false);
+            libxl_defbool_setdefault(&b_info->u.hvm.pit,                false);
+        } else {
+            libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
+            libxl_defbool_setdefault(&b_info->u.hvm.lapic,              true);
+            libxl_defbool_setdefault(&b_info->u.hvm.ioapic,             true);
+            libxl_defbool_setdefault(&b_info->u.hvm.rtc,                true);
+            libxl_defbool_setdefault(&b_info->u.hvm.power_management,   true);
+            libxl_defbool_setdefault(&b_info->u.hvm.pic,                true);
+            libxl_defbool_setdefault(&b_info->u.hvm.pit,                true);
+        }
+
         libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
         if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
             (b_info->u.hvm.spice.usbredirection > 0) ){
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 92c95e5..8a21cda 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -519,6 +519,12 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("serial_list",      libxl_string_list),
                                        ("rdm", libxl_rdm_reserve),
                                        ("rdm_mem_boundary_memkb", MemKB),
+                                       ("lapic",            libxl_defbool),
+                                       ("ioapic",           libxl_defbool),
+                                       ("rtc",              libxl_defbool),
+                                       ("power_management", libxl_defbool),
+                                       ("pic",              libxl_defbool),
+                                       ("pit",              libxl_defbool),
                                        ])),
                  ("pv", Struct(None, [("kernel", string),
                                       ("slack_memkb", MemKB),
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 46cfafb..92f25fd 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
                                       xc_domain_configuration_t *xc_config)
 {
+    struct libxl_domain_build_info *info = &d_config->b_info;
 
-    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
-        d_config->b_info.device_model_version !=
-        LIBXL_DEVICE_MODEL_VERSION_NONE) {
-        /* HVM domains with a device model. */
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
+        /* PV guests. */
+        xc_config->emulation_flags = XEN_X86_EMU_PIT;
+    } else if (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
+        /* HVM guests with a device model. */
         xc_config->emulation_flags = XEN_X86_EMU_ALL;
     } else {
-        /* PV or HVM domains without a device model. */
+        /* HVM guests without a device model. */
         xc_config->emulation_flags = 0;
+
+        if (libxl_defbool_val(info->u.hvm.lapic))
+            xc_config->emulation_flags |= XEN_X86_EMU_LAPIC;
+        if (libxl_defbool_val(info->u.hvm.ioapic))
+            xc_config->emulation_flags |= XEN_X86_EMU_IOAPIC;
+        if (libxl_defbool_val(info->u.hvm.rtc))
+            xc_config->emulation_flags |= XEN_X86_EMU_RTC;
+        if (libxl_defbool_val(info->u.hvm.power_management))
+            xc_config->emulation_flags |= XEN_X86_EMU_PM;
+        if (libxl_defbool_val(info->u.hvm.pic))
+            xc_config->emulation_flags |= XEN_X86_EMU_PIC;
+        if (libxl_defbool_val(info->u.hvm.pit))
+            xc_config->emulation_flags |= XEN_X86_EMU_PIT;
+        if (libxl_defbool_val(info->u.hvm.hpet))
+            xc_config->emulation_flags |= XEN_X86_EMU_HPET;
+
+        if (info->u.hvm.vga.kind != LIBXL_VGA_INTERFACE_TYPE_NONE)
+            xc_config->emulation_flags |= XEN_X86_EMU_VGA;
+
+        if (d_config->num_pcidevs != 0)
+            xc_config->emulation_flags |= XEN_X86_EMU_IOMMU;
     }
 
     return 0;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 25507c7..3654097 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1514,6 +1514,13 @@ static void parse_config_data(const char *config_source,
         xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
         xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
         xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
+        xlu_cfg_get_defbool(config, "lapic", &b_info->u.hvm.lapic, 0);
+        xlu_cfg_get_defbool(config, "ioapic", &b_info->u.hvm.ioapic, 0);
+        xlu_cfg_get_defbool(config, "rtc", &b_info->u.hvm.rtc, 0);
+        xlu_cfg_get_defbool(config, "power_management",
+                            &b_info->u.hvm.power_management, 0);
+        xlu_cfg_get_defbool(config, "pic", &b_info->u.hvm.pic, 0);
+        xlu_cfg_get_defbool(config, "pit", &b_info->u.hvm.pit, 0);
         xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
 
         switch (xlu_cfg_get_list(config, "viridian",
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Ian Campbell]

On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> Allow enabling or disabling emulated devices from the libxl domain
> configuration file. For HVM guests with a device model all the emulated
> devices are enabled. For HVM guests without a device model no devices are
> enabled by default, although they can be enabled using the options
> provided.
> The arbiter of whether a combination is posible or not is always Xen,
> libxl
> doesn't do any kind of check.
> 
> This set of options is also propagated inside of the libxl migration record
> as part of the contents of the libxl_domain_build_info struct.

... and this is the real motivation for this change, not actually allowing
users to control all this AIUI.

Did you check that the fields updated using libxl_defbool_setdefault are
actually updated in the JSON copy and therefore propagated to the other
side of a migration as specific values and not as "pick a default"? I think
we don't want these changing on migration. I think/hope all this was
automatically handled by the work Wei did in the last release cycle.

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  docs/man/xl.cfg.pod.5       | 39 +++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h         | 11 +++++++++++
>  tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
>  tools/libxl/libxl_types.idl |  6 ++++++
>  tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
>  tools/libxl/xl_cmdimpl.c    |  7 +++++++
>  6 files changed, 111 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 8899f75..46d4529 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1762,6 +1762,45 @@ See F<docs/misc/pci-device-reservations.txt> for
> more information.
>  
>  =back
>  
> +=head3 HVM without a device model options
> +
> +This options can be used to change the set of emulated devices provided

"These..."

> +to guests without a device model. Note that Xen might not support all
> +possible combinations. By default HVM guests without a device model
> +don't have any of them enabled.

... and for those with a device model? The title and text suggest these
options are invalid/ignored in that case, but the code does actually honour
what the user specified in this case.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 92c95e5..8a21cda 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -519,6 +519,12 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("serial_list",      libxl_string_list),
>                                         ("rdm", libxl_rdm_reserve),
>                                         ("rdm_mem_boundary_memkb", MemKB),
> +                                       ("lapic",            libxl_defbool),
> +                                       ("ioapic",           libxl_defbool),
> +                                       ("rtc",              libxl_defbool),
> +                                       ("power_management", libxl_defbool),
> +                                       ("pic",              libxl_defbool),
> +                                       ("pit",              libxl_defbool),

I wonder if these should go in a sub-struct. Although you might reasonably
argue that this is already such a dumping ground it doesn't matter...

>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 46cfafb..92f25fd 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>                                        libxl_domain_config *d_config,
>                                        xc_domain_configuration_t
> *xc_config)
>  {
> +    struct libxl_domain_build_info *info = &d_config->b_info;
>  
> -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> -        d_config->b_info.device_model_version !=
> -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
> -        /* HVM domains with a device model. */

So, I'm not 100% clear on why this check and the corresponding logic to set
xc_config->emulation_flags is not also sufficient for after migration.
Could you explain (and likely eventually add the rationale to the commit
message).

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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monné]

El 20/01/16 a les 14.01, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>> Allow enabling or disabling emulated devices from the libxl domain
>> configuration file. For HVM guests with a device model all the emulated
>> devices are enabled. For HVM guests without a device model no devices are
>> enabled by default, although they can be enabled using the options
>> provided.
>> The arbiter of whether a combination is posible or not is always Xen,
>> libxl
>> doesn't do any kind of check.
>>
>> This set of options is also propagated inside of the libxl migration record
>> as part of the contents of the libxl_domain_build_info struct.
> 
> ... and this is the real motivation for this change, not actually allowing
> users to control all this AIUI.
> 
> Did you check that the fields updated using libxl_defbool_setdefault are
> actually updated in the JSON copy and therefore propagated to the other
> side of a migration as specific values and not as "pick a default"? I think
> we don't want these changing on migration. I think/hope all this was
> automatically handled by the work Wei did in the last release cycle.

No, values populated by the {build/create}_info_setdefault functions are
not propagated, OTOH values manually set by the user in the config file
are indeed propagated. Do we have any guarantee that _setdefault is
always going to behave in the same way?

If we don't have that guarantee I think this is already a bug, and we
should call _setdefault before sending the domain info to the other end.
In fact I have a patch that does exactly that, but I'm unsure if it's
needed because I don't know the policy regarding default values in libxl.

>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  docs/man/xl.cfg.pod.5       | 39 +++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl.h         | 11 +++++++++++
>>  tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
>>  tools/libxl/libxl_types.idl |  6 ++++++
>>  tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
>>  tools/libxl/xl_cmdimpl.c    |  7 +++++++
>>  6 files changed, 111 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 8899f75..46d4529 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1762,6 +1762,45 @@ See F<docs/misc/pci-device-reservations.txt> for
>> more information.
>>  
>>  =back
>>  
>> +=head3 HVM without a device model options
>> +
>> +This options can be used to change the set of emulated devices provided
> 
> "These..."
> 
>> +to guests without a device model. Note that Xen might not support all
>> +possible combinations. By default HVM guests without a device model
>> +don't have any of them enabled.
> 
> ... and for those with a device model? The title and text suggest these
> options are invalid/ignored in that case, but the code does actually honour
> what the user specified in this case.

Right, I've clarified this by adding the following paragraph:

"It is important to notice that these options (except the hpet one) are
not available to HVM guests with a device model, and trying to set them
will cause xl to exit with an error."

I've also fixed up the code in libxl__domain_build_info_setdefault to
actually error out if a HVM guest with device model tries to set any of
them.

>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 92c95e5..8a21cda 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -519,6 +519,12 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                         ("serial_list",      libxl_string_list),
>>                                         ("rdm", libxl_rdm_reserve),
>>                                         ("rdm_mem_boundary_memkb", MemKB),
>> +                                       ("lapic",            libxl_defbool),
>> +                                       ("ioapic",           libxl_defbool),
>> +                                       ("rtc",              libxl_defbool),
>> +                                       ("power_management", libxl_defbool),
>> +                                       ("pic",              libxl_defbool),
>> +                                       ("pit",              libxl_defbool),
> 
> I wonder if these should go in a sub-struct. Although you might reasonably
> argue that this is already such a dumping ground it doesn't matter...

Right, TBH I saw that ARM added an arch_arm sub-struct, which sounds
fine and should have been done earlier. Now the hvm sub-struct is
already so x86 specific that, as you said, I don't think it matters much.

>>                                         ])),
>>                   ("pv", Struct(None, [("kernel", string),
>>                                        ("slack_memkb", MemKB),
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 46cfafb..92f25fd 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>                                        libxl_domain_config *d_config,
>>                                        xc_domain_configuration_t
>> *xc_config)
>>  {
>> +    struct libxl_domain_build_info *info = &d_config->b_info;
>>  
>> -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>> -        d_config->b_info.device_model_version !=
>> -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
>> -        /* HVM domains with a device model. */
> 
> So, I'm not 100% clear on why this check and the corresponding logic to set
> xc_config->emulation_flags is not also sufficient for after migration.
> Could you explain (and likely eventually add the rationale to the commit
> message).

As I understand this, we want to avoid having two different places where
the policy (ie: the set of enabled devices) is enforced.

With the current code, libxl basically limits the set of allowed masks
to what it knows. After the change libxl just becomes a proxy for
transmitting what the user has selected to Xen, and Xen either accepts
or refuses it, basically making Xen the only arbiter that decides which
emulated devices get enabled or not. This means that if we want to make
more emulated devices available to the guest in the future no libxl
changes will be required.

It also means that HVMlite guests created with current Xen will be
capable of migrating to newer version of Xen, that might have a
different default policy. For example in the future we might want to
enable the lapic by default, so if a guest is created with the current
Xen version it doesn't get a lapic at all, and then when migrated to
newer versions a lapic would magically appear after the migration, which
is not desired.

Roger.


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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Ian Campbell]

On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monné wrote:
> El 20/01/16 a les 14.01, Ian Campbell ha escrit:
> > On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> > > Allow enabling or disabling emulated devices from the libxl domain
> > > configuration file. For HVM guests with a device model all the
> > > emulated
> > > devices are enabled. For HVM guests without a device model no devices
> > > are
> > > enabled by default, although they can be enabled using the options
> > > provided.
> > > The arbiter of whether a combination is posible or not is always Xen,
> > > libxl
> > > doesn't do any kind of check.
> > > 
> > > This set of options is also propagated inside of the libxl migration
> > > record
> > > as part of the contents of the libxl_domain_build_info struct.
> > 
> > ... and this is the real motivation for this change, not actually
> > allowing
> > users to control all this AIUI.
> > 
> > Did you check that the fields updated using libxl_defbool_setdefault
> > are
> > actually updated in the JSON copy and therefore propagated to the other
> > side of a migration as specific values and not as "pick a default"? I
> > think
> > we don't want these changing on migration. I think/hope all this was
> > automatically handled by the work Wei did in the last release cycle.
> 
> No, values populated by the {build/create}_info_setdefault functions are
> not propagated, OTOH values manually set by the user in the config file
> are indeed propagated. Do we have any guarantee that _setdefault is
> always going to behave in the same way?

No, part of the purpose of defbool and the other "do the default" values is
that we can evolve things over time.

> If we don't have that guarantee I think this is already a bug, and we
> should call _setdefault before sending the domain info to the other end.
> In fact I have a patch that does exactly that, but I'm unsure if it's
> needed because I don't know the policy regarding default values in libxl.

Wei, isn't this (turning the defaults into concrete values) supposed to be
taken care of by the JSON mangling which you added?

> 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  docs/man/xl.cfg.pod.5       | 39
> > > +++++++++++++++++++++++++++++++++++++++
> > >  tools/libxl/libxl.h         | 11 +++++++++++
> > >  tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
> > >  tools/libxl/libxl_types.idl |  6 ++++++
> > >  tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
> > >  tools/libxl/xl_cmdimpl.c    |  7 +++++++
> > >  6 files changed, 111 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index 8899f75..46d4529 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -1762,6 +1762,45 @@ See F<docs/misc/pci-device-reservations.txt>
> > > for
> > > more information.
> > >  
> > >  =back
> > >  
> > > +=head3 HVM without a device model options
> > > +
> > > +This options can be used to change the set of emulated devices
> > > provided
> > 
> > "These..."
> > 
> > > +to guests without a device model. Note that Xen might not support
> > > all
> > > +possible combinations. By default HVM guests without a device model
> > > +don't have any of them enabled.
> > 
> > ... and for those with a device model? The title and text suggest these
> > options are invalid/ignored in that case, but the code does actually
> > honour
> > what the user specified in this case.
> 
> Right, I've clarified this by adding the following paragraph:
> 
> "It is important to notice that these options (except the hpet one) are
> not available to HVM guests with a device model, and trying to set them
> will cause xl to exit with an error."
> 
> I've also fixed up the code in libxl__domain_build_info_setdefault to
> actually error out if a HVM guest with device model tries to set any of
> them.
> 
> > > diff --git a/tools/libxl/libxl_types.idl
> > > b/tools/libxl/libxl_types.idl
> > > index 92c95e5..8a21cda 100644
> > > --- a/tools/libxl/libxl_types.idl
> > > +++ b/tools/libxl/libxl_types.idl
> > > @@ -519,6 +519,12 @@ libxl_domain_build_info =
> > > Struct("domain_build_info",[
> > >                                         ("serial_list",      libxl_st
> > > ring_list),
> > >                                         ("rdm", libxl_rdm_reserve),
> > >                                         ("rdm_mem_boundary_memkb",
> > > MemKB),
> > > +                                       ("lapic",            libxl_de
> > > fbool),
> > > +                                       ("ioapic",           libxl_de
> > > fbool),
> > > +                                       ("rtc",              libxl_de
> > > fbool),
> > > +                                       ("power_management",
> > > libxl_defbool),
> > > +                                       ("pic",              libxl_de
> > > fbool),
> > > +                                       ("pit",              libxl_de
> > > fbool),
> > 
> > I wonder if these should go in a sub-struct. Although you might
> > reasonably
> > argue that this is already such a dumping ground it doesn't matter...
> 
> Right, TBH I saw that ARM added an arch_arm sub-struct, which sounds
> fine and should have been done earlier. Now the hvm sub-struct is
> already so x86 specific that, as you said, I don't think it matters much.

I meant a substruct of hvm (i.e. vhm.emul_opts), but your point is also
valid.

> > >                                         ])),
> > >                   ("pv", Struct(None, [("kernel", string),
> > >                                        ("slack_memkb", MemKB),
> > > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> > > index 46cfafb..92f25fd 100644
> > > --- a/tools/libxl/libxl_x86.c
> > > +++ b/tools/libxl/libxl_x86.c
> > > @@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > >                                        libxl_domain_config *d_config,
> > >                                        xc_domain_configuration_t
> > > *xc_config)
> > >  {
> > > +    struct libxl_domain_build_info *info = &d_config->b_info;
> > >  
> > > -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> > > -        d_config->b_info.device_model_version !=
> > > -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
> > > -        /* HVM domains with a device model. */
> > 
> > So, I'm not 100% clear on why this check and the corresponding logic to set
> > xc_config->emulation_flags is not also sufficient for after migration.
> > Could you explain (and likely eventually add the rationale to the commit
> > message).
> 
> As I understand this, we want to avoid having two different places where
> the policy (ie: the set of enabled devices) is enforced.

But it must _always_ be enforced by Xen as the last resort.

> With the current code, libxl basically limits the set of allowed masks
> to what it knows. After the change libxl just becomes a proxy for
> transmitting what the user has selected to Xen, and Xen either accepts
> or refuses it, basically making Xen the only arbiter that decides which
> emulated devices get enabled or not. This means that if we want to make
> more emulated devices available to the guest in the future no libxl
> changes will be required.

We would need to add a new defbool for the new feature.

> It also means that HVMlite guests created with current Xen will be
> capable of migrating to newer version of Xen, that might have a
> different default policy. For example in the future we might want to
> enable the lapic by default, so if a guest is created with the current
> Xen version it doesn't get a lapic at all, and then when migrated to
> newer versions a lapic would magically appear after the migration, which
> is not desired.

... and the reason these details can't be propagated via the Xen blob is
that this emul stuff needs to be set exactly once at domain create time I
suppose? Changing it to be later binding is considered to be too hard/too
big a yak?

Even with the set of devices set at domain creation time Xen needs to take
care when reading its blob, and not fall apart (from a security PoV, it's
allowed to fail the state load) when presented with a save record relating
to something which is supposedly disabled. Has this been checked?

Ian.

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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monné]

El 21/01/16 a les 10.39, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monné wrote:
>> El 20/01/16 a les 14.01, Ian Campbell ha escrit:
>>> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>>>> Allow enabling or disabling emulated devices from the libxl domain
>>>> configuration file. For HVM guests with a device model all the
>>>> emulated
>>>> devices are enabled. For HVM guests without a device model no devices
>>>> are
>>>> enabled by default, although they can be enabled using the options
>>>> provided.
>>>> The arbiter of whether a combination is posible or not is always Xen,
>>>> libxl
>>>> doesn't do any kind of check.
>>>>
>>>> This set of options is also propagated inside of the libxl migration
>>>> record
>>>> as part of the contents of the libxl_domain_build_info struct.
>>>
>>> ... and this is the real motivation for this change, not actually
>>> allowing
>>> users to control all this AIUI.
>>>
>>> Did you check that the fields updated using libxl_defbool_setdefault
>>> are
>>> actually updated in the JSON copy and therefore propagated to the other
>>> side of a migration as specific values and not as "pick a default"? I
>>> think
>>> we don't want these changing on migration. I think/hope all this was
>>> automatically handled by the work Wei did in the last release cycle.
>>
>> No, values populated by the {build/create}_info_setdefault functions are
>> not propagated, OTOH values manually set by the user in the config file
>> are indeed propagated. Do we have any guarantee that _setdefault is
>> always going to behave in the same way?
> 
> No, part of the purpose of defbool and the other "do the default" values is
> that we can evolve things over time.
>
>> If we don't have that guarantee I think this is already a bug, and we
>> should call _setdefault before sending the domain info to the other end.
>> In fact I have a patch that does exactly that, but I'm unsure if it's
>> needed because I don't know the policy regarding default values in libxl.
> 
> Wei, isn't this (turning the defaults into concrete values) supposed to be
> taken care of by the JSON mangling which you added?

Heh, I think you mean the JSON mangling added by Wei. In order to 
propagate the values filled by default in libxl_domain_config I had to 
add the following patch, which basically calls the _setdefault 
functions before converting the domain_config into JSON. I'm planning 
to make it part of this series in the next iteration:

---
commit b1b2cea4b61ce9bd05797d3dc5ff0f5fffccfd05
Author: Roger Pau Monne <roger.pau@citrix.com>
Date:   Wed Jan 20 19:06:50 2016 +0100

    libxl: introduce libxl_domain_info_setdefault to the public API
    
    The newly introduced function populates the libxl_domain_config with their
    default values, just like it's done during domain creation.
    
    This is needed so the libxl_domain_config sent to the restore side during
    migration is accurate, since default values might change between libxl
    versions.
    
    Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
    ---
    ---
    NB: I'm unsure whether this is a bug or not. IMHO I think it is, because
    there's no guarantee that the default values will stay the same between
    libxl versions, so a domain created with an old libxl version might see
    differences when migrated to a newer libxl version.

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 157f07c..70bb6e1 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -886,6 +886,15 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  */
 #define LIBXL_HAVE_VGA_INTERFACE_TYPE_UNKNOWN 1
 
+/*
+ * LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT
+ *
+ * In the case that LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT is set libxl
+ * provides the libxl_domain_info_setdefault function that can be used
+ * to set the libxl_domain_config fields to their default values.
+ */
+#define LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1202,6 +1211,9 @@ int libxl_domain_soft_reset(libxl_ctx *ctx,
 void libxl_domain_config_init(libxl_domain_config *d_config);
 void libxl_domain_config_dispose(libxl_domain_config *d_config);
 
+/* Fill the libxl_domain_config struct with their default values. */
+int libxl_domain_info_setdefault(libxl_ctx *ctx, libxl_domain_config *d_config);
+
 /*
  * Retrieve domain configuration and filled it in d_config. The
  * returned configuration can be used to rebuild a domain. It only
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c7700a7..c988c2e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -886,17 +886,10 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
-    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
-    if (ret) {
-        LOG(ERROR, "Unable to set domain create info defaults");
-        goto error_out;
-    }
-
-    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
-    if (ret) {
-        LOG(ERROR, "Unable to set domain build info defaults");
+    ret = libxl_domain_info_setdefault(CTX, d_config);
+    if (ret)
+        /* libxl_domain_info_setdefault already logs errors. */
         goto error_out;
-    }
 
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
@@ -1804,6 +1797,26 @@ int libxl_domain_soft_reset(libxl_ctx *ctx,
                                 aop_console_how);
 }
 
+int libxl_domain_info_setdefault(libxl_ctx *ctx, libxl_domain_config *d_config)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
+    if (rc) {
+        LOG(ERROR, "Unable to set domain create info defaults");
+        return rc;
+    }
+    rc = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
+    if (rc) {
+        LOG(ERROR, "Unable to set domain build info defaults");
+        return rc;
+    }
+
+    GC_FREE;
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 25507c7..0454efa 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4044,6 +4044,14 @@ static void save_domain_core_begin(uint32_t domid,
         }
     }
 
+#ifdef LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT
+    rc = libxl_domain_info_setdefault(ctx, &d_config);
+    if (rc) {
+        fprintf(stderr, "unable to set default configuration values\n");
+        exit(2);
+    }
+#endif
+
     config_c = libxl_domain_config_to_json(ctx, &d_config);
     if (!config_c) {
         fprintf(stderr, "unable to convert config file to JSON\n");




>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> ---
>>>>  docs/man/xl.cfg.pod.5       | 39
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>  tools/libxl/libxl.h         | 11 +++++++++++
>>>>  tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
>>>>  tools/libxl/libxl_types.idl |  6 ++++++
>>>>  tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
>>>>  tools/libxl/xl_cmdimpl.c    |  7 +++++++
>>>>  6 files changed, 111 insertions(+), 6 deletions(-)
>>>> diff --git a/tools/libxl/libxl_types.idl
>>>> b/tools/libxl/libxl_types.idl
>>>> index 92c95e5..8a21cda 100644
>>>> --- a/tools/libxl/libxl_types.idl
>>>> +++ b/tools/libxl/libxl_types.idl
>>>> @@ -519,6 +519,12 @@ libxl_domain_build_info =
>>>> Struct("domain_build_info",[
>>>>                                         ("serial_list",      libxl_st
>>>> ring_list),
>>>>                                         ("rdm", libxl_rdm_reserve),
>>>>                                         ("rdm_mem_boundary_memkb",
>>>> MemKB),
>>>> +                                       ("lapic",            libxl_de
>>>> fbool),
>>>> +                                       ("ioapic",           libxl_de
>>>> fbool),
>>>> +                                       ("rtc",              libxl_de
>>>> fbool),
>>>> +                                       ("power_management",
>>>> libxl_defbool),
>>>> +                                       ("pic",              libxl_de
>>>> fbool),
>>>> +                                       ("pit",              libxl_de
>>>> fbool),
>>>
>>> I wonder if these should go in a sub-struct. Although you might
>>> reasonably
>>> argue that this is already such a dumping ground it doesn't matter...
>>
>> Right, TBH I saw that ARM added an arch_arm sub-struct, which sounds
>> fine and should have been done earlier. Now the hvm sub-struct is
>> already so x86 specific that, as you said, I don't think it matters much.
> 
> I meant a substruct of hvm (i.e. vhm.emul_opts), but your point is also
> valid.

I would probably place them in the hvm struct, since it already 
contains a hpet defbool.

>>>>                                         ])),
>>>>                   ("pv", Struct(None, [("kernel", string),
>>>>                                        ("slack_memkb", MemKB),
>>>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>>>> index 46cfafb..92f25fd 100644
>>>> --- a/tools/libxl/libxl_x86.c
>>>> +++ b/tools/libxl/libxl_x86.c
>>>> @@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>>                                        libxl_domain_config *d_config,
>>>>                                        xc_domain_configuration_t
>>>> *xc_config)
>>>>  {
>>>> +    struct libxl_domain_build_info *info = &d_config->b_info;
>>>>  
>>>> -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>>>> -        d_config->b_info.device_model_version !=
>>>> -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
>>>> -        /* HVM domains with a device model. */
>>>
>>> So, I'm not 100% clear on why this check and the corresponding logic to set
>>> xc_config->emulation_flags is not also sufficient for after migration.
>>> Could you explain (and likely eventually add the rationale to the commit
>>> message).
>>
>> As I understand this, we want to avoid having two different places where
>> the policy (ie: the set of enabled devices) is enforced.
> 
> But it must _always_ be enforced by Xen as the last resort.

Yes, that's already done, Xen always has the last word on what gets 
enabled or not.

> 
>> With the current code, libxl basically limits the set of allowed masks
>> to what it knows. After the change libxl just becomes a proxy for
>> transmitting what the user has selected to Xen, and Xen either accepts
>> or refuses it, basically making Xen the only arbiter that decides which
>> emulated devices get enabled or not. This means that if we want to make
>> more emulated devices available to the guest in the future no libxl
>> changes will be required.
> 
> We would need to add a new defbool for the new feature.

Yes, but I was thinking more in the direction of enabling them, rather 
than adding new ones.

> 
>> It also means that HVMlite guests created with current Xen will be
>> capable of migrating to newer version of Xen, that might have a
>> different default policy. For example in the future we might want to
>> enable the lapic by default, so if a guest is created with the current
>> Xen version it doesn't get a lapic at all, and then when migrated to
>> newer versions a lapic would magically appear after the migration, which
>> is not desired.
> 
> ... and the reason these details can't be propagated via the Xen blob is
> that this emul stuff needs to be set exactly once at domain create time I
> suppose? Changing it to be later binding is considered to be too hard/too
> big a yak?

Right, emulated devices are initialised as part of the 
XEN_DOMCTL_createdomain hypercall. Allowing them to be added later on 
and introducing a kind of intermediate domain building phase where only 
a certain set of hypercalls are valid is a possibility that Andrew 
already pointed out, however this seems like a very big project.

> Even with the set of devices set at domain creation time Xen needs to take
> care when reading its blob, and not fall apart (from a security PoV, it's
> allowed to fail the state load) when presented with a save record relating
> to something which is supposedly disabled. Has this been checked?

Yes, trying to load a state into a disable device will result in 
-ENODEV.

Roger.

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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Ian Campbell]

On Thu, 2016-01-21 at 11:01 +0100, Roger Pau Monné wrote:
> El 21/01/16 a les 10.39, Ian Campbell ha escrit:
> > > If we don't have that guarantee I think this is already a bug, and we
> > > should call _setdefault before sending the domain info to the other end.
> > > In fact I have a patch that does exactly that, but I'm unsure if it's
> > > needed because I don't know the policy regarding default values in libxl.
> > 
> > Wei, isn't this (turning the defaults into concrete values) supposed to be
> > taken care of by the JSON mangling which you added?
> 
> Heh, I think you mean the JSON mangling added by Wei.

Correct.

>  In order to 
> propagate the values filled by default in libxl_domain_config I had to 
> add the following patch, which basically calls the _setdefault 
> functions before converting the domain_config into JSON. I'm planning 
> to make it part of this series in the next iteration:

I'll let Wei comment on why this isn't already done.

> > > With the current code, libxl basically limits the set of allowed masks
> > > to what it knows. After the change libxl just becomes a proxy for
> > > transmitting what the user has selected to Xen, and Xen either accepts
> > > or refuses it, basically making Xen the only arbiter that decides which
> > > emulated devices get enabled or not. This means that if we want to make
> > > more emulated devices available to the guest in the future no libxl
> > > changes will be required.
> > 
> > We would need to add a new defbool for the new feature.
> 
> Yes, but I was thinking more in the direction of enabling them, rather 
> than adding new ones.

Which would then require changing the defbool_set_default in libxl after
this change, so you do still need to change libxl.

> > > It also means that HVMlite guests created with current Xen will be
> > > capable of migrating to newer version of Xen, that might have a
> > > different default policy. For example in the future we might want to
> > > enable the lapic by default, so if a guest is created with the current
> > > Xen version it doesn't get a lapic at all, and then when migrated to
> > > newer versions a lapic would magically appear after the migration, which
> > > is not desired.
> > 
> > ... and the reason these details can't be propagated via the Xen blob is
> > that this emul stuff needs to be set exactly once at domain create time I
> > suppose? Changing it to be later binding is considered to be too hard/too
> > big a yak?
> 
> Right, emulated devices are initialised as part of the 
> XEN_DOMCTL_createdomain hypercall. Allowing them to be added later on 
> and introducing a kind of intermediate domain building phase where only 
> a certain set of hypercalls are valid is a possibility that Andrew 
> already pointed out, however this seems like a very big project.

This seems like the right approach to me, but I appreciate you not wanting
to tackle this here and now.

Would it be possible to set the set of "potential" emulated devices at
create time and only "commit" to it after the save state is loaded? That
would essentially mean init-all, load state, de-init those which didn't get
any state loaded? Not as nice as doing it with the split of hypercall
availability, but might be more achievable, since you already have the de-
init code for domain teardown time.

In any case, whatever is chosen as the solution the commit message needs to
go into a fair amount of detail as to why we picked that way of doing
things, particularly if it is a compromise vs doing it properly.

This is important so we can answer "why did we do it this way" in 2 years
time.

> > Even with the set of devices set at domain creation time Xen needs to take
> > care when reading its blob, and not fall apart (from a security PoV, it's
> > allowed to fail the state load) when presented with a save record relating
> > to something which is supposedly disabled. Has this been checked?
> 
> Yes, trying to load a state into a disable device will result in 
> -ENODEV.

Grand.

Ian.

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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monné]

El 21/01/16 a les 11.29, Ian Campbell ha escrit:
> On Thu, 2016-01-21 at 11:01 +0100, Roger Pau Monné wrote:
>> El 21/01/16 a les 10.39, Ian Campbell ha escrit:
>>>> It also means that HVMlite guests created with current Xen will be
>>>> capable of migrating to newer version of Xen, that might have a
>>>> different default policy. For example in the future we might want to
>>>> enable the lapic by default, so if a guest is created with the current
>>>> Xen version it doesn't get a lapic at all, and then when migrated to
>>>> newer versions a lapic would magically appear after the migration, which
>>>> is not desired.
>>>
>>> ... and the reason these details can't be propagated via the Xen blob is
>>> that this emul stuff needs to be set exactly once at domain create time I
>>> suppose? Changing it to be later binding is considered to be too hard/too
>>> big a yak?
>>
>> Right, emulated devices are initialised as part of the 
>> XEN_DOMCTL_createdomain hypercall. Allowing them to be added later on 
>> and introducing a kind of intermediate domain building phase where only 
>> a certain set of hypercalls are valid is a possibility that Andrew 
>> already pointed out, however this seems like a very big project.
> 
> This seems like the right approach to me, but I appreciate you not wanting
> to tackle this here and now.
> 
> Would it be possible to set the set of "potential" emulated devices at
> create time and only "commit" to it after the save state is loaded? That
> would essentially mean init-all, load state, de-init those which didn't get
> any state loaded? Not as nice as doing it with the split of hypercall
> availability, but might be more achievable, since you already have the de-
> init code for domain teardown time.

Sadly there are devices that AFAICT don't restore any state (like the
VGA), which means a more complex mechanism is needed and we cannot rely
exclusively on restores in order to decide if a device is present or not.

IMHO the current approach is not that bad, and I think we should be able
to migrate to a better one without problems. If in the future we want to
do something like what you describe above (setting the set of emulated
devices based on the Xen context restored), the extra information in the
libxl JSON stream is certainly not going to hurt us. At worst we could
always check that the libxl JSON information matches with what the
hypervisor context contains for extra safety.

> In any case, whatever is chosen as the solution the commit message needs to
> go into a fair amount of detail as to why we picked that way of doing
> things, particularly if it is a compromise vs doing it properly.
> 
> This is important so we can answer "why did we do it this way" in 2 years
> time.

Right, thanks, I will update the commit message with the outcome of this
discussion.

Roger.


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

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Wei Liu]

On Thu, Jan 21, 2016 at 11:01:43AM +0100, Roger Pau Monné wrote:
> El 21/01/16 a les 10.39, Ian Campbell ha escrit:
> > On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monné wrote:
> >> El 20/01/16 a les 14.01, Ian Campbell ha escrit:
> >>> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> >>>> Allow enabling or disabling emulated devices from the libxl domain
> >>>> configuration file. For HVM guests with a device model all the
> >>>> emulated
> >>>> devices are enabled. For HVM guests without a device model no devices
> >>>> are
> >>>> enabled by default, although they can be enabled using the options
> >>>> provided.
> >>>> The arbiter of whether a combination is posible or not is always Xen,
> >>>> libxl
> >>>> doesn't do any kind of check.
> >>>>
> >>>> This set of options is also propagated inside of the libxl migration
> >>>> record
> >>>> as part of the contents of the libxl_domain_build_info struct.
> >>>
> >>> ... and this is the real motivation for this change, not actually
> >>> allowing
> >>> users to control all this AIUI.
> >>>
> >>> Did you check that the fields updated using libxl_defbool_setdefault
> >>> are
> >>> actually updated in the JSON copy and therefore propagated to the other
> >>> side of a migration as specific values and not as "pick a default"? I
> >>> think
> >>> we don't want these changing on migration. I think/hope all this was
> >>> automatically handled by the work Wei did in the last release cycle.
> >>
> >> No, values populated by the {build/create}_info_setdefault functions are
> >> not propagated, OTOH values manually set by the user in the config file
> >> are indeed propagated. Do we have any guarantee that _setdefault is
> >> always going to behave in the same way?
> > 
> > No, part of the purpose of defbool and the other "do the default" values is
> > that we can evolve things over time.
> >
> >> If we don't have that guarantee I think this is already a bug, and we
> >> should call _setdefault before sending the domain info to the other end.
> >> In fact I have a patch that does exactly that, but I'm unsure if it's
> >> needed because I don't know the policy regarding default values in libxl.
> > 
> > Wei, isn't this (turning the defaults into concrete values) supposed to be
> > taken care of by the JSON mangling which you added?
> 
> Heh, I think you mean the JSON mangling added by Wei. In order to 
> propagate the values filled by default in libxl_domain_config I had to 
> add the following patch, which basically calls the _setdefault 
> functions before converting the domain_config into JSON. I'm planning 
> to make it part of this series in the next iteration:

The requirement of recording decision made in libxl and pass that to the
receiving end is not new. We had the same problem for uuid, disk and
some other things.

The first way of doing it is to update JSON before it is sent -- see
libxl.c:libxl_retrieve_domain_configuration. It uses the stashed JSON
file as template and pull in various bits from hypervisor and xenstore.
Your need of recording what emulated devices are available fits here.
You just need to provide a way to retrieve those bits in that function.

Another way of doing it is to update the stashed JSON template before it
is saved. See libxl_internal.c:libxl__update_domain_configuration. I
think this might be easier than the first way of doing it.

You should not export the _setdefault function to xl because it is a
layer violation.

Hope this clarify things.

Wei.

Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monné]

El 21/01/16 a les 12.31, Wei Liu ha escrit:
> On Thu, Jan 21, 2016 at 11:01:43AM +0100, Roger Pau Monné wrote:
>> El 21/01/16 a les 10.39, Ian Campbell ha escrit:
>>> On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monné wrote:
>>>> El 20/01/16 a les 14.01, Ian Campbell ha escrit:
>>>>> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>>>>>> Allow enabling or disabling emulated devices from the libxl domain
>>>>>> configuration file. For HVM guests with a device model all the
>>>>>> emulated
>>>>>> devices are enabled. For HVM guests without a device model no devices
>>>>>> are
>>>>>> enabled by default, although they can be enabled using the options
>>>>>> provided.
>>>>>> The arbiter of whether a combination is posible or not is always Xen,
>>>>>> libxl
>>>>>> doesn't do any kind of check.
>>>>>>
>>>>>> This set of options is also propagated inside of the libxl migration
>>>>>> record
>>>>>> as part of the contents of the libxl_domain_build_info struct.
>>>>>
>>>>> ... and this is the real motivation for this change, not actually
>>>>> allowing
>>>>> users to control all this AIUI.
>>>>>
>>>>> Did you check that the fields updated using libxl_defbool_setdefault
>>>>> are
>>>>> actually updated in the JSON copy and therefore propagated to the other
>>>>> side of a migration as specific values and not as "pick a default"? I
>>>>> think
>>>>> we don't want these changing on migration. I think/hope all this was
>>>>> automatically handled by the work Wei did in the last release cycle.
>>>>
>>>> No, values populated by the {build/create}_info_setdefault functions are
>>>> not propagated, OTOH values manually set by the user in the config file
>>>> are indeed propagated. Do we have any guarantee that _setdefault is
>>>> always going to behave in the same way?
>>>
>>> No, part of the purpose of defbool and the other "do the default" values is
>>> that we can evolve things over time.
>>>
>>>> If we don't have that guarantee I think this is already a bug, and we
>>>> should call _setdefault before sending the domain info to the other end.
>>>> In fact I have a patch that does exactly that, but I'm unsure if it's
>>>> needed because I don't know the policy regarding default values in libxl.
>>>
>>> Wei, isn't this (turning the defaults into concrete values) supposed to be
>>> taken care of by the JSON mangling which you added?
>>
>> Heh, I think you mean the JSON mangling added by Wei. In order to 
>> propagate the values filled by default in libxl_domain_config I had to 
>> add the following patch, which basically calls the _setdefault 
>> functions before converting the domain_config into JSON. I'm planning 
>> to make it part of this series in the next iteration:
> 
> The requirement of recording decision made in libxl and pass that to the
> receiving end is not new. We had the same problem for uuid, disk and
> some other things.
> 
> The first way of doing it is to update JSON before it is sent -- see
> libxl.c:libxl_retrieve_domain_configuration. It uses the stashed JSON
> file as template and pull in various bits from hypervisor and xenstore.
> Your need of recording what emulated devices are available fits here.
> You just need to provide a way to retrieve those bits in that function.
> 
> Another way of doing it is to update the stashed JSON template before it
> is saved. See libxl_internal.c:libxl__update_domain_configuration. I
> think this might be easier than the first way of doing it.
> 
> You should not export the _setdefault function to xl because it is a
> layer violation.
> 
> Hope this clarify things.

Hello,

Yes, thank you very much, it has indeed clarified things. I've
implemented it inside of libxl__update_domain_configuration without issues.

Roger.