[PATCH v2 0/4] HVMlite: minor fixes and Dom0 preparatory patches

[PATCH v2 0/4] HVMlite: minor fixes and Dom0 preparatory patches

[Roger Pau Monne]

Hello,

This series contains some bug fixes for HVMlite DomU and a preparatory 
patch for HVMlite Dom0 support (2/4).

Thanks, Roger.

[PATCH v2 1/4] xen/elfnotes: check phys_entry against UNSET_ADDR32

[Roger Pau Monne]

And introduce UNSET_ADDR32.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Fix commit title.
---
 tools/libxc/xc_dom_elfloader.c     | 2 +-
 xen/common/libelf/libelf-dominfo.c | 1 +
 xen/include/xen/libelf.h           | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 2ae575e..5039f3f 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -57,7 +57,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
     uint64_t machine = elf_uval(elf, elf->ehdr, e_machine);
 
     if ( dom->container_type == XC_DOM_HVM_CONTAINER &&
-         dom->parms.phys_entry != UNSET_ADDR )
+         dom->parms.phys_entry != UNSET_ADDR32 )
         return "hvm-3.0-x86_32";
 
     switch ( machine )
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 02d6cfb..ec69449 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -503,6 +503,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
     parms->virt_hv_start_low = UNSET_ADDR;
     parms->p2m_base = UNSET_ADDR;
     parms->elf_paddr_offset = UNSET_ADDR;
+    parms->phys_entry = UNSET_ADDR32;
 
     /* Find and parse elf notes. */
     count = elf_phdr_count(elf);
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 6da4cc0..95b5370 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -386,6 +386,7 @@ elf_errorstatus elf_reloc(struct elf_binary *elf);
 /* xc_libelf_dominfo.c                                                      */
 
 #define UNSET_ADDR          ((uint64_t)-1)
+#define UNSET_ADDR32        ((uint32_t)-1)
 
 enum xen_elfnote_type {
     XEN_ENT_NONE = 0,
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v2 2/4] 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 v2 3/4] x86/hvm: don't set the BSP as initialised in hvm_vcpu_initialise

[Roger Pau Monne]

The BSP will be marked as initialised after hvm_load_cpu_ctxt has loaded the
initial state, which is called from the toolstack during domain creation.

Previous to my HVMlite series HVM guests were started without setting any
explicit CPU state (in fact we placed that horrible jmp at 0x0, because the
IP was by default set to 0x0). This is no longer true, and now HVM guests
require that a proper CPU context is loaded before starting. This change
helps enforce this policy.

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 v1:
 - Expand commit description.
---
 xen/arch/x86/hvm/hvm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 787b7de..05c3ca1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2474,10 +2474,6 @@ int hvm_vcpu_initialise(struct vcpu *v)
  
         /* Init guest TSC to start from zero. */
         hvm_set_guest_tsc(v, 0);
-
-        /* Can start up without SIPI-SIPI or setvcpucontext domctl. */
-        v->is_initialised = 1;
-        clear_bit(_VPF_down, &v->pause_flags);
     }
 
     return 0;
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v2 4/4] x86/PV: enable the emulated PIT

[Roger Pau Monne]

The HVMlite series removed the initialization of the emulated PIT for PV
guests, this patch re-enables it.

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>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
NB: Since it's not clear why an emulated PIT is needed, it won't be enabled
by default for HVMlite guests until we can figure out if/why it's needed.
---
Changes since v1:
 - New in this version.
---
 tools/libxl/libxl_x86.c | 8 +++++++-
 xen/arch/x86/domain.c   | 5 +++--
 xen/arch/x86/setup.c    | 4 +++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 46cfafb..7f47cc5 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -13,8 +13,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         LIBXL_DEVICE_MODEL_VERSION_NONE) {
         /* HVM domains with a device model. */
         xc_config->emulation_flags = XEN_X86_EMU_ALL;
+    } else if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
+        /* PV domains. */
+        xc_config->emulation_flags = XEN_X86_EMU_PIT;
     } else {
-        /* PV or HVM domains without a device model. */
+        assert(d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM);
+        assert(d_config->b_info.device_model_version ==
+               LIBXL_DEVICE_MODEL_VERSION_NONE);
+        /* HVMlite domains. */
         xc_config->emulation_flags = 0;
     }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 159d960..8ee5d76 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -542,8 +542,9 @@ 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_hvm_domain(d) && config->emulation_flags != XEN_X86_EMU_ALL &&
+             config->emulation_flags != 0) ||
+             (!is_hvm_domain(d) && config->emulation_flags != XEN_X86_EMU_PIT) )
         {
             printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
                    "with the current selection of emulators: %#x\n",
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 76c7b0f..08bd3fb 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -582,7 +582,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .parity    = 'n',
         .stop_bits = 1
     };
-    struct xen_arch_domainconfig config = { .emulation_flags = 0 };
+    struct xen_arch_domainconfig config = {
+        .emulation_flags = XEN_X86_EMU_PIT,
+    };
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
 
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v2 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 15.01.16 at 15:59, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -542,8 +542,9 @@ 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_hvm_domain(d) && config->emulation_flags != XEN_X86_EMU_ALL &&
> +             config->emulation_flags != 0) ||
> +             (!is_hvm_domain(d) && config->emulation_flags != XEN_X86_EMU_PIT) )

IMO it would be more readable when transforming

    if ( (a && b) || (!a && c) )

into

    if ( a ? b : c )

With that change
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH v3 4/4] x86/PV: enable the emulated PIT

[Roger Pau Monne]

The HVMlite series removed the initialization of the emulated PIT for PV
guests, this patch re-enables it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
NB: Since it's not clear why an emulated PIT is needed, it won't be enabled
by default for HVMlite guests until we can figure out if/why it's needed.
---
Changes since v2:
 - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.

