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

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

[Roger Pau Monne]

Hello,

This series contains some bug fixes for HVMlite DomU and some preparatory 
patches for HVMlite Dom0 support.

Thanks, Roger.

[PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Roger Pau Monne]

And introduce UNSET_ADDR32.

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/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 2/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 3/5] 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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 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 4/5] x86/PV: remove the emulated PIT

[Roger Pau Monne]

The HVMlite series removed the initialization of the emulated PIT for PV
guests, but the handler was still reachable, which means a PV guests can
crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
PV PIT handler and move the PIT initialization to HVM guests only.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain.c         |  3 ---
 xen/arch/x86/hvm/hvm.c        |  2 ++
 xen/arch/x86/hvm/i8254.c      | 27 ---------------------------
 xen/arch/x86/traps.c          | 12 ++----------
 xen/include/asm-x86/hvm/vpt.h |  1 -
 5 files changed, 4 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 159d960..868ef49 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -647,9 +647,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
     spin_lock_init(&d->arch.vtsc_lock);
 
-    /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
-    pit_init(d, cpu_khz);
-
     return 0;
 
  fail:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 05c3ca1..28c6cd9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1622,6 +1622,8 @@ int hvm_domain_initialise(struct domain *d)
 
     msixtbl_init(d);
 
+    pit_init(d, cpu_khz);
+
     register_portio_handler(d, 0xe9, 1, hvm_print_line);
     register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index b517cd6..83eae33 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -558,33 +558,6 @@ static int handle_speaker_io(
     return X86EMUL_OKAY;
 }
 
-int pv_pit_handler(int port, int data, int write)
-{
-    ioreq_t ioreq = {
-        .size = 1,
-        .type = IOREQ_TYPE_PIO,
-        .addr = port,
-        .dir  = write ? IOREQ_WRITE : IOREQ_READ,
-        .data = data
-    };
-
-    if ( is_hardware_domain(current->domain) && hwdom_pit_access(&ioreq) )
-    {
-        /* nothing to do */;
-    }
-    else
-    {
-        uint32_t val = data;
-        if ( port == 0x61 )
-            handle_speaker_io(ioreq.dir, port, 1, &val);
-        else
-            handle_pit_io(ioreq.dir, port, 1, &val);
-        ioreq.data = val;
-    }
-
-    return !write ? ioreq.data : 0;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e105b95..a9d7e83 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1839,11 +1839,7 @@ uint32_t guest_io_read(unsigned int port, unsigned int bytes,
         unsigned int size = 1;
         uint32_t sub_data = ~0;
 
-        if ( (port == 0x42) || (port == 0x43) || (port == 0x61) )
-        {
-            sub_data = pv_pit_handler(port, 0, 0);
-        }
-        else if ( (port == RTC_PORT(0)) )
+        if ( (port == RTC_PORT(0)) )
         {
             sub_data = currd->arch.cmos_idx;
         }
@@ -1908,11 +1904,7 @@ void guest_io_write(unsigned int port, unsigned int bytes, uint32_t data,
     {
         unsigned int size = 1;
 
-        if ( (port == 0x42) || (port == 0x43) || (port == 0x61) )
-        {
-            pv_pit_handler(port, (uint8_t)data, 1);
-        }
-        else if ( (port == RTC_PORT(0)) )
+        if ( (port == RTC_PORT(0)) )
         {
             currd->arch.cmos_idx = data;
         }
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 495d669..557bb4a 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -172,7 +172,6 @@ void create_periodic_time(
     uint64_t period, uint8_t irq, time_cb *cb, void *data);
 void destroy_periodic_time(struct periodic_time *pt);
 
-int pv_pit_handler(int port, int data, int write);
 void pit_reset(struct domain *d);
 
 void pit_init(struct domain *d, unsigned long cpu_khz);
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Roger Pau Monne]

Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
server, otherwise it makes no sense since the only code that uses the value
stored by hvm_access_cf8 is the IOREQ server.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 28c6cd9..24c3d46 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1141,6 +1141,13 @@ static int hvm_create_ioreq_server(struct domain *d, domid_t domid,
     if ( rc )
         goto fail3;
 
+    /*
+     * We cannot fail after this point, or we risk registering the handler
+     * multiple times (there's no unregister function yet).
+     */
+    if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
+        register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
+
     list_add(&s->list_entry,
              &d->arch.hvm_domain.ioreq_server.list);
 
@@ -1625,7 +1632,6 @@ int hvm_domain_initialise(struct domain *d)
     pit_init(d, cpu_khz);
 
     register_portio_handler(d, 0xe9, 1, hvm_print_line);
-    register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 
     rc = hvm_funcs.domain_initialise(d);
     if ( rc != 0 )
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Jan Beulich]

>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:

Re the title: I think the fix here really is to the checking against the
right value, not the initialization?

>  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(-)

With this diffstat, any reason only tools maintainers got Cc-ed?

Contents: Acked-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Roger Pau Monné]

El 13/01/16 a les 13.46, Jan Beulich ha escrit:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> 
> Re the title: I think the fix here really is to the checking against the
> right value, not the initialization?

Yes, the initialization doesn't matter much, the proper fix is the
checking. I would change it to:

xen/elfnotes: check phys_entry against UNSET_ADDR32

Which I think can be done by the committer (or I can resend if needed).

>>  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(-)
> 
> With this diffstat, any reason only tools maintainers got Cc-ed?

Hm, weird, that's what scripts/get_maintainer.pl gave me as output.

> Contents: Acked-by: Jan Beulich <jbeulich@suse.com>
> 
>> --- 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 
> 
> 
> 
> 

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Jan Beulich]

>>> On 13.01.16 at 13:52, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 13.46, Jan Beulich ha escrit:
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> 
>> Re the title: I think the fix here really is to the checking against the
>> right value, not the initialization?
> 
> Yes, the initialization doesn't matter much, the proper fix is the
> checking. I would change it to:
> 
> xen/elfnotes: check phys_entry against UNSET_ADDR32
> 
> Which I think can be done by the committer (or I can resend if needed).

Sure.

>>>  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(-)
>> 
>> With this diffstat, any reason only tools maintainers got Cc-ed?
> 
> Hm, weird, that's what scripts/get_maintainer.pl gave me as output.

Something must be broken there.

Jan

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Andrew Cooper]

