[PATCH 0/9] Fix and improvements to pvshim

[PATCH 0/9] Fix and improvements to pvshim

[Wei Liu]

First batch of patches to fix problems I know of.

Wei Liu (9):
  Update shim.config
  libxl: remove whitespaces introduced in 62982da926
  x86/guest: clean up guest/xen.h
  Remove sched=null from shim cmdline and doc
  x86: relocate pvh_info
  Revert "x86/boot: Map more than the first 16MB"
  libxl: lower shim related message to level info
  xen/consoled: discard NUL from guest
  x86/shim: pass through vcpu runstate to L0 Xen

 docs/man/xl.cfg.pod.5.in              |  2 +-
 tools/firmware/xen-dir/shim.config    |  1 -
 tools/libxl/libxl_dom.c               |  8 +++---
 tools/libxl/libxl_internal.h          |  2 +-
 xen/arch/x86/boot/Makefile            |  7 ++++-
 xen/arch/x86/boot/build32.mk          |  3 ++
 xen/arch/x86/boot/defs.h              |  3 ++
 xen/arch/x86/boot/head.S              | 25 +++++++++--------
 xen/arch/x86/boot/reloc.c             | 53 +++++++++++++++++++++++++++++++----
 xen/arch/x86/boot/x86_64.S            |  3 +-
 xen/common/domain.c                   | 10 +++++++
 xen/drivers/char/consoled.c           |  4 ++-
 xen/include/asm-x86/guest/hypercall.h |  7 +++++
 xen/include/asm-x86/guest/xen.h       | 12 +++++---
 14 files changed, 109 insertions(+), 31 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/9] Update shim.config

[Wei Liu]

To avoid having it changed every time the shim is built.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/firmware/xen-dir/shim.config | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/firmware/xen-dir/shim.config b/tools/firmware/xen-dir/shim.config
index 78b965f4c7..d5bd516632 100644
--- a/tools/firmware/xen-dir/shim.config
+++ b/tools/firmware/xen-dir/shim.config
@@ -68,7 +68,6 @@ CONFIG_HAS_EHCI=y
 CONFIG_HAS_CPUFREQ=y
 CONFIG_HAS_PASSTHROUGH=y
 CONFIG_HAS_PCI=y
-# CONFIG_VGA is not set
 CONFIG_DEFCONFIG_LIST="$ARCH_DEFCONFIG"
 CONFIG_ARCH_SUPPORTS_INT128=y
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/9] libxl: remove whitespaces introduced in 62982da926

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_dom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index b03386409f..e1a3e747fc 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1069,7 +1069,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
                 }
             }
         }