Changes since v1:
 - New in this version.
---
 tools/libxl/libxl_x86.c | 8 +++++++-
 xen/arch/x86/domain.c   | 5 +++--
 xen/arch/x86/setup.c    | 4 +++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 46cfafb..7f47cc5 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -13,8 +13,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         LIBXL_DEVICE_MODEL_VERSION_NONE) {
         /* HVM domains with a device model. */
         xc_config->emulation_flags = XEN_X86_EMU_ALL;
+    } else if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
+        /* PV domains. */
+        xc_config->emulation_flags = XEN_X86_EMU_PIT;
     } else {
-        /* PV or HVM domains without a device model. */
+        assert(d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM);
+        assert(d_config->b_info.device_model_version ==
+               LIBXL_DEVICE_MODEL_VERSION_NONE);
+        /* HVMlite domains. */
         xc_config->emulation_flags = 0;
     }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 159d960..ae85e45 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
+             config->emulation_flags != 0) :
+             (config->emulation_flags != XEN_X86_EMU_PIT) )
         {
             printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
                    "with the current selection of emulators: %#x\n",
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 76c7b0f..08bd3fb 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -582,7 +582,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .parity    = 'n',
         .stop_bits = 1
     };
-    struct xen_arch_domainconfig config = { .emulation_flags = 0 };
+    struct xen_arch_domainconfig config = {
+        .emulation_flags = XEN_X86_EMU_PIT,
+    };
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
 
-- 
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/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
> Changes since v2:
>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.

Thanks, but after some more thinking about it I'm afraid there are
a few more aspects to consider here:

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
> +             config->emulation_flags != 0) :
> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>          {

For one I think it would be a good idea to allow zero for PV domains,
and perhaps even default new DomU-s to have the PIT flag clear.
(Also - indentation.)

Which gets us to the second, broader issue: These flags shouldn't
be forced to a particular value during migration, but instead they
should be part of the state getting migrated. Incoming domains
then would - if the field is missing due to coming from an older
hypervisor - have the flag default to 1.

And then - is all this working as intended for the hwdom != Dom0
case?

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 18/01/2016 07:43, Jan Beulich wrote:
>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>> Changes since v2:
>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
> Thanks, but after some more thinking about it I'm afraid there are
> a few more aspects to consider here:
>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>> +             config->emulation_flags != 0) :
>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>          {
> For one I think it would be a good idea to allow zero for PV domains,
> and perhaps even default new DomU-s to have the PIT flag clear.
> (Also - indentation.)
>
> Which gets us to the second, broader issue: These flags shouldn't
> be forced to a particular value during migration, but instead they
> should be part of the state getting migrated. Incoming domains
> then would - if the field is missing due to coming from an older
> hypervisor - have the flag default to 1.

There is sadly another ratsnest here.

These values are needed for domain creation, which means that putting
them anywhere in the migration stream is already too late, as the domain
has been created before the stream header is read.

In principle, the best which could occur is that a value gets stashed in
the stream and used as a sanity check.  That will at least catch the
case when they are different.

I was planning to do exactly the same for the vcpu count etc. as part of
my further cpuid work.  However, there is a substantial quantity of
development work required to make this function in a rational way.

For now,  I wouldn't worry too much.  It is just one of a very large
number of things which should be moved on migrate, but isn't.  (Most
notably, the cpuid policy.)

~Andrew

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>> Changes since v2:
>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>> Thanks, but after some more thinking about it I'm afraid there are
>> a few more aspects to consider here:
>>
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>> +             config->emulation_flags != 0) :
>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>          {
>> For one I think it would be a good idea to allow zero for PV domains,
>> and perhaps even default new DomU-s to have the PIT flag clear.
>> (Also - indentation.)
>>
>> Which gets us to the second, broader issue: These flags shouldn't
>> be forced to a particular value during migration, but instead they
>> should be part of the state getting migrated. Incoming domains
>> then would - if the field is missing due to coming from an older
>> hypervisor - have the flag default to 1.
> 
> There is sadly another ratsnest here.

I've been afraid of that.

> These values are needed for domain creation, which means that putting
> them anywhere in the migration stream is already too late, as the domain
> has been created before the stream header is read.

Is that an inherent requirement, or just a result of current code
structure? I ask because migrating the emulation flags is going to
be a requirement for relaxing the current (almost) all-or-nothing
policy on those flags.

> In principle, the best which could occur is that a value gets stashed in
> the stream and used as a sanity check.  That will at least catch the
> case when they are different.

That'd be a minimal first step.

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Roger Pau Monné]