On 13/01/16 13:02, Jan Beulich wrote:
>>>> On 13.01.16 at 13:52, <roger.pau@citrix.com> wrote:
>> El 13/01/16 a les 13.46, Jan Beulich ha escrit:
>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> Re the title: I think the fix here really is to the checking against the
>>> right value, not the initialization?
>> Yes, the initialization doesn't matter much, the proper fix is the
>> checking. I would change it to:
>>
>> xen/elfnotes: check phys_entry against UNSET_ADDR32
>>
>> Which I think can be done by the committer (or I can resend if needed).
> Sure.
>
>>>>  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(-)
>>> With this diffstat, any reason only tools maintainers got Cc-ed?
>> Hm, weird, that's what scripts/get_maintainer.pl gave me as output.
> Something must be broken there.

There is not an entry covering xen/common.

~Andrew

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Jan Beulich]

>>> On 13.01.16 at 14:05, <andrew.cooper3@citrix.com> wrote:
> On 13/01/16 13:02, Jan Beulich wrote:
>>>>> On 13.01.16 at 13:52, <roger.pau@citrix.com> wrote:
>>> El 13/01/16 a les 13.46, Jan Beulich ha escrit:
>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>  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(-)
>>>> With this diffstat, any reason only tools maintainers got Cc-ed?
>>> Hm, weird, that's what scripts/get_maintainer.pl gave me as output.
>> Something must be broken there.
> 
> There is not an entry covering xen/common.

And hence for those file it should fall back to THE REST.

Jan

Re: [PATCH 1/5] xen/elfnotes: initialise phys_entry to UNSET_ADDR32

[Andrew Cooper]

On 13/01/16 13:08, Jan Beulich wrote:
>>>> On 13.01.16 at 14:05, <andrew.cooper3@citrix.com> wrote:
>> On 13/01/16 13:02, Jan Beulich wrote:
>>>>>> On 13.01.16 at 13:52, <roger.pau@citrix.com> wrote:
>>>> El 13/01/16 a les 13.46, Jan Beulich ha escrit:
>>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>>  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(-)
>>>>> With this diffstat, any reason only tools maintainers got Cc-ed?
>>>> Hm, weird, that's what scripts/get_maintainer.pl gave me as output.
>>> Something must be broken there.
>> There is not an entry covering xen/common.
> And hence for those file it should fall back to THE REST.