-        
+
         if (state->pv_ramdisk.path && strlen(state->pv_ramdisk.path)) {
             if (state->pv_ramdisk.mapped) {
                 rc = xc_dom_module_mem(dom, state->pv_ramdisk.data,
@@ -1183,7 +1183,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 
     xc_dom_loginit(ctx->xch);
 
-    /* 
+    /*
      * If PVH and we have a shim override, use the shim cmdline.
      * If PVH and no shim override, use the pv cmdline.
      * If not PVH, use info->cmdline.
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/9] x86/guest: clean up guest/xen.h

[Wei Liu]

Remove extraneous semicolon. Add blank lines.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/include/asm-x86/guest/xen.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
index 11243fe60d..31326442f7 100644
--- a/xen/include/asm-x86/guest/xen.h
+++ b/xen/include/asm-x86/guest/xen.h
@@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
 #define xen_guest 0
 #define pv_console 0
 
-static inline void probe_hypervisor(void) {};
+static inline void probe_hypervisor(void) {}
+
 static inline void hypervisor_setup(void)
 {
     ASSERT_UNREACHABLE();
@@ -63,20 +64,23 @@ static inline void hypervisor_fixup_e820(struct e820map *e820)
 {
     ASSERT_UNREACHABLE();
 }
+
 static inline const unsigned long *hypervisor_reserved_pages(unsigned int *size)
 {
     ASSERT_UNREACHABLE();
     return NULL;
-};
+}
+
 static inline uint32_t hypervisor_cpuid_base(void)
 {
     ASSERT_UNREACHABLE();
     return 0;
-};
+}
+
 static inline void hypervisor_resume(void)
 {
     ASSERT_UNREACHABLE();
-};
+}
 
 #endif /* CONFIG_XEN_GUEST */
 #endif /* __X86_GUEST_XEN_H__ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/9] Remove sched=null from shim cmdline and doc

[Wei Liu]

We use the default scheduler (credit1 as of writing). The NULL
scheduler still has bugs to fix.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 docs/man/xl.cfg.pod.5.in     | 2 +-
 tools/libxl/libxl_internal.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 30fe4b8531..a699367779 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -531,7 +531,7 @@ Ignored if pvhsim is false.
 =item B<pvshim_cmdline="STRING">
 
 Command line for the shim.
-Default is "pv-shim console=xen,pv sched=null".
+Default is "pv-shim console=xen,pv".
 Ignored if pvhsim is false.
 
 =item B<pvshim_extra="STRING">
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0f89364466..7ff9a67e50 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -119,7 +119,7 @@
 #define DOMID_XS_PATH "domid"
 #define INVALID_DOMID ~0
 #define PVSHIM_BASENAME "xen-shim"
-#define PVSHIM_CMDLINE "pv-shim console=xen,pv sched=null"
+#define PVSHIM_CMDLINE "pv-shim console=xen,pv"
 
 /* Size macros. */
 #define __AC(X,Y)   (X##Y)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/9] x86: relocate pvh_info

[Wei Liu]

Modify early boot code to relocate pvh info as well, so that we can be
sure __va in __start_xen works.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/boot/Makefile   |  7 +++++-
 xen/arch/x86/boot/build32.mk |  3 +++
 xen/arch/x86/boot/defs.h     |  3 +++
 xen/arch/x86/boot/head.S     | 25 ++++++++++++---------
 xen/arch/x86/boot/reloc.c    | 53 +++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 73 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index c6246c85d2..9fe5b309c5 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -7,10 +7,15 @@ CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
 RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
 	     $(BASEDIR)/include/xen/multiboot2.h
 
+ifeq ($(CONFIG_PVH_GUEST),y)
+RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
+RELOC_EXTRA = CONFIG_PVH_GUEST=y
+endif
+
 head.o: cmdline.S reloc.S
 
 cmdline.S: cmdline.c $(CMDLINE_DEPS)
 	$(MAKE) -f build32.mk $@ CMDLINE_DEPS="$(CMDLINE_DEPS)"
 
 reloc.S: reloc.c $(RELOC_DEPS)
-	$(MAKE) -f build32.mk $@ RELOC_DEPS="$(RELOC_DEPS)"
+	$(MAKE) -f build32.mk $@ RELOC_DEPS="$(RELOC_DEPS)" $(RELOC_EXTRA)
diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
index 48c7407c00..028ac19b96 100644
--- a/xen/arch/x86/boot/build32.mk
+++ b/xen/arch/x86/boot/build32.mk
@@ -36,5 +36,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
 cmdline.o: cmdline.c $(CMDLINE_DEPS)
 
 reloc.o: reloc.c $(RELOC_DEPS)
+ifeq ($(CONFIG_PVH_GUEST),y)
+reloc.o: CFLAGS += -DCONFIG_PVH_GUEST
+endif
 
 .PRECIOUS: %.bin %.lnk
diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
index 6abdc15446..05921a64a3 100644
--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -51,6 +51,9 @@ typedef unsigned short u16;
 typedef unsigned int u32;
 typedef unsigned long long u64;
 typedef unsigned int size_t;
+typedef u8 uint8_t;
+typedef u32 uint32_t;
+typedef u64 uint64_t;
 
 #define U16_MAX		((u16)(~0U))
 #define UINT_MAX	(~0U)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0f652cea11..2f94c286d5 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -414,6 +414,7 @@ __pvh_start:
 
         /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
         movw    $0x1000, sym_esi(trampoline_phys)
+        xor     %eax, %eax /* Needed by reloc.c */
         jmp     trampoline_setup
 
 #endif /* CONFIG_PVH_GUEST */
@@ -578,18 +579,20 @@ trampoline_setup:
         /* Get bottom-most low-memory stack address. */
         add     $TRAMPOLINE_SPACE,%ecx
 
-#ifdef CONFIG_PVH_GUEST
-        cmpb    $0, sym_fs(pvh_boot)
-        jne     1f
-#endif
-
-        /* Save the Multiboot info struct (after relocation) for later use. */
+        /* Save Multiboot / PVH info struct (after relocation) for later use. */
         push    %ecx                /* Bottom-most low-memory stack address. */
-        push    %ebx                /* Multiboot information address. */
-        push    %eax                /* Multiboot magic. */
+        push    %ebx                /* Multiboot / PVH information address. */
+        push    %eax                /* Magic number. */
         call    reloc
-        mov     %eax,sym_fs(multiboot_ptr)
+#ifdef CONFIG_PVH_GUEST
+        cmp     $0,sym_fs(pvh_boot)
+        je      1f
+        mov     %eax,sym_fs(pvh_start_info_pa)
+        jmp     2f
+#endif
 1:
+        mov     %eax,sym_fs(multiboot_ptr)
+2:
 
         /*
          * Now trampoline_phys points to the following structure (lowest address
@@ -598,12 +601,12 @@ trampoline_setup:
          * +------------------------+
          * | TRAMPOLINE_STACK_SPACE |
          * +------------------------+
-         * |        mbi data        |
+         * |     Data (MBI / PVH)   |
          * +- - - - - - - - - - - - +
          * |    TRAMPOLINE_SPACE    |
          * +------------------------+
          *
-         * mbi data grows downwards from the highest address of TRAMPOLINE_SPACE
+         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
          * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
          * reserved for trampoline code and data.
          */
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index b992678b5e..b01e148772 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -14,8 +14,8 @@
 
 /*
  * This entry point is entered from xen/arch/x86/boot/head.S with:
- *   - 0x4(%esp) = MULTIBOOT_MAGIC,
- *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
+ *   - 0x4(%esp) = MAGIC,
+ *   - 0x8(%esp) = INFORMATION_ADDRESS,
  *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
  */
 asm (
@@ -29,6 +29,10 @@ asm (
 #include "../../../include/xen/multiboot.h"
 #include "../../../include/xen/multiboot2.h"
 
+#ifdef CONFIG_PVH_GUEST
+#include <public/arch-x86/hvm/start_info.h>
+#endif
+
 #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
 #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
 
@@ -71,6 +75,38 @@ static u32 copy_string(u32 src)
     return copy_mem(src, p - src + 1);
 }
 
+#ifdef CONFIG_PVH_GUEST
+static struct hvm_start_info *pvh_info_reloc(u32 in)
+{
+    struct hvm_start_info *out;
+
+    out = _p(copy_mem(in, sizeof(*out)));
+
+    if ( out->cmdline_paddr )
+        out->cmdline_paddr = copy_string(out->cmdline_paddr);
+
+    if ( out->nr_modules )
+    {
+        unsigned int i;
+        struct hvm_modlist_entry *mods;
+
+        out->modlist_paddr =
+            copy_mem(out->modlist_paddr,
+                     out->nr_modules * sizeof(struct hvm_modlist_entry));
+
+        mods = _p(out->modlist_paddr);
+
+        for ( i = 0; i < out->nr_modules; i++ )
+        {
+            if ( mods[i].cmdline_paddr )
+                mods[i].cmdline_paddr = copy_string(mods[i].cmdline_paddr);
+        }
+    }
+
+    return out;
+}
+#endif
+
 static multiboot_info_t *mbi_reloc(u32 mbi_in)
 {
     int i;
@@ -226,14 +262,19 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
     return mbi_out;
 }
 
-multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
+void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
 {
     alloc = trampoline;
 
-    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
-        return mbi2_reloc(mbi_in);
+#ifdef CONFIG_PVH_GUEST
+    if ( magic == 0 )
+        return pvh_info_reloc(in);
+    else
+#endif
+    if ( magic == MULTIBOOT2_BOOTLOADER_MAGIC )
+        return mbi2_reloc(in);
     else
-        return mbi_reloc(mbi_in);
+        return mbi_reloc(in);
 }
 
 /*
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/9] Revert "x86/boot: Map more than the first 16MB"

[Wei Liu]

This reverts commit 7d6f958d9d18c54017f5ef6e299a08037f035747.

Now we have PVH info relocation support, this change is no longer
needed.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/boot/x86_64.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 42636cf334..cf47e019f5 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -114,10 +114,11 @@ GLOBAL(__page_tables_start)
 GLOBAL(l2_identmap)
         .quad sym_offs(l1_identmap) + __PAGE_HYPERVISOR
         idx = 1
-        .rept 4 * L2_PAGETABLE_ENTRIES - 1
+        .rept 7
         .quad (idx << L2_PAGETABLE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
         idx = idx + 1
         .endr
+        .fill 4 * L2_PAGETABLE_ENTRIES - 8, 8, 0
         .size l2_identmap, . - l2_identmap
 
 /*
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 7/9] libxl: lower shim related message to level info

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_dom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e1a3e747fc..b880341d7e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1035,7 +1035,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
 
             /* We've loaded the shim, so load the kernel as a secondary module */
             if (state->pv_kernel.mapped) {
-                LOG(WARN, "xc_dom_module_mem, cmdline %s",
+                LOG(INFO, "xc_dom_module_mem, cmdline %s",
                     state->pv_cmdline);
                 rc = xc_dom_module_mem(dom, state->pv_kernel.data,
                                        state->pv_kernel.size, state->pv_cmdline);
@@ -1044,7 +1044,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
                     goto out;
                 }
             } else {
-                LOG(WARN, "xc_dom_module_file, path %s cmdline %s",
+                LOG(INFO, "xc_dom_module_file, path %s cmdline %s",
                     state->pv_kernel.path, state->pv_cmdline);
                 rc = xc_dom_module_file(dom, state->pv_kernel.path, state->pv_cmdline);
                 if (rc) {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 8/9] xen/consoled: discard NUL from guest

[Wei Liu]

According to [0], some program sends NUL for padding purpose. We can
discard them.

https://www.gnu.org/software/termutils/manual/termcap-1.3/html_mono/termcap.html#SEC7

Reported-by: Sarah Newman <srn@prgmr.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Sarah Newman <srn@prgmr.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

A bit RFC. Awaiting test results.
---
 xen/drivers/char/consoled.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
index 552abf5766..6fcb2aa115 100644
--- a/xen/drivers/char/consoled.c
+++ b/xen/drivers/char/consoled.c
@@ -72,7 +72,9 @@ size_t consoled_guest_rx(void)
     {
         char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
 
-        buf[idx++] = c;
+        /* Discard NUL. See the "Padding" section of termcap manual. */
+        if ( c != '\0' )
+            buf[idx++] = c;
         recv++;
 
         if ( idx >= BUF_SZ )
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 9/9] x86/shim: pass through vcpu runstate to L0 Xen

[Wei Liu]

So that we can have accurate stolen time accounting. Idea taken from
Vixen work from Amazon.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/common/domain.c                   | 10 ++++++++++
 xen/include/asm-x86/guest/hypercall.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 558318e852..189ffac9b1 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -45,6 +45,7 @@
 
 #ifdef CONFIG_X86
 #include <asm/guest.h>
+#include <asm/guest/hypercall.h>
 #endif
 
 /* Linux config option: propageted to domain0 */
@@ -1425,6 +1426,15 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( !guest_handle_okay(area.addr.h, 1) )
             break;
 
+#if CONFIG_X86
+        if ( pv_shim )
+        {
+            rc = xen_hypercall_vcpu_op(VCPUOP_register_runstate_memory_area,
+                                       vcpuid, &area);
+            break;
+        }
+#endif
+
         rc = 0;
         runstate_guest(v) = area.addr.h;
 
diff --git a/xen/include/asm-x86/guest/hypercall.h b/xen/include/asm-x86/guest/hypercall.h
index e9e626b474..07b9302263 100644
--- a/xen/include/asm-x86/guest/hypercall.h
+++ b/xen/include/asm-x86/guest/hypercall.h
@@ -192,6 +192,13 @@ static inline long xen_hypercall_shutdown(unsigned int reason)
     return 0;
 }
 
+static inline int xen_hypercall_vcpu_op(unsigned int cmd, unsigned int vcpu,
+                                        void *arg)
+{
+    ASSERT_UNREACHABLE();
+    return 0;
+}
+
 #endif /* CONFIG_XEN_GUEST */
 #endif /* __X86_XEN_HYPERCALL_H__ */
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/9] Update shim.config

[Andrew Cooper]

On 18/01/18 18:16, Wei Liu wrote:
> To avoid having it changed every time the shim is built.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

One downside of having a this config in-tree is that it needs
regenerating every time the Kconfig logic changes.

OTOH, I did deliberately wire up all the config options, so

make -C tools/firmware/xen-dir shim-olddefconfig

or shim-menuconfig etc do work as expected.

Do we know why this line is disappearing?  I presume it is some change
in the Kconfig logic, but I can't spot which changeset.

~Andrew

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/firmware/xen-dir/shim.config | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tools/firmware/xen-dir/shim.config b/tools/firmware/xen-dir/shim.config
> index 78b965f4c7..d5bd516632 100644
> --- a/tools/firmware/xen-dir/shim.config
> +++ b/tools/firmware/xen-dir/shim.config
> @@ -68,7 +68,6 @@ CONFIG_HAS_EHCI=y
>  CONFIG_HAS_CPUFREQ=y
>  CONFIG_HAS_PASSTHROUGH=y
>  CONFIG_HAS_PCI=y
> -# CONFIG_VGA is not set
>  CONFIG_DEFCONFIG_LIST="$ARCH_DEFCONFIG"
>  CONFIG_ARCH_SUPPORTS_INT128=y
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/9] x86/guest: clean up guest/xen.h

[Andrew Cooper]

On 18/01/18 18:16, Wei Liu wrote:
> Remove extraneous semicolon. Add blank lines.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/9] Remove sched=null from shim cmdline and doc

[Andrew Cooper]

On 18/01/18 18:16, Wei Liu wrote:
> We use the default scheduler (credit1 as of writing). The NULL
> scheduler still has bugs to fix.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/9] Update shim.config

[Wei Liu]

On Thu, Jan 18, 2018 at 06:26:02PM +0000, Andrew Cooper wrote:
> On 18/01/18 18:16, Wei Liu wrote:
> > To avoid having it changed every time the shim is built.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> One downside of having a this config in-tree is that it needs
> regenerating every time the Kconfig logic changes.
> 
> OTOH, I did deliberately wire up all the config options, so
> 
> make -C tools/firmware/xen-dir shim-olddefconfig
> 
> or shim-menuconfig etc do work as expected.
> 
> Do we know why this line is disappearing?  I presume it is some change
> in the Kconfig logic, but I can't spot which changeset.

I guess that's due to "xen/x86: make VGA support selectable".

The shim.config probably is generated before that patch.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/9] Update shim.config