El 18/01/16 a les 8.43, Jan Beulich ha escrit:
>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>> Changes since v2:
>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
> 
> Thanks, but after some more thinking about it I'm afraid there are
> a few more aspects to consider here:
> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>> +             config->emulation_flags != 0) :
>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>          {
> 
> For one I think it would be a good idea to allow zero for PV domains,
> and perhaps even default new DomU-s to have the PIT flag clear.
> (Also - indentation.)

This sounds fine to me, but IMHO, it should be done in a separate patch.
This patch just restores previous behaviour for PV guests, then we can
move on from there.

> And then - is all this working as intended for the hwdom != Dom0
> case?

I have to admit I have not tried it, but AFAICT in the hwdom != Dom0
case the set of enabled emulated devices should be the same as a normal
guest, the hardware domain doesn't get any more or less emulated devices
than any other guest ATM.

Roger.

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 18.01.16 at 10:50, <roger.pau@citrix.com> wrote:
> El 18/01/16 a les 8.43, Jan Beulich ha escrit:
>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>> Changes since v2:
>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>> 
>> Thanks, but after some more thinking about it I'm afraid there are
>> a few more aspects to consider here:
>> 
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>> +             config->emulation_flags != 0) :
>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>          {
>> 
>> For one I think it would be a good idea to allow zero for PV domains,
>> and perhaps even default new DomU-s to have the PIT flag clear.
>> (Also - indentation.)
> 
> This sounds fine to me, but IMHO, it should be done in a separate patch.
> This patch just restores previous behaviour for PV guests, then we can
> move on from there.

Well, maybe. To me it would seem cleaner if both options got
permitted by the code right away.

>> And then - is all this working as intended for the hwdom != Dom0
>> case?
> 
> I have to admit I have not tried it, but AFAICT in the hwdom != Dom0
> case the set of enabled emulated devices should be the same as a normal
> guest, the hardware domain doesn't get any more or less emulated devices
> than any other guest ATM.

No - see the use of is_hardware_domain() in pv_pit_handler().
IMO the flag should be forced on by the hypervisor for the
hardware domain, i.e. things be made independent of whatever
tooling gets used to create that domain.

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 18/01/16 09:44, Jan Beulich wrote:
>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>> Changes since v2:
>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>>> Thanks, but after some more thinking about it I'm afraid there are
>>> a few more aspects to consider here:
>>>
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>>> +             config->emulation_flags != 0) :
>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>          {
>>> For one I think it would be a good idea to allow zero for PV domains,
>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>> (Also - indentation.)
>>>
>>> Which gets us to the second, broader issue: These flags shouldn't
>>> be forced to a particular value during migration, but instead they
>>> should be part of the state getting migrated. Incoming domains
>>> then would - if the field is missing due to coming from an older
>>> hypervisor - have the flag default to 1.
>> There is sadly another ratsnest here.
> I've been afraid of that.
>
>> These values are needed for domain creation, which means that putting
>> them anywhere in the migration stream is already too late, as the domain
>> has been created before the stream header is read.
> Is that an inherent requirement, or just a result of current code
> structure?

Depends.  As far as libxc/libxl migration levels go, current code structure.

Whatever (eventually) gets used to set these values will however be
present in the xl configuration, which is at the very start of the
stream, and is what is used to create the new domain.

We really don't want the libxc migrate code to be making the
DOMCTL_createdomain hypercall itself; it opens up a whole new attack
surface via cunningly-crafted save image.  The best we can do is have a
sanity check later on.

>  I ask because migrating the emulation flags is going to
> be a requirement for relaxing the current (almost) all-or-nothing
> policy on those flags.
>
>> In principle, the best which could occur is that a value gets stashed in
>> the stream and used as a sanity check.  That will at least catch the
>> case when they are different.
> That'd be a minimal first step.

This is a substantial quantity of work to do properly.  As the emulation
flags are just one in a very long list of fields handed like this, I
don't think this issue should block the series.

~Andrew

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 18.01.16 at 11:41, <andrew.cooper3@citrix.com> wrote:
> On 18/01/16 09:44, Jan Beulich wrote:
>>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>>> Changes since v2:
>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>>>> Thanks, but after some more thinking about it I'm afraid there are
>>>> a few more aspects to consider here:
>>>>
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>>>> +             config->emulation_flags != 0) :
>>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>>          {
>>>> For one I think it would be a good idea to allow zero for PV domains,
>>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>>> (Also - indentation.)
>>>>
>>>> Which gets us to the second, broader issue: These flags shouldn't
>>>> be forced to a particular value during migration, but instead they
>>>> should be part of the state getting migrated. Incoming domains
>>>> then would - if the field is missing due to coming from an older
>>>> hypervisor - have the flag default to 1.
>>> There is sadly another ratsnest here.
>> I've been afraid of that.
>>
>>> These values are needed for domain creation, which means that putting
>>> them anywhere in the migration stream is already too late, as the domain
>>> has been created before the stream header is read.
>> Is that an inherent requirement, or just a result of current code
>> structure?
> 
> Depends.  As far as libxc/libxl migration levels go, current code structure.

I.e. fixable.

> Whatever (eventually) gets used to set these values will however be
> present in the xl configuration, which is at the very start of the
> stream, and is what is used to create the new domain.

Which makes me repeat the question: Is this an inherent property
or just "that's the way it is right now"? And then of course the
question arises whether setting those flags at domain creation time
is the right model. I.e. ...

> We really don't want the libxc migrate code to be making the
> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
> surface via cunningly-crafted save image.  The best we can do is have a
> sanity check later on.

... what about deriving the emulation flags from the various
pieces of state getting loaded, at least when there are matching
pairs (which namely is the case for PIT)?

>>> In principle, the best which could occur is that a value gets stashed in
>>> the stream and used as a sanity check.  That will at least catch the
>>> case when they are different.
>> That'd be a minimal first step.
> 
> This is a substantial quantity of work to do properly.  As the emulation
> flags are just one in a very long list of fields handed like this, I
> don't think this issue should block the series.

Ugly, but the way things seem to stand it may indeed be
unavoidable to ignore the issue for now.

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 18/01/16 11:06, Jan Beulich wrote:
>> Which gets us to the second, broader issue: These flags shouldn't
>> be forced to a particular value during migration, but instead they
>> should be part of the state getting migrated. Incoming domains
>> then would - if the field is missing due to coming from an older
>> hypervisor - have the flag default to 1.
>>>> There is sadly another ratsnest here.
>>> I've been afraid of that.
>>>
>>>> These values are needed for domain creation, which means that putting
>>>> them anywhere in the migration stream is already too late, as the domain
>>>> has been created before the stream header is read.
>>> Is that an inherent requirement, or just a result of current code
>>> structure?
>> Depends.  As far as libxc/libxl migration levels go, current code structure.
> I.e. fixable.
>
>> Whatever (eventually) gets used to set these values will however be
>> present in the xl configuration, which is at the very start of the
>> stream, and is what is used to create the new domain.
> Which makes me repeat the question: Is this an inherent property
> or just "that's the way it is right now"? And then of course the
> question arises whether setting those flags at domain creation time
> is the right model. I.e. ...
>
>> We really don't want the libxc migrate code to be making the
>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>> surface via cunningly-crafted save image.  The best we can do is have a
>> sanity check later on.
> ... what about deriving the emulation flags from the various
> pieces of state getting loaded, at least when there are matching
> pairs (which namely is the case for PIT)?

How would you suggest setting theses flags up in the plain domain build
case then?

~Andrew

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 18.01.16 at 17:10, <andrew.cooper3@citrix.com> wrote:
> On 18/01/16 11:06, Jan Beulich wrote:
>>> Whatever (eventually) gets used to set these values will however be
>>> present in the xl configuration, which is at the very start of the
>>> stream, and is what is used to create the new domain.
>> Which makes me repeat the question: Is this an inherent property
>> or just "that's the way it is right now"? And then of course the
>> question arises whether setting those flags at domain creation time
>> is the right model. I.e. ...
>>
>>> We really don't want the libxc migrate code to be making the
>>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>>> surface via cunningly-crafted save image.  The best we can do is have a
>>> sanity check later on.
>> ... what about deriving the emulation flags from the various
>> pieces of state getting loaded, at least when there are matching
>> pairs (which namely is the case for PIT)?
> 
> How would you suggest setting theses flags up in the plain domain build
> case then?

Via a specific (new) hypercall, along the lines of what
XEN_DOMCTL_arm_configure_domain was?

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 18/01/16 16:27, Jan Beulich wrote:
>>>> On 18.01.16 at 17:10, <andrew.cooper3@citrix.com> wrote:
>> On 18/01/16 11:06, Jan Beulich wrote:
>>>> Whatever (eventually) gets used to set these values will however be
>>>> present in the xl configuration, which is at the very start of the
>>>> stream, and is what is used to create the new domain.
>>> Which makes me repeat the question: Is this an inherent property
>>> or just "that's the way it is right now"? And then of course the
>>> question arises whether setting those flags at domain creation time
>>> is the right model. I.e. ...
>>>
>>>> We really don't want the libxc migrate code to be making the
>>>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>>>> surface via cunningly-crafted save image.  The best we can do is have a
>>>> sanity check later on.
>>> ... what about deriving the emulation flags from the various
>>> pieces of state getting loaded, at least when there are matching
>>> pairs (which namely is the case for PIT)?
>> How would you suggest setting theses flags up in the plain domain build
>> case then?
> Via a specific (new) hypercall, along the lines of what
> XEN_DOMCTL_arm_configure_domain was?

This adds the existing problems we have between the createdomain and
max_cpus hypercalls.

We need to either specify all information in a single hypercall, or have
a dedicated construction phase, during which most hypercalls are invalid
to use.

(IMO - All domain construction is a rats nest in need of redesigning
from scratch.)

~Andrew

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Jan Beulich]