Per original review, THE REST is only invoked on a patch which doesn't
match any paths.

As one file matched toolstack, THE REST wasn't invoked.

There really should be a xen/ entry.

~Andrew

Re: [PATCH 3/5] x86/hvm: don't set the BSP as initialised in hvm_vcpu_initialise

[Jan Beulich]

>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> 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.

But the comment that you remove says explicitly that this isn't a
requirement.

Jan

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  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

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Jan Beulich]

>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> The HVMlite series removed the initialization of the emulated PIT for PV
> guests, but the handler was still reachable, which means a PV guests can
> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
> PV PIT handler and move the PIT initialization to HVM guests only.

As said on IRC - this is needed for Dom0 to be able to drive the
PC speaker. You'll need to provide a fix for the suppressed
initialization instead, at least for Dom0. (As an aside, your patch
orphans hwdom_pit_access().)

Jan

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Jan Beulich]

>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
> server, otherwise it makes no sense since the only code that uses the value
> stored by hvm_access_cf8 is the IOREQ server.

Afaict an ioreq server could also attach subsequently - Paul?

Jan

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Andrew Cooper]

On 13/01/16 16:38, Jan Beulich wrote:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>> server, otherwise it makes no sense since the only code that uses the value
>> stored by hvm_access_cf8 is the IOREQ server.
> Afaict an ioreq server could also attach subsequently - Paul?

Indeed it can.

The last-latched cfc/cf8 values should be accurate at that point.  The
intercept needs to stay.

~Andrew

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 January 2016 16:39
> To: Paul Durrant; Roger Pau Monne
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
> port 0xcf8 unconditionally
> 
> >>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> > Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
> > server, otherwise it makes no sense since the only code that uses the value
> > stored by hvm_access_cf8 is the IOREQ server.
> 
> Afaict an ioreq server could also attach subsequently - Paul?
> 

Indeed, ioreq servers can come and go at any time.

  Paul

> Jan

Re: [PATCH 3/5] x86/hvm: don't set the BSP as initialised in hvm_vcpu_initialise

[Roger Pau Monné]

El 13/01/16 a les 17.29, Jan Beulich ha escrit:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> 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.
> 
> But the comment that you remove says explicitly that this isn't a
> requirement.

Previous to my HVMlite series that was true, and 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. I believe this is better, and this change helps enforce
this policy.

Roger.

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Roger Pau Monné]

El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> The HVMlite series removed the initialization of the emulated PIT for PV
>> guests, but the handler was still reachable, which means a PV guests can
>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>> PV PIT handler and move the PIT initialization to HVM guests only.
> 
> As said on IRC - this is needed for Dom0 to be able to drive the
> PC speaker. You'll need to provide a fix for the suppressed
> initialization instead, at least for Dom0. (As an aside, your patch
> orphans hwdom_pit_access().)

Thanks for the clarification. AFAICT I can leave the usage of
hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
that right?

Roger.

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Roger Pau Monné]

El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 January 2016 16:39
>> To: Paul Durrant; Roger Pau Monne
>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>> port 0xcf8 unconditionally
>>
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>> server, otherwise it makes no sense since the only code that uses the value
>>> stored by hvm_access_cf8 is the IOREQ server.
>>
>> Afaict an ioreq server could also attach subsequently - Paul?
>>
> 
> Indeed, ioreq servers can come and go at any time.

Right, then I will have to prevent hvm_access_cf8 from being added if
the domain is the hardware domain, otherwise it overlaps with the Dom0
passthrough intercept (handle_pvh_io).

Roger.

Re: [PATCH 3/5] x86/hvm: don't set the BSP as initialised in hvm_vcpu_initialise

[Jan Beulich]

>>> On 13.01.16 at 18:23, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 17.29, Jan Beulich ha escrit:
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> 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.
>> 
>> But the comment that you remove says explicitly that this isn't a
>> requirement.
> 
> Previous to my HVMlite series that was true, and 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. I believe this is better, and this change helps enforce
> this policy.

Well, that's fine, but the patch description then needs to reflect
this.

Jan

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Jan Beulich]