[Andrew Cooper]

On 18/01/18 18:28, Wei Liu wrote:
> On Thu, Jan 18, 2018 at 06:26:02PM +0000, Andrew Cooper wrote:
>> On 18/01/18 18:16, Wei Liu wrote:
>>> To avoid having it changed every time the shim is built.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> One downside of having a this config in-tree is that it needs
>> regenerating every time the Kconfig logic changes.
>>
>> OTOH, I did deliberately wire up all the config options, so
>>
>> make -C tools/firmware/xen-dir shim-olddefconfig
>>
>> or shim-menuconfig etc do work as expected.
>>
>> Do we know why this line is disappearing?  I presume it is some change
>> in the Kconfig logic, but I can't spot which changeset.
> I guess that's due to "xen/x86: make VGA support selectable".
>
> The shim.config probably is generated before that patch.

Can you identify that in the commit message?  With that, Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] x86: relocate pvh_info

[Jan Beulich]

>>> On 18.01.18 at 19:16, <wei.liu2@citrix.com> wrote:
> Modify early boot code to relocate pvh info as well, so that we can be
> sure __va in __start_xen works.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one cosmetic request:

> @@ -29,6 +29,10 @@ asm (
>  #include "../../../include/xen/multiboot.h"
>  #include "../../../include/xen/multiboot2.h"
>  
> +#ifdef CONFIG_PVH_GUEST
> +#include <public/arch-x86/hvm/start_info.h>
> +#endif

Would you mind moving the #include ...

> @@ -71,6 +75,38 @@ static u32 copy_string(u32 src)
>      return copy_mem(src, p - src + 1);
>  }
>  
> +#ifdef CONFIG_PVH_GUEST
> +static struct hvm_start_info *pvh_info_reloc(u32 in)