>>> On 18.01.16 at 17:33, <andrew.cooper3@citrix.com> wrote:
> On 18/01/16 16:27, Jan Beulich wrote:
>>>>> On 18.01.16 at 17:10, <andrew.cooper3@citrix.com> wrote:
>>> On 18/01/16 11:06, Jan Beulich wrote:
>>>>> Whatever (eventually) gets used to set these values will however be
>>>>> present in the xl configuration, which is at the very start of the
>>>>> stream, and is what is used to create the new domain.
>>>> Which makes me repeat the question: Is this an inherent property
>>>> or just "that's the way it is right now"? And then of course the
>>>> question arises whether setting those flags at domain creation time
>>>> is the right model. I.e. ...
>>>>
>>>>> We really don't want the libxc migrate code to be making the
>>>>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>>>>> surface via cunningly-crafted save image.  The best we can do is have a
>>>>> sanity check later on.
>>>> ... what about deriving the emulation flags from the various
>>>> pieces of state getting loaded, at least when there are matching
>>>> pairs (which namely is the case for PIT)?
>>> How would you suggest setting theses flags up in the plain domain build
>>> case then?
>> Via a specific (new) hypercall, along the lines of what
>> XEN_DOMCTL_arm_configure_domain was?
> 
> This adds the existing problems we have between the createdomain and
> max_cpus hypercalls.
> 
> We need to either specify all information in a single hypercall, or have
> a dedicated construction phase, during which most hypercalls are invalid
> to use.

Which is a reasonable requirement to enforce imo.

Jan

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Roger Pau Monné]