>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>> guests, but the handler was still reachable, which means a PV guests can
>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>> PV PIT handler and move the PIT initialization to HVM guests only.
>> 
>> As said on IRC - this is needed for Dom0 to be able to drive the
>> PC speaker. You'll need to provide a fix for the suppressed
>> initialization instead, at least for Dom0. (As an aside, your patch
>> orphans hwdom_pit_access().)
> 
> Thanks for the clarification. AFAICT I can leave the usage of
> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
> that right?

I don't think so - see the explanation Tim gave on IRC. Afaict the
mention of BIOS here isn't related to a virtual BIOS, but to that
of a passed through graphics card.

Jan

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Jan Beulich]

>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 13 January 2016 16:39
>>> To: Paul Durrant; Roger Pau Monne
>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>> port 0xcf8 unconditionally
>>>
>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>> server, otherwise it makes no sense since the only code that uses the value
>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>
>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>
>> 
>> Indeed, ioreq servers can come and go at any time.
> 
> Right, then I will have to prevent hvm_access_cf8 from being added if
> the domain is the hardware domain, otherwise it overlaps with the Dom0
> passthrough intercept (handle_pvh_io).

Yes, that indeed makes sense.

Jan

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Andrew Cooper]

On 14/01/16 09:12, Jan Beulich wrote:
>>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
>> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 13 January 2016 16:39
>>>> To: Paul Durrant; Roger Pau Monne
>>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>>> port 0xcf8 unconditionally
>>>>
>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>>> server, otherwise it makes no sense since the only code that uses the value
>>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>>
>>> Indeed, ioreq servers can come and go at any time.
>> Right, then I will have to prevent hvm_access_cf8 from being added if
>> the domain is the hardware domain, otherwise it overlaps with the Dom0
>> passthrough intercept (handle_pvh_io).
> Yes, that indeed makes sense.

Even for the hardware domain, cf8 needs trapping and emulating (although
on a different path).  Being an indirect port pair shared by Xen and
dom0, dom0 cannot use it directly of its own accord.

~Andrew

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Roger Pau Monné]

El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>> guests, but the handler was still reachable, which means a PV guests can
>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>
>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>> PC speaker. You'll need to provide a fix for the suppressed
>>> initialization instead, at least for Dom0. (As an aside, your patch
>>> orphans hwdom_pit_access().)
>>
>> Thanks for the clarification. AFAICT I can leave the usage of
>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>> that right?
> 
> I don't think so - see the explanation Tim gave on IRC. Afaict the
> mention of BIOS here isn't related to a virtual BIOS, but to that
> of a passed through graphics card.

I'm sorry but I still don't fully understand why that's needed, and it
arises a couple of questions. First of all, the only reference that I
can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
where they mention that the VGA POST method might make use of the i8254.

This seems reasonable, but I still don't understand why we provide an
emulated i8254 to DomUs. They don't have access to the low 1MB, which is
where the VGA BIOS resides, so there's no way they can call into the VGA
POST at all.

Also, allowing a VGA BIOS access to the i8254 used by the OS seems like
asking for trouble, so that same paper [0] describes that they provide
an emulated i8254 for the VGA BIOS to use (because they run the VGA BIOS
code inside x86emu), in order to prevent it from messing with the OS
setup. I have to admit that greatly depends on whether the OS makes use
of the i8254 or not.

Anyway, I would be in favour of adding an emulated i8254 to the hardware
domain (and let it make use of the PC speaker), but I don't see any
reason to provide it to DomUs.

Roger.

[0] https://2008.asiabsdcon.org/papers/P9A-paper.pdf

Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally

[Roger Pau Monné]

El 14/01/16 a les 11.50, Andrew Cooper ha escrit:
> On 14/01/16 09:12, Jan Beulich wrote:
>>>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
>>> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>>>> -----Original Message-----
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: 13 January 2016 16:39
>>>>> To: Paul Durrant; Roger Pau Monne
>>>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>>>> port 0xcf8 unconditionally
>>>>>
>>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>>>> server, otherwise it makes no sense since the only code that uses the value
>>>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>>>
>>>> Indeed, ioreq servers can come and go at any time.
>>> Right, then I will have to prevent hvm_access_cf8 from being added if
>>> the domain is the hardware domain, otherwise it overlaps with the Dom0
>>> passthrough intercept (handle_pvh_io).
>> Yes, that indeed makes sense.
> 
> Even for the hardware domain, cf8 needs trapping and emulating (although
> on a different path).  Being an indirect port pair shared by Xen and
> dom0, dom0 cannot use it directly of its own accord.