... above this line, to avoid the duplicate #ifdef?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/9] Revert "x86/boot: Map more than the first 16MB"

[Jan Beulich]

>>> On 18.01.18 at 19:16, <wei.liu2@citrix.com> wrote:
> This reverts commit 7d6f958d9d18c54017f5ef6e299a08037f035747.
> 
> Now we have PVH info relocation support, this change is no longer
> needed.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] x86: relocate pvh_info

[Andrew Cooper]

On 18/01/18 18:16, Wei Liu wrote:
> Modify early boot code to relocate pvh info as well, so that we can be
> sure __va in __start_xen works.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

:(

It was a fear of code like this which caused me to go with the gross
hack in the short term.

Let me see if I can clean it up somewhat.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/9] Update shim.config

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:30:05PM +0000, Andrew Cooper wrote:
> On 18/01/18 18:28, Wei Liu wrote:
> > On Thu, Jan 18, 2018 at 06:26:02PM +0000, Andrew Cooper wrote:
> >> On 18/01/18 18:16, Wei Liu wrote:
> >>> To avoid having it changed every time the shim is built.
> >>>
> >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> One downside of having a this config in-tree is that it needs
> >> regenerating every time the Kconfig logic changes.
> >>
> >> OTOH, I did deliberately wire up all the config options, so
> >>
> >> make -C tools/firmware/xen-dir shim-olddefconfig
> >>
> >> or shim-menuconfig etc do work as expected.
> >>
> >> Do we know why this line is disappearing?  I presume it is some change
> >> in the Kconfig logic, but I can't spot which changeset.
> > I guess that's due to "xen/x86: make VGA support selectable".
> >
> > The shim.config probably is generated before that patch.
> 
> Can you identify that in the commit message?  With that, Reviewed-by:
> Andrew Cooper <andrew.cooper3@citrix.com>