El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
> On 18/01/16 09:44, Jan Beulich wrote:
>>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>>> Changes since v2:
>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>>>> Thanks, but after some more thinking about it I'm afraid there are
>>>> a few more aspects to consider here:
>>>>
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>>>> +             config->emulation_flags != 0) :
>>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>>          {
>>>> For one I think it would be a good idea to allow zero for PV domains,
>>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>>> (Also - indentation.)
>>>>
>>>> Which gets us to the second, broader issue: These flags shouldn't
>>>> be forced to a particular value during migration, but instead they
>>>> should be part of the state getting migrated. Incoming domains
>>>> then would - if the field is missing due to coming from an older
>>>> hypervisor - have the flag default to 1.
>>> There is sadly another ratsnest here.
>> I've been afraid of that.
>>
>>> These values are needed for domain creation, which means that putting
>>> them anywhere in the migration stream is already too late, as the domain
>>> has been created before the stream header is read.
>> Is that an inherent requirement, or just a result of current code
>> structure?
> 
> Depends.  As far as libxc/libxl migration levels go, current code structure.
> 
> Whatever (eventually) gets used to set these values will however be
> present in the xl configuration, which is at the very start of the
> stream, and is what is used to create the new domain.
> 
> We really don't want the libxc migrate code to be making the
> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
> surface via cunningly-crafted save image.  The best we can do is have a
> sanity check later on.
> 
>>  I ask because migrating the emulation flags is going to
>> be a requirement for relaxing the current (almost) all-or-nothing
>> policy on those flags.
>>
>>> In principle, the best which could occur is that a value gets stashed in
>>> the stream and used as a sanity check.  That will at least catch the
>>> case when they are different.
>> That'd be a minimal first step.
> 
> This is a substantial quantity of work to do properly.  As the emulation
> flags are just one in a very long list of fields handed like this, I
> don't think this issue should block the series.

You certainly are more familiar with the migration code than me, but
wouldn't it be enough to add a new field to libxl_domain_build_info
(uint32_t emulation_flags), and teach
libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
 how to properly parse it?

This however raises the question about how to signal that the field is
not initialised, because 0 is a valid value (maybe ~0)?

Roger.

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 18/01/16 17:58, Roger Pau Monné wrote:
> El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
>> On 18/01/16 09:44, Jan Beulich wrote:
>>>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>>>> Changes since v2:
>>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c )'.
>>>>> Thanks, but after some more thinking about it I'm afraid there are
>>>>> a few more aspects to consider here:
>>>>>
>>>>>> --- a/xen/arch/x86/domain.c
>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags != XEN_X86_EMU_ALL &&
>>>>>> +             config->emulation_flags != 0) :
>>>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>>>          {
>>>>> For one I think it would be a good idea to allow zero for PV domains,
>>>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>>>> (Also - indentation.)
>>>>>
>>>>> Which gets us to the second, broader issue: These flags shouldn't
>>>>> be forced to a particular value during migration, but instead they
>>>>> should be part of the state getting migrated. Incoming domains
>>>>> then would - if the field is missing due to coming from an older
>>>>> hypervisor - have the flag default to 1.
>>>> There is sadly another ratsnest here.
>>> I've been afraid of that.
>>>
>>>> These values are needed for domain creation, which means that putting
>>>> them anywhere in the migration stream is already too late, as the domain
>>>> has been created before the stream header is read.
>>> Is that an inherent requirement, or just a result of current code
>>> structure?
>> Depends.  As far as libxc/libxl migration levels go, current code structure.
>>
>> Whatever (eventually) gets used to set these values will however be
>> present in the xl configuration, which is at the very start of the
>> stream, and is what is used to create the new domain.
>>
>> We really don't want the libxc migrate code to be making the
>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>> surface via cunningly-crafted save image.  The best we can do is have a
>> sanity check later on.
>>
>>>  I ask because migrating the emulation flags is going to
>>> be a requirement for relaxing the current (almost) all-or-nothing
>>> policy on those flags.
>>>
>>>> In principle, the best which could occur is that a value gets stashed in
>>>> the stream and used as a sanity check.  That will at least catch the
>>>> case when they are different.
>>> That'd be a minimal first step.
>> This is a substantial quantity of work to do properly.  As the emulation
>> flags are just one in a very long list of fields handed like this, I
>> don't think this issue should block the series.
> You certainly are more familiar with the migration code than me, but
> wouldn't it be enough to add a new field to libxl_domain_build_info
> (uint32_t emulation_flags), and teach
> libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
>  how to properly parse it?

That would let it be configured from an xl.cfg file, and would normally
be moved in the migration stream.  However, there is a specific option
in xl to restore but using a brand new configuration file.

What it doesn't do it check that the settings for the domain in the
stream match the settings of the domid being restored into.

~Andrew

>
> This however raises the question about how to signal that the field is
> not initialised, because 0 is a valid value (maybe ~0)?
>
> Roger.
>

Re: [PATCH v2 1/4] xen/elfnotes: check phys_entry against UNSET_ADDR32

[Wei Liu]

On Fri, Jan 15, 2016 at 03:59:40PM +0100, Roger Pau Monne wrote:
> And introduce UNSET_ADDR32.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> Changes since v1:
>  - Fix commit title.
> ---
>  tools/libxc/xc_dom_elfloader.c     | 2 +-
>  xen/common/libelf/libelf-dominfo.c | 1 +
>  xen/include/xen/libelf.h           | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index 2ae575e..5039f3f 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -57,7 +57,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
>      uint64_t machine = elf_uval(elf, elf->ehdr, e_machine);
>  
>      if ( dom->container_type == XC_DOM_HVM_CONTAINER &&
> -         dom->parms.phys_entry != UNSET_ADDR )
> +         dom->parms.phys_entry != UNSET_ADDR32 )
>          return "hvm-3.0-x86_32";
>  

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Ian Campbell]