Sure, but it has to be handled by handle_pvh_io (which I plan to rename
in due time) and not hvm_access_cf8.

Roger.

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Jan Beulich]

>>> On 14.01.16 at 11:59, <roger.pau@citrix.com> wrote:
> El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>>> guests, but the handler was still reachable, which means a PV guests can
>>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>>
>>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>>> PC speaker. You'll need to provide a fix for the suppressed
>>>> initialization instead, at least for Dom0. (As an aside, your patch
>>>> orphans hwdom_pit_access().)
>>>
>>> Thanks for the clarification. AFAICT I can leave the usage of
>>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>>> that right?
>> 
>> I don't think so - see the explanation Tim gave on IRC. Afaict the
>> mention of BIOS here isn't related to a virtual BIOS, but to that
>> of a passed through graphics card.
> 
> I'm sorry but I still don't fully understand why that's needed, and it
> arises a couple of questions. First of all, the only reference that I
> can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
> where they mention that the VGA POST method might make use of the i8254.
> 
> This seems reasonable, but I still don't understand why we provide an
> emulated i8254 to DomUs. They don't have access to the low 1MB, which is
> where the VGA BIOS resides, so there's no way they can call into the VGA
> POST at all.

All of this arrangement predates me, but see the original change
introducing this: "Provide PV guests with emulated PIT", which
suggests this wasn't just for Dom0. I'm hesitant to accept removal
of code when we don't know exactly by whom and for what purpose
it might be used. When I enabled Dom0 speaker control, I
intentionally retained the original code for DomU purposes.

Jan

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Roger Pau Monné]

El 14/01/16 a les 13.38, Jan Beulich ha escrit:
>>>> On 14.01.16 at 11:59, <roger.pau@citrix.com> wrote:
>> El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>>>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>>>> guests, but the handler was still reachable, which means a PV guests can
>>>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>>>
>>>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>>>> PC speaker. You'll need to provide a fix for the suppressed
>>>>> initialization instead, at least for Dom0. (As an aside, your patch
>>>>> orphans hwdom_pit_access().)
>>>>
>>>> Thanks for the clarification. AFAICT I can leave the usage of
>>>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>>>> that right?
>>>
>>> I don't think so - see the explanation Tim gave on IRC. Afaict the
>>> mention of BIOS here isn't related to a virtual BIOS, but to that
>>> of a passed through graphics card.
>>
>> I'm sorry but I still don't fully understand why that's needed, and it
>> arises a couple of questions. First of all, the only reference that I
>> can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
>> where they mention that the VGA POST method might make use of the i8254.
>>
>> This seems reasonable, but I still don't understand why we provide an
>> emulated i8254 to DomUs. They don't have access to the low 1MB, which is
>> where the VGA BIOS resides, so there's no way they can call into the VGA
>> POST at all.
> 
> All of this arrangement predates me, but see the original change
> introducing this: "Provide PV guests with emulated PIT", which
> suggests this wasn't just for Dom0. I'm hesitant to accept removal
> of code when we don't know exactly by whom and for what purpose
> it might be used. When I enabled Dom0 speaker control, I
> intentionally retained the original code for DomU purposes.

What about we do the following: enable the PIT for PV(H) guests
(DomU/Dom0), and completely remove it for HVMlite guests for the moment?

We might consider enabling it for HVMlite, but the plan is that this
could be done on a per-domain basis using the flags in the
xen_arch_domainconfig struct.

Roger.

Re: [PATCH 4/5] x86/PV: remove the emulated PIT

[Jan Beulich]

>>> On 14.01.16 at 15:03, <roger.pau@citrix.com> wrote:
> What about we do the following: enable the PIT for PV(H) guests
> (DomU/Dom0), and completely remove it for HVMlite guests for the moment?
> 
> We might consider enabling it for HVMlite, but the plan is that this
> could be done on a per-domain basis using the flags in the
> xen_arch_domainconfig struct.

That sounds okay.

Jan