AFAICT this is because the Kconfig option for VGA contains:

bool "VGA support" if !PV_SHIM_EXCLUSIVE

So if PV_SHIM_EXCLUSIVE is selected this option is not even available.

With that fixed in the commit message:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Although possibly having to change this file every time there's a
Kconfig change seems very annoying (and easy to miss).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/9] libxl: remove whitespaces introduced in 62982da926

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:45PM +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would add to the commit message: "No functional change.", but I guess
it's already clear from the subject.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] x86: relocate pvh_info

[Jan Beulich]

>>> On 19.01.18 at 11:10, <andrew.cooper3@citrix.com> wrote:
> On 18/01/18 18:16, Wei Liu wrote:
>> Modify early boot code to relocate pvh info as well, so that we can be
>> sure __va in __start_xen works.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> :(
> 
> It was a fear of code like this which caused me to go with the gross
> hack in the short term.
> 
> Let me see if I can clean it up somewhat.

Is it really that bad, i.e. that much worse than the other relocation
code?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/9] x86/guest: clean up guest/xen.h

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:46PM +0000, Wei Liu wrote:
> Remove extraneous semicolon. Add blank lines.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/include/asm-x86/guest/xen.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
> index 11243fe60d..31326442f7 100644
> --- a/xen/include/asm-x86/guest/xen.h
> +++ b/xen/include/asm-x86/guest/xen.h
> @@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
>  #define xen_guest 0
>  #define pv_console 0
>  
> -static inline void probe_hypervisor(void) {};
> +static inline void probe_hypervisor(void) {}
> +
>  static inline void hypervisor_setup(void)
>  {
>      ASSERT_UNREACHABLE();
> @@ -63,20 +64,23 @@ static inline void hypervisor_fixup_e820(struct e820map *e820)
>  {
>      ASSERT_UNREACHABLE();
>  }
> +
>  static inline const unsigned long *hypervisor_reserved_pages(unsigned int *size)
>  {
>      ASSERT_UNREACHABLE();
>      return NULL;
> -};
> +}
> +
>  static inline uint32_t hypervisor_cpuid_base(void)
>  {
>      ASSERT_UNREACHABLE();
>      return 0;
> -};
> +}
> +
>  static inline void hypervisor_resume(void)
>  {
>      ASSERT_UNREACHABLE();
> -};
> +}

AFAICT hypervisor_cpuid_base and hypervisor_resume could be removed,
because those are only called from shim code, and it's not even
possible to compile the shim code without having Xen guest support
enabled.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/9] Remove sched=null from shim cmdline and doc

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:47PM +0000, Wei Liu wrote:
> We use the default scheduler (credit1 as of writing). The NULL
> scheduler still has bugs to fix.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

This is AFAICT missing the change to shim.config, because the shim in
staging is still being built with:

CONFIG_SCHED_NULL_DEFAULT=y
CONFIG_SCHED_DEFAULT="null"

This however doesn't seem to be the case for the 4.10.0-shim-comet
branch, where it's built with:

CONFIG_SCHED_CREDIT_DEFAULT=y
CONFIG_SCHED_DEFAULT="credit"

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] x86: relocate pvh_info

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:48PM +0000, Wei Liu wrote:
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 0f652cea11..2f94c286d5 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -414,6 +414,7 @@ __pvh_start:
>  
>          /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
>          movw    $0x1000, sym_esi(trampoline_phys)
> +        xor     %eax, %eax /* Needed by reloc.c */

I would rather do:

movl $XEN_HVM_START_MAGIC_VALUE,%eax

Or:

mov (%ebx),%eax

So that...

> -multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
> +void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
>  {
>      alloc = trampoline;
>  
> -    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
> -        return mbi2_reloc(mbi_in);
> +#ifdef CONFIG_PVH_GUEST
> +    if ( magic == 0 )

... here you can do magic == XEN_HVM_START_MAGIC_VALUE.

> +        return pvh_info_reloc(in);
> +    else
> +#endif
> +    if ( magic == MULTIBOOT2_BOOTLOADER_MAGIC )
> +        return mbi2_reloc(in);
>      else
> -        return mbi_reloc(mbi_in);
> +        return mbi_reloc(in);

A 'switch ( magic )' would be better here IMHO.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/9] libxl: lower shim related message to level info

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:50PM +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would even make them DEBUG in fact. Feel free to keep the RB if you
turn them into DEBUG.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/9] xen/consoled: discard NUL from guest

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:51PM +0000, Wei Liu wrote:
> According to [0], some program sends NUL for padding purpose. We can
> discard them.
> 
> https://www.gnu.org/software/termutils/manual/termcap-1.3/html_mono/termcap.html#SEC7
> 
> Reported-by: Sarah Newman <srn@prgmr.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Although I have a suggestion below.

> ---
> Cc: Sarah Newman <srn@prgmr.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> A bit RFC. Awaiting test results.
> ---
>  xen/drivers/char/consoled.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> index 552abf5766..6fcb2aa115 100644
> --- a/xen/drivers/char/consoled.c
> +++ b/xen/drivers/char/consoled.c
> @@ -72,7 +72,9 @@ size_t consoled_guest_rx(void)
>      {
>          char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];

You could also do:

if ( c == '\0' )
    continue;

recv AFAICT it's just a cosmetic return value that's not consumed by
the caller. This avoids checking the "idx >= BUF_SZ" condition with
the same idx value.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 9/9] x86/shim: pass through vcpu runstate to L0 Xen

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 06:16:52PM +0000, Wei Liu wrote:
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 558318e852..189ffac9b1 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -45,6 +45,7 @@
>  
>  #ifdef CONFIG_X86
>  #include <asm/guest.h>
> +#include <asm/guest/hypercall.h>
>  #endif
>  
>  /* Linux config option: propageted to domain0 */
> @@ -1425,6 +1426,15 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( !guest_handle_okay(area.addr.h, 1) )
>              break;
>  
> +#if CONFIG_X86
> +        if ( pv_shim )
> +        {
> +            rc = xen_hypercall_vcpu_op(VCPUOP_register_runstate_memory_area,
> +                                       vcpuid, &area);
> +            break;
> +        }
> +#endif

This only fixes VCPUOP_register_runstate_memory_area, but
VCPUOP_get_runstate_info will still report wrong information. I've
also wondered whether simply returning L0 information is correct. To
get the exact information the shim should actually return the L0
information plus whatever time it steals from the guest.

Also, before adding more hooks to do_vcpu_op I would attempt to add a
pv_shim_do_vcpu_op helper and patch the hypercall table, in order to
avoid modifying more common code. AFAICT this doesn't require adding
much compat code to the shim implementation.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/9] Remove sched=null from shim cmdline and doc

[Wei Liu]

On Fri, Jan 19, 2018 at 10:28:04AM +0000, Roger Pau Monné wrote:
> On Thu, Jan 18, 2018 at 06:16:47PM +0000, Wei Liu wrote:
> > We use the default scheduler (credit1 as of writing). The NULL
> > scheduler still has bugs to fix.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> This is AFAICT missing the change to shim.config, because the shim in
> staging is still being built with:
> 
> CONFIG_SCHED_NULL_DEFAULT=y
> CONFIG_SCHED_DEFAULT="null"
> 
> This however doesn't seem to be the case for the 4.10.0-shim-comet
> branch, where it's built with:
> 
> CONFIG_SCHED_CREDIT_DEFAULT=y
> CONFIG_SCHED_DEFAULT="credit"
> 

-ETOOMANYBRANCHES

I thought this branch (forward-ported version of 4.10.0-shim-comet) had
this in. I probably ported a stale branch.

I will fix this up in next version.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] x86: relocate pvh_info

[Jan Beulich]