On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
> On 18/01/16 17:58, Roger Pau Monné wrote:
> > El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
> > > On 18/01/16 09:44, Jan Beulich wrote:
> > > > > > > On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
> > > > > On 18/01/2016 07:43, Jan Beulich wrote:
> > > > > > > > > On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
> > > > > > > Changes since v2:
> > > > > > >  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c
> > > > > > > )'.
> > > > > > Thanks, but after some more thinking about it I'm afraid there
> > > > > > are
> > > > > > a few more aspects to consider here:
> > > > > > 
> > > > > > > --- a/xen/arch/x86/domain.c
> > > > > > > +++ b/xen/arch/x86/domain.c
> > > > > > > @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags !=
> > > > > > > XEN_X86_EMU_ALL &&
> > > > > > > +             config->emulation_flags != 0) :
> > > > > > > +             (config->emulation_flags != XEN_X86_EMU_PIT) )
> > > > > > >          {
> > > > > > For one I think it would be a good idea to allow zero for PV
> > > > > > domains,
> > > > > > and perhaps even default new DomU-s to have the PIT flag clear.
> > > > > > (Also - indentation.)
> > > > > > 
> > > > > > Which gets us to the second, broader issue: These flags
> > > > > > shouldn't
> > > > > > be forced to a particular value during migration, but instead
> > > > > > they
> > > > > > should be part of the state getting migrated. Incoming domains
> > > > > > then would - if the field is missing due to coming from an
> > > > > > older
> > > > > > hypervisor - have the flag default to 1.
> > > > > There is sadly another ratsnest here.
> > > > I've been afraid of that.
> > > > 
> > > > > These values are needed for domain creation, which means that
> > > > > putting
> > > > > them anywhere in the migration stream is already too late, as the
> > > > > domain
> > > > > has been created before the stream header is read.
> > > > Is that an inherent requirement, or just a result of current code
> > > > structure?
> > > Depends.  As far as libxc/libxl migration levels go, current code
> > > structure.
> > > 
> > > Whatever (eventually) gets used to set these values will however be
> > > present in the xl configuration, which is at the very start of the
> > > stream, and is what is used to create the new domain.
> > > 
> > > We really don't want the libxc migrate code to be making the
> > > DOMCTL_createdomain hypercall itself; it opens up a whole new attack
> > > surface via cunningly-crafted save image.  The best we can do is have
> > > a
> > > sanity check later on.
> > > 
> > > >  I ask because migrating the emulation flags is going to
> > > > be a requirement for relaxing the current (almost) all-or-nothing
> > > > policy on those flags.
> > > > 
> > > > > In principle, the best which could occur is that a value gets
> > > > > stashed in
> > > > > the stream and used as a sanity check.  That will at least catch
> > > > > the
> > > > > case when they are different.
> > > > That'd be a minimal first step.
> > > This is a substantial quantity of work to do properly.  As the
> > > emulation
> > > flags are just one in a very long list of fields handed like this, I
> > > don't think this issue should block the series.
> > You certainly are more familiar with the migration code than me, but
> > wouldn't it be enough to add a new field to libxl_domain_build_info
> > (uint32_t emulation_flags), and teach
> > libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
> >  how to properly parse it?
> 
> That would let it be configured from an xl.cfg file, and would normally
> be moved in the migration stream.  However, there is a specific option
> in xl to restore but using a brand new configuration file.
> 
> What it doesn't do it check that the settings for the domain in the
> stream match the settings of the domid being restored into.

That would be the responsibility of the user who has chosen to override the
configuration in this way.

Ian.


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

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 19/01/16 09:24, Ian Campbell wrote:
> On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
>> On 18/01/16 17:58, Roger Pau Monné wrote:
>>> El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
>>>> On 18/01/16 09:44, Jan Beulich wrote:
>>>>>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>>>>>> Changes since v2:
>>>>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c
>>>>>>>> )'.
>>>>>>> Thanks, but after some more thinking about it I'm afraid there
>>>>>>> are
>>>>>>> a few more aspects to consider here:
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags !=
>>>>>>>> XEN_X86_EMU_ALL &&
>>>>>>>> +             config->emulation_flags != 0) :
>>>>>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>>>>>          {
>>>>>>> For one I think it would be a good idea to allow zero for PV
>>>>>>> domains,
>>>>>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>>>>>> (Also - indentation.)
>>>>>>>
>>>>>>> Which gets us to the second, broader issue: These flags
>>>>>>> shouldn't
>>>>>>> be forced to a particular value during migration, but instead
>>>>>>> they
>>>>>>> should be part of the state getting migrated. Incoming domains
>>>>>>> then would - if the field is missing due to coming from an
>>>>>>> older
>>>>>>> hypervisor - have the flag default to 1.
>>>>>> There is sadly another ratsnest here.
>>>>> I've been afraid of that.
>>>>>
>>>>>> These values are needed for domain creation, which means that
>>>>>> putting
>>>>>> them anywhere in the migration stream is already too late, as the
>>>>>> domain
>>>>>> has been created before the stream header is read.
>>>>> Is that an inherent requirement, or just a result of current code
>>>>> structure?
>>>> Depends.  As far as libxc/libxl migration levels go, current code
>>>> structure.
>>>>
>>>> Whatever (eventually) gets used to set these values will however be
>>>> present in the xl configuration, which is at the very start of the
>>>> stream, and is what is used to create the new domain.
>>>>
>>>> We really don't want the libxc migrate code to be making the
>>>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>>>> surface via cunningly-crafted save image.  The best we can do is have
>>>> a
>>>> sanity check later on.
>>>>
>>>>>  I ask because migrating the emulation flags is going to
>>>>> be a requirement for relaxing the current (almost) all-or-nothing
>>>>> policy on those flags.
>>>>>
>>>>>> In principle, the best which could occur is that a value gets
>>>>>> stashed in
>>>>>> the stream and used as a sanity check.  That will at least catch
>>>>>> the
>>>>>> case when they are different.
>>>>> That'd be a minimal first step.
>>>> This is a substantial quantity of work to do properly.  As the
>>>> emulation
>>>> flags are just one in a very long list of fields handed like this, I
>>>> don't think this issue should block the series.
>>> You certainly are more familiar with the migration code than me, but
>>> wouldn't it be enough to add a new field to libxl_domain_build_info
>>> (uint32_t emulation_flags), and teach
>>> libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
>>>  how to properly parse it?
>> That would let it be configured from an xl.cfg file, and would normally
>> be moved in the migration stream.  However, there is a specific option
>> in xl to restore but using a brand new configuration file.
>>
>> What it doesn't do it check that the settings for the domain in the
>> stream match the settings of the domid being restored into.
> That would be the responsibility of the user who has chosen to override the
> configuration in this way.

It is the responsibility of Xen to ensure there are no exploitable holes
due to partial or misconfiguration.

In particular, this PIT emulation patch fixes an accidental NULL pointer
dereference in Xen, due to the accidental disabling of the PIT in PV guests.

~Andrew

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

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Ian Campbell]

On Tue, 2016-01-19 at 10:09 +0000, Andrew Cooper wrote:
> On 19/01/16 09:24, Ian Campbell wrote:
> > On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
> > > On 18/01/16 17:58, Roger Pau Monné wrote:
> > > > El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
> > > > > On 18/01/16 09:44, Jan Beulich wrote:
> > > > > > > > > On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
> > > > > > > On 18/01/2016 07:43, Jan Beulich wrote:
> > > > > > > > > > > On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
> > > > > > > > > Changes since v2:
> > > > > > > > >  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b
> > > > > > > > > : c
> > > > > > > > > )'.
> > > > > > > > Thanks, but after some more thinking about it I'm afraid
> > > > > > > > there
> > > > > > > > are
> > > > > > > > a few more aspects to consider here:
> > > > > > > > 
> > > > > > > > > --- a/xen/arch/x86/domain.c
> > > > > > > > > +++ b/xen/arch/x86/domain.c
> > > > > > > > > @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags 
> > > > > > > > > !=
> > > > > > > > > XEN_X86_EMU_ALL &&
> > > > > > > > > +             config->emulation_flags != 0) :
> > > > > > > > > +             (config->emulation_flags !=
> > > > > > > > > XEN_X86_EMU_PIT) )
> > > > > > > > >          {
> > > > > > > > For one I think it would be a good idea to allow zero for
> > > > > > > > PV
> > > > > > > > domains,
> > > > > > > > and perhaps even default new DomU-s to have the PIT flag
> > > > > > > > clear.
> > > > > > > > (Also - indentation.)
> > > > > > > > 
> > > > > > > > Which gets us to the second, broader issue: These flags
> > > > > > > > shouldn't
> > > > > > > > be forced to a particular value during migration, but
> > > > > > > > instead
> > > > > > > > they
> > > > > > > > should be part of the state getting migrated. Incoming
> > > > > > > > domains
> > > > > > > > then would - if the field is missing due to coming from an
> > > > > > > > older
> > > > > > > > hypervisor - have the flag default to 1.
> > > > > > > There is sadly another ratsnest here.
> > > > > > I've been afraid of that.
> > > > > > 
> > > > > > > These values are needed for domain creation, which means that
> > > > > > > putting
> > > > > > > them anywhere in the migration stream is already too late, as
> > > > > > > the
> > > > > > > domain
> > > > > > > has been created before the stream header is read.
> > > > > > Is that an inherent requirement, or just a result of current
> > > > > > code
> > > > > > structure?
> > > > > Depends.  As far as libxc/libxl migration levels go, current code
> > > > > structure.
> > > > > 
> > > > > Whatever (eventually) gets used to set these values will however
> > > > > be
> > > > > present in the xl configuration, which is at the very start of
> > > > > the
> > > > > stream, and is what is used to create the new domain.
> > > > > 
> > > > > We really don't want the libxc migrate code to be making the
> > > > > DOMCTL_createdomain hypercall itself; it opens up a whole new
> > > > > attack
> > > > > surface via cunningly-crafted save image.  The best we can do is
> > > > > have
> > > > > a
> > > > > sanity check later on.
> > > > > 
> > > > > >  I ask because migrating the emulation flags is going to
> > > > > > be a requirement for relaxing the current (almost) all-or-
> > > > > > nothing
> > > > > > policy on those flags.
> > > > > > 
> > > > > > > In principle, the best which could occur is that a value gets
> > > > > > > stashed in
> > > > > > > the stream and used as a sanity check.  That will at least
> > > > > > > catch
> > > > > > > the
> > > > > > > case when they are different.
> > > > > > That'd be a minimal first step.
> > > > > This is a substantial quantity of work to do properly.  As the
> > > > > emulation
> > > > > flags are just one in a very long list of fields handed like
> > > > > this, I
> > > > > don't think this issue should block the series.
> > > > You certainly are more familiar with the migration code than me,
> > > > but
> > > > wouldn't it be enough to add a new field to libxl_domain_build_info
> > > > (uint32_t emulation_flags), and teach
> > > > libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_jso
> > > > n
> > > >  how to properly parse it?
> > > That would let it be configured from an xl.cfg file, and would
> > > normally
> > > be moved in the migration stream.  However, there is a specific
> > > option
> > > in xl to restore but using a brand new configuration file.
> > > 
> > > What it doesn't do it check that the settings for the domain in the
> > > stream match the settings of the domid being restored into.
> > That would be the responsibility of the user who has chosen to override
> > the
> > configuration in this way.
> 
> It is the responsibility of Xen to ensure there are no exploitable holes
> due to partial or misconfiguration.

Indeed, but it only needs to check things and fail, not work in the face of
a bogus save file + cfg file configuration. Perhaps I misunderstood what
was being contended here.

Ian.

> In particular, this PIT emulation patch fixes an accidental NULL pointer
> dereference in Xen, due to the accidental disabling of the PIT in PV
> guests.
> 
> ~Andrew

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

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper]

On 19/01/16 10:28, Ian Campbell wrote:
> On Tue, 2016-01-19 at 10:09 +0000, Andrew Cooper wrote:
>> On 19/01/16 09:24, Ian Campbell wrote:
>>> On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
>>>> On 18/01/16 17:58, Roger Pau Monné wrote:
>>>>> El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
>>>>>> On 18/01/16 09:44, Jan Beulich wrote:
>>>>>>>>>> On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
>>>>>>>>>> Changes since v2:
>>>>>>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b
>>>>>>>>>> : c
>>>>>>>>>> )'.
>>>>>>>>> Thanks, but after some more thinking about it I'm afraid
>>>>>>>>> there
>>>>>>>>> are
>>>>>>>>> a few more aspects to consider here:
>>>>>>>>>
>>>>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>>>>> @@ -542,8 +542,9 @@ 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_hvm_domain(d) ? (config->emulation_flags 
>>>>>>>>>> !=
>>>>>>>>>> XEN_X86_EMU_ALL &&
>>>>>>>>>> +             config->emulation_flags != 0) :
>>>>>>>>>> +             (config->emulation_flags !=
>>>>>>>>>> XEN_X86_EMU_PIT) )
>>>>>>>>>>          {
>>>>>>>>> For one I think it would be a good idea to allow zero for
>>>>>>>>> PV
>>>>>>>>> domains,
>>>>>>>>> and perhaps even default new DomU-s to have the PIT flag
>>>>>>>>> clear.
>>>>>>>>> (Also - indentation.)
>>>>>>>>>
>>>>>>>>> Which gets us to the second, broader issue: These flags
>>>>>>>>> shouldn't
>>>>>>>>> be forced to a particular value during migration, but
>>>>>>>>> instead
>>>>>>>>> they
>>>>>>>>> should be part of the state getting migrated. Incoming
>>>>>>>>> domains
>>>>>>>>> then would - if the field is missing due to coming from an
>>>>>>>>> older
>>>>>>>>> hypervisor - have the flag default to 1.
>>>>>>>> There is sadly another ratsnest here.
>>>>>>> I've been afraid of that.
>>>>>>>
>>>>>>>> These values are needed for domain creation, which means that
>>>>>>>> putting
>>>>>>>> them anywhere in the migration stream is already too late, as
>>>>>>>> the
>>>>>>>> domain
>>>>>>>> has been created before the stream header is read.
>>>>>>> Is that an inherent requirement, or just a result of current
>>>>>>> code
>>>>>>> structure?
>>>>>> Depends.  As far as libxc/libxl migration levels go, current code
>>>>>> structure.
>>>>>>
>>>>>> Whatever (eventually) gets used to set these values will however
>>>>>> be
>>>>>> present in the xl configuration, which is at the very start of
>>>>>> the
>>>>>> stream, and is what is used to create the new domain.
>>>>>>
>>>>>> We really don't want the libxc migrate code to be making the
>>>>>> DOMCTL_createdomain hypercall itself; it opens up a whole new
>>>>>> attack
>>>>>> surface via cunningly-crafted save image.  The best we can do is
>>>>>> have
>>>>>> a
>>>>>> sanity check later on.
>>>>>>
>>>>>>>  I ask because migrating the emulation flags is going to
>>>>>>> be a requirement for relaxing the current (almost) all-or-
>>>>>>> nothing
>>>>>>> policy on those flags.
>>>>>>>
>>>>>>>> In principle, the best which could occur is that a value gets
>>>>>>>> stashed in
>>>>>>>> the stream and used as a sanity check.  That will at least
>>>>>>>> catch
>>>>>>>> the
>>>>>>>> case when they are different.
>>>>>>> That'd be a minimal first step.
>>>>>> This is a substantial quantity of work to do properly.  As the
>>>>>> emulation
>>>>>> flags are just one in a very long list of fields handed like
>>>>>> this, I
>>>>>> don't think this issue should block the series.
>>>>> You certainly are more familiar with the migration code than me,
>>>>> but
>>>>> wouldn't it be enough to add a new field to libxl_domain_build_info
>>>>> (uint32_t emulation_flags), and teach
>>>>> libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_jso
>>>>> n
>>>>>  how to properly parse it?
>>>> That would let it be configured from an xl.cfg file, and would
>>>> normally
>>>> be moved in the migration stream.  However, there is a specific
>>>> option
>>>> in xl to restore but using a brand new configuration file.
>>>>
>>>> What it doesn't do it check that the settings for the domain in the
>>>> stream match the settings of the domid being restored into.
>>> That would be the responsibility of the user who has chosen to override
>>> the
>>> configuration in this way.
>> It is the responsibility of Xen to ensure there are no exploitable holes
>> due to partial or misconfiguration.
> Indeed, but it only needs to check things and fail, not work in the face of
> a bogus save file + cfg file configuration. Perhaps I misunderstood what
> was being contended here.

It would appear that the choices are:

1) Rearchitect all domain building/restore from scratch
2) Implement a check & fail properly (Still a large quantity of work,
but less than 1)
3) Hack up a check & fail quickly

There are a very large number of areas which should be checked on
migrate which currently are not.  I already have plans to address 2) for
the cpuid work.

~Andrew

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

Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Ian Campbell]

On Mon, 2016-01-18 at 18:58 +0100, Roger Pau Monné wrote:
> You certainly are more familiar with the migration code than me, but
> wouldn't it be enough to add a new field to libxl_domain_build_info
> (uint32_t emulation_flags), and teach
> libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
>  how to properly parse it?

Is libxl not already aware whether a domain is to be dmlite vs pv or hvm
based on the domain config?

As such can't it figure out the hardcoded set to use already without
actually needing to expose this in libxl API (until we actually have a
desire to expose such nobs to users)? Can't this work on resume just like
it does on create?

> This however raises the question about how to signal that the field is
> not initialised, because 0 is a valid value (maybe ~0)?

In libxl's API we tend to try and avoid opaque hex numbers, so the natural
result would be a struct full of libxl_defbool options, which IIRC already
get properly propagated and have handling for defaulting already in place.

Ian.
> 

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