>>> On 19.01.18 at 11:53, <roger.pau@citrix.com> wrote:
> On Thu, Jan 18, 2018 at 06:16:48PM +0000, Wei Liu wrote:
>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>> index 0f652cea11..2f94c286d5 100644
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -414,6 +414,7 @@ __pvh_start:
>>  
>>          /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
>>          movw    $0x1000, sym_esi(trampoline_phys)
>> +        xor     %eax, %eax /* Needed by reloc.c */
> 
> I would rather do:
> 
> movl $XEN_HVM_START_MAGIC_VALUE,%eax
> 
> Or:
> 
> mov (%ebx),%eax
> 
> So that...
> 
>> -multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
>> +void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
>>  {
>>      alloc = trampoline;
>>  
>> -    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
>> -        return mbi2_reloc(mbi_in);
>> +#ifdef CONFIG_PVH_GUEST
>> +    if ( magic == 0 )
> 
> ... here you can do magic == XEN_HVM_START_MAGIC_VALUE.
> 
>> +        return pvh_info_reloc(in);
>> +    else
>> +#endif
>> +    if ( magic == MULTIBOOT2_BOOTLOADER_MAGIC )
>> +        return mbi2_reloc(in);
>>      else
>> -        return mbi_reloc(mbi_in);
>> +        return mbi_reloc(in);
> 
> A 'switch ( magic )' would be better here IMHO.

Ah, indeed, that's better.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/9] x86/guest: clean up guest/xen.h

[Wei Liu]

On Fri, Jan 19, 2018 at 10:21:46AM +0000, Roger Pau Monné wrote:
> On Thu, Jan 18, 2018 at 06:16:46PM +0000, Wei Liu wrote:
> > Remove extraneous semicolon. Add blank lines.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  xen/include/asm-x86/guest/xen.h | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
> > index 11243fe60d..31326442f7 100644
> > --- a/xen/include/asm-x86/guest/xen.h
> > +++ b/xen/include/asm-x86/guest/xen.h
> > @@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
> >  #define xen_guest 0
> >  #define pv_console 0
> >  
> > -static inline void probe_hypervisor(void) {};
> > +static inline void probe_hypervisor(void) {}
> > +
> >  static inline void hypervisor_setup(void)
> >  {
> >      ASSERT_UNREACHABLE();
> > @@ -63,20 +64,23 @@ static inline void hypervisor_fixup_e820(struct e820map *e820)
> >  {
> >      ASSERT_UNREACHABLE();
> >  }
> > +
> >  static inline const unsigned long *hypervisor_reserved_pages(unsigned int *size)
> >  {
> >      ASSERT_UNREACHABLE();
> >      return NULL;
> > -};
> > +}
> > +
> >  static inline uint32_t hypervisor_cpuid_base(void)
> >  {
> >      ASSERT_UNREACHABLE();
> >      return 0;
> > -};
> > +}
> > +
> >  static inline void hypervisor_resume(void)
> >  {
> >      ASSERT_UNREACHABLE();
> > -};
> > +}
> 
> AFAICT hypervisor_cpuid_base and hypervisor_resume could be removed,
> because those are only called from shim code, and it's not even
> possible to compile the shim code without having Xen guest support
> enabled.

They need to stay. This header is included by the shim code itself.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/9] x86/guest: clean up guest/xen.h

[Roger Pau Monné]

On Fri, Jan 19, 2018 at 12:22:48PM +0000, Wei Liu wrote:
> On Fri, Jan 19, 2018 at 10:21:46AM +0000, Roger Pau Monné wrote:
> > On Thu, Jan 18, 2018 at 06:16:46PM +0000, Wei Liu wrote:
> > > Remove extraneous semicolon. Add blank lines.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > ---
> > >  xen/include/asm-x86/guest/xen.h | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
> > > index 11243fe60d..31326442f7 100644
> > > --- a/xen/include/asm-x86/guest/xen.h
> > > +++ b/xen/include/asm-x86/guest/xen.h
> > > @@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
> > >  #define xen_guest 0
> > >  #define pv_console 0
> > >  
> > > -static inline void probe_hypervisor(void) {};
> > > +static inline void probe_hypervisor(void) {}
> > > +
> > >  static inline void hypervisor_setup(void)
> > >  {
> > >      ASSERT_UNREACHABLE();
> > > @@ -63,20 +64,23 @@ static inline void hypervisor_fixup_e820(struct e820map *e820)
> > >  {
> > >      ASSERT_UNREACHABLE();
> > >  }
> > > +
> > >  static inline const unsigned long *hypervisor_reserved_pages(unsigned int *size)
> > >  {
> > >      ASSERT_UNREACHABLE();
> > >      return NULL;
> > > -};
> > > +}
> > > +
> > >  static inline uint32_t hypervisor_cpuid_base(void)
> > >  {
> > >      ASSERT_UNREACHABLE();
> > >      return 0;
> > > -};
> > > +}
> > > +
> > >  static inline void hypervisor_resume(void)
> > >  {
> > >      ASSERT_UNREACHABLE();
> > > -};
> > > +}
> > 
> > AFAICT hypervisor_cpuid_base and hypervisor_resume could be removed,
> > because those are only called from shim code, and it's not even
> > possible to compile the shim code without having Xen guest support
> > enabled.
> 
> They need to stay. This header is included by the shim code itself.

But the shim code never sees those, instead it should see the function
prototypes above (inside the #ifdef CONFIG_XEN_GUEST guard).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/9] x86/guest: clean up guest/xen.h

[Wei Liu]

On Fri, Jan 19, 2018 at 12:29:17PM +0000, Roger Pau Monné wrote:
> On Fri, Jan 19, 2018 at 12:22:48PM +0000, Wei Liu wrote:
> > On Fri, Jan 19, 2018 at 10:21:46AM +0000, Roger Pau Monné wrote:
> > > On Thu, Jan 18, 2018 at 06:16:46PM +0000, Wei Liu wrote:
> > > > Remove extraneous semicolon. Add blank lines.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > ---
> > > >  xen/include/asm-x86/guest/xen.h | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
> > > > index 11243fe60d..31326442f7 100644
> > > > --- a/xen/include/asm-x86/guest/xen.h
> > > > +++ b/xen/include/asm-x86/guest/xen.h
> > > > @@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
> > > >  #define xen_guest 0
> > > >  #define pv_console 0
> > > >  
> > > > -static inline void probe_hypervisor(void) {};
> > > > +static inline void probe_hypervisor(void) {}
> > > > +
> > > >  static inline void hypervisor_setup(void)
> > > >  {
> > > >      ASSERT_UNREACHABLE();
> > > > @@ -63,20 +64,23 @@ static inline void hypervisor_fixup_e820(struct e820map *e820)
> > > >  {
> > > >      ASSERT_UNREACHABLE();
> > > >  }
> > > > +
> > > >  static inline const unsigned long *hypervisor_reserved_pages(unsigned int *size)
> > > >  {
> > > >      ASSERT_UNREACHABLE();
> > > >      return NULL;
> > > > -};
> > > > +}
> > > > +
> > > >  static inline uint32_t hypervisor_cpuid_base(void)
> > > >  {
> > > >      ASSERT_UNREACHABLE();
> > > >      return 0;
> > > > -};
> > > > +}
> > > > +
> > > >  static inline void hypervisor_resume(void)
> > > >  {
> > > >      ASSERT_UNREACHABLE();
> > > > -};
> > > > +}
> > > 
> > > AFAICT hypervisor_cpuid_base and hypervisor_resume could be removed,
> > > because those are only called from shim code, and it's not even
> > > possible to compile the shim code without having Xen guest support
> > > enabled.
> > 
> > They need to stay. This header is included by the shim code itself.
> 
> But the shim code never sees those, instead it should see the function
> prototypes above (inside the #ifdef CONFIG_XEN_GUEST guard).

I see. You mean removing the !CONFIG_PVH_GUEST static inline functions.
That would be fine by me.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 9/9] x86/shim: pass through vcpu runstate to L0 Xen

[Wei Liu]

On Fri, Jan 19, 2018 at 11:18:38AM +0000, Roger Pau Monné wrote:
> On Thu, Jan 18, 2018 at 06:16:52PM +0000, Wei Liu wrote:
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 558318e852..189ffac9b1 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -45,6 +45,7 @@
> >  
> >  #ifdef CONFIG_X86
> >  #include <asm/guest.h>
> > +#include <asm/guest/hypercall.h>
> >  #endif
> >  
> >  /* Linux config option: propageted to domain0 */
> > @@ -1425,6 +1426,15 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          if ( !guest_handle_okay(area.addr.h, 1) )
> >              break;
> >  
> > +#if CONFIG_X86
> > +        if ( pv_shim )
> > +        {
> > +            rc = xen_hypercall_vcpu_op(VCPUOP_register_runstate_memory_area,
> > +                                       vcpuid, &area);
> > +            break;
> > +        }
> > +#endif
> 
> This only fixes VCPUOP_register_runstate_memory_area, but
> VCPUOP_get_runstate_info will still report wrong information. I've

Yes, it appears that we should passthrough that call as well.

> also wondered whether simply returning L0 information is correct. To
> get the exact information the shim should actually return the L0
> information plus whatever time it steals from the guest.
> 

I think that should be mostly correct. The error margin in the shim
should be very small.

> Also, before adding more hooks to do_vcpu_op I would attempt to add a
> pv_shim_do_vcpu_op helper and patch the hypercall table, in order to
> avoid modifying more common code. AFAICT this doesn't require adding
> much compat code to the shim implementation.
> 

Good idea.

I'm going to revisit this idea next week when I have more time. In the
mean time I will repost the fixes patches I have.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/9] xen/consoled: discard NUL from guest

[Wei Liu]

On Fri, Jan 19, 2018 at 11:03:02AM +0000, Roger Pau Monné wrote:
> On Thu, Jan 18, 2018 at 06:16:51PM +0000, Wei Liu wrote:
> > According to [0], some program sends NUL for padding purpose. We can
> > discard them.
> > 
> > https://www.gnu.org/software/termutils/manual/termcap-1.3/html_mono/termcap.html#SEC7
> > 
> > Reported-by: Sarah Newman <srn@prgmr.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Although I have a suggestion below.
> 
> > ---
> > Cc: Sarah Newman <srn@prgmr.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > A bit RFC. Awaiting test results.
> > ---
> >  xen/drivers/char/consoled.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> > index 552abf5766..6fcb2aa115 100644
> > --- a/xen/drivers/char/consoled.c
> > +++ b/xen/drivers/char/consoled.c
> > @@ -72,7 +72,9 @@ size_t consoled_guest_rx(void)
> >      {
> >          char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
> 
> You could also do:
> 
> if ( c == '\0' )
>     continue;
> 
> recv AFAICT it's just a cosmetic return value that's not consumed by
> the caller. This avoids checking the "idx >= BUF_SZ" condition with
> the same idx value.
> 

I would like to keep the increment of recv but I like the idea of not
repeatedly checking idx. I will rearrange the code a bit.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel