[PATCH v3 0/4] 32-bit domU PVH support

[PATCH v3 0/4] 32-bit domU PVH support

[Boris Ostrovsky]

Initial 32-bit PVH support, dom0 will need more work.

Changes in v3:
* Swapped patches 1 and 2
* Added is_pvh_32bit_domain() macro
* Dropped a few unnecessary tests for 32b mode
* Added error for unsupported modes
* Added changes to switch_native() similar to those in switch_compat()
* Fully declared compat_mmuext_op() in hvm.c

No need to make any additional changes to prevent a PVH guest from switching
execution mode: hvm_set_cr0() doesn't allow PVH guests to turn off paging and
hvm_set_efer() doesn't allow toggling EFER_LME while paging is on.


Boris Ostrovsky (4):
  x86/pvh: Set 32b PVH guest mode in XEN_DOMCTL_set_address_size
  x86/compat: Test both PV and PVH guests for compat mode
  x86/pvh: Handle hypercalls for 32b PVH guests
  libxc/x86/pvh: Allow creation of 32b PVH guests

 tools/libxc/xc_dom_x86.c      | 32 ++++++++++++------------
 xen/arch/x86/domain.c         | 33 ++++++++++++++-----------
 xen/arch/x86/domctl.c         |  5 ++--
 xen/arch/x86/hvm/hvm.c        | 57 ++++++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/vmx/vmcs.c   |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c    | 19 +++++++++++++++
 xen/include/asm-x86/domain.h  |  1 +
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 8 files changed, 111 insertions(+), 40 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/4] x86/pvh: Set 32b PVH guest mode in XEN_DOMCTL_set_address_size

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v3:
* Updated switch_native() not to call release_compat_l4() on PVH guests
* hvm_set_mode() handles both 8 and 4 modes and returns
  -EOPNOTSUPP otherwise. Similar chages to vmx_set_mode()


 xen/arch/x86/domain.c         | 27 ++++++++++++++++-----------
 xen/arch/x86/hvm/hvm.c        | 24 +++++++++++++++++++++++-
 xen/arch/x86/hvm/vmx/vmcs.c   |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c    | 19 +++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 5 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 956ac70..9c29ef2 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -366,7 +366,11 @@ int switch_native(struct domain *d)
     for_each_vcpu( d, v )
     {
         free_compat_arg_xlat(v);
-        release_compat_l4(v);
+
+        if ( !is_pvh_domain(d) )
+            release_compat_l4(v);
+        else
+            hvm_set_mode(v, 8);
     }
 
     return 0;
@@ -377,25 +381,26 @@ int switch_compat(struct domain *d)
     struct vcpu *v;
     int rc;
 
-    if ( is_pvh_domain(d) )
-    {
-        printk(XENLOG_G_INFO
-               "Xen currently does not support 32bit PVH guests\n");
-        return -EINVAL;
-    }
-
     if ( !may_switch_mode(d) )
         return -EACCES;
     if ( is_pv_32bit_domain(d) )
         return 0;
 
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 1;
+    d->arch.has_32bit_shinfo = 1;
+    if ( is_pv_domain(d) )
+        d->arch.is_32bit_pv = 1;
 
     for_each_vcpu( d, v )
     {
         rc = setup_compat_arg_xlat(v);
         if ( !rc )
-            rc = setup_compat_l4(v);
+        {
+            if ( !is_pvh_domain(d) )
+                rc = setup_compat_l4(v);
+            else
+                rc = hvm_set_mode(v, 4);
+        }
+
         if ( rc )
             goto undo_and_fail;
     }
@@ -410,7 +415,7 @@ int switch_compat(struct domain *d)
     {
         free_compat_arg_xlat(v);
 
-        if ( !pagetable_is_null(v->arch.guest_table) )
+        if ( !is_pvh_domain(d) && !pagetable_is_null(v->arch.guest_table) )
             release_compat_l4(v);
     }
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ebcf7a9..6f247a0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2373,7 +2373,6 @@ int hvm_vcpu_initialise(struct vcpu *v)
 
     if ( is_pvh_domain(d) )
     {
-        v->arch.hvm_vcpu.hcall_64bit = 1;    /* PVH 32bitfixme. */
         /* This is for hvm_long_mode_enabled(v). */
         v->arch.hvm_vcpu.guest_efer = EFER_LMA | EFER_LME;
         return 0;
@@ -6463,6 +6462,29 @@ void hvm_toggle_singlestep(struct vcpu *v)
     v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
 }
 
+int hvm_set_mode(struct vcpu *v, int mode)
+{
+
+    switch ( mode )
+    {
+    case 4:
+        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
+        break;
+    case 8:
+        v->arch.hvm_vcpu.guest_efer |= (EFER_LMA | EFER_LME);
+        break;
+    default:
+        return -EOPNOTSUPP;
+    }
+
+    hvm_update_guest_efer(v);
+
+    if ( hvm_funcs.set_mode )
+        return hvm_funcs.set_mode(v, mode);
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 4c5ceb5..55ab7ad 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1125,7 +1125,7 @@ static int construct_vmcs(struct vcpu *v)
     __vmwrite(GUEST_FS_AR_BYTES, 0xc093);
     __vmwrite(GUEST_GS_AR_BYTES, 0xc093);
     if ( is_pvh_domain(d) )
-        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
+        /* CS.L == 1, exec, read/write, accessed. */
         __vmwrite(GUEST_CS_AR_BYTES, 0xa09b);
     else
         __vmwrite(GUEST_CS_AR_BYTES, 0xc09b); /* exec/read, accessed */
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bc3212f..888dea2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1770,6 +1770,24 @@ static bool_t vmx_is_singlestep_supported(void)
     return cpu_has_monitor_trap_flag;
 }
 
+static int vmx_set_mode(struct vcpu *v, int mode)
+{
+    unsigned long attr;
+
+    if ( !is_pvh_vcpu(v) )
+        return 0;
+
+    ASSERT((mode == 4) || (mode == 8));
+
+    attr = (mode == 4) ? 0xc09b : 0xa09b;
+
+    vmx_vmcs_enter(v);
+    __vmwrite(GUEST_CS_AR_BYTES, attr);
+    vmx_vmcs_exit(v);
+
+    return 0;
+}
+
 static struct hvm_function_table __initdata vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
@@ -1828,6 +1846,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf,
     .enable_msr_exit_interception = vmx_enable_msr_exit_interception,
     .is_singlestep_supported = vmx_is_singlestep_supported,
+    .set_mode = vmx_set_mode,
 };
 
 const struct hvm_function_table * __init start_vmx(void)
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 35f1300..c35dd19 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -203,6 +203,7 @@ struct hvm_function_table {
 
     void (*enable_msr_exit_interception)(struct domain *d);
     bool_t (*is_singlestep_supported)(void);
+    int (*set_mode)(struct vcpu *v, int mode);
 };
 
 extern struct hvm_function_table hvm_funcs;
@@ -237,6 +238,7 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc);
 u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc);
 #define hvm_get_guest_tsc(v) hvm_get_guest_tsc_fixed(v, 0)
 
+int hvm_set_mode(struct vcpu *v, int mode);
 void hvm_init_guest_time(struct domain *d);
 void hvm_set_guest_time(struct vcpu *v, u64 guest_time);
 u64 hvm_get_guest_time_fixed(struct vcpu *v, u64 at_tsc);
-- 
1.8.1.4

[PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Boris Ostrovsky]

Add is_pvh_32bit_domain() macro and use it alongside is_pv_32bit_domain() where
necessary.

Since PVH guests cannot change execution mode, has_32bit_shinfo is a good
indicator of whether the guest is PVH and 32-bit.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v3:
* Defined is_pvh_32bit_domain()
* Dropped unnecessary changes in hypercall_create_continuation() and
  do_domctl() (XEN_DOMCTL_set/getvcpucontext)


 xen/arch/x86/domain.c        | 6 +++---
 xen/arch/x86/domctl.c        | 5 +++--
 xen/include/asm-x86/domain.h | 1 +
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9c29ef2..c743362 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -358,7 +358,7 @@ int switch_native(struct domain *d)
 
     if ( !may_switch_mode(d) )
         return -EACCES;
-    if ( !is_pv_32bit_domain(d) )
+    if ( !is_pv_32bit_domain(d) && !is_pvh_32bit_domain(d) )
         return 0;
 
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
@@ -383,7 +383,7 @@ int switch_compat(struct domain *d)
 
     if ( !may_switch_mode(d) )
         return -EACCES;
-    if ( is_pv_32bit_domain(d) )
+    if ( is_pv_32bit_domain(d) || is_pvh_32bit_domain(d) )
         return 0;
 
     d->arch.has_32bit_shinfo = 1;
@@ -771,7 +771,7 @@ int arch_set_info_guest(
 
     /* The context is a compat-mode one if the target domain is compat-mode;
      * we expect the tools to DTRT even in compat-mode callers. */
-    compat = is_pv_32bit_domain(d);
+    compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);
 
 #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
     flags = c(flags);
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b5047db..63cca52 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -349,7 +349,8 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_get_address_size:
         domctl->u.address_size.size =
-            is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG;
+            (is_pv_32bit_domain(d) || is_pvh_32bit_domain(d)) ?
+            32 : BITS_PER_LONG;
         copyback = 1;
         break;
 
@@ -1203,7 +1204,7 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 {
     unsigned int i;
     const struct domain *d = v->domain;
-    bool_t compat = is_pv_32bit_domain(d);
+    bool_t compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);
 #define c(fld) (!compat ? (c.nat->fld) : (c.cmp->fld))
 
     if ( !is_pv_domain(d) )
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 201436d..263c022 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -14,6 +14,7 @@
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
 #define is_pv_32bit_domain(d)  ((d)->arch.is_32bit_pv)
 #define is_pv_32bit_vcpu(v)    (is_pv_32bit_domain((v)->domain))
+#define is_pvh_32bit_domain(d) (is_pvh_domain(d) && has_32bit_shinfo(d))
 
 #define is_hvm_pv_evtchn_domain(d) (has_hvm_container_domain(d) && \
         d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
-- 
1.8.1.4

[PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v3:
* Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t)
  is not defined in header files so I used 'void' type. I am not convinced
  this is the best solution)

 xen/arch/x86/hvm/hvm.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6f247a0..a4e7185 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4931,7 +4931,6 @@ static hvm_hypercall_t *const hvm_hypercall32_table[NR_hypercalls] = {
     [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
 };
 
-/* PVH 32bitfixme. */
 static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
     HYPERCALL(platform_op),
     HYPERCALL(memory_op),
@@ -4951,6 +4950,29 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
     [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
 };
 
+extern int compat_mmuext_op(XEN_GUEST_HANDLE_PARAM(void) cmp_uops,
+                            unsigned int count,
+                            XEN_GUEST_HANDLE_PARAM(uint) pdone,
+                            unsigned int foreigndom);
+static hvm_hypercall_t *const pvh_hypercall32_table[NR_hypercalls] = {
+    HYPERCALL(platform_op),
+    COMPAT_CALL(memory_op),
+    HYPERCALL(xen_version),
+    HYPERCALL(console_io),
+    [ __HYPERVISOR_grant_table_op ]  =
+        (hvm_hypercall_t *)hvm_grant_table_op_compat32,
+    COMPAT_CALL(vcpu_op),
+    COMPAT_CALL(mmuext_op),
+    HYPERCALL(xsm_op),
+    COMPAT_CALL(sched_op),
+    HYPERCALL(event_channel_op),
+    [ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op_compat32,
+    HYPERCALL(hvm_op),
+    HYPERCALL(sysctl),
+    HYPERCALL(domctl),
+    [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
+};
+
 extern const uint8_t hypercall_args_table[], compat_hypercall_args_table[];
 
 int hvm_do_hypercall(struct cpu_user_regs *regs)
@@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         return viridian_hypercall(regs);
 
     if ( (eax >= NR_hypercalls) ||
-         (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax]
+         (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax]
                                : !hvm_hypercall32_table[eax]) )
     {
         regs->eax = -ENOSYS;
@@ -5037,8 +5059,6 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         }
 #endif
     }
-    else if ( unlikely(is_pvh_domain(currd)) )
-        regs->_eax = -ENOSYS; /* PVH 32bitfixme. */
     else
     {
         unsigned int ebx = regs->_ebx;
@@ -5064,7 +5084,10 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         }
 #endif
 
-        regs->_eax = hvm_hypercall32_table[eax](ebx, ecx, edx, esi, edi, ebp);
+        regs->_eax = (is_pvh_vcpu(curr)
+                      ? pvh_hypercall32_table
+                      : hvm_hypercall32_table)[eax](ebx, ecx, edx,
+                                                    esi, edi, ebp);
 
 #ifndef NDEBUG
         if ( !curr->arch.hvm_vcpu.hcall_preempted )
-- 
1.8.1.4

[PATCH v3 4/4] libxc/x86/pvh: Allow creation of 32b PVH guests

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 6a04399..d0723f1 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -301,7 +301,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
         pgpfn = (addr - dom->parms.virt_base) >> PAGE_SHIFT_X86;
         l1tab[l1off] =
             pfn_to_paddr(xc_dom_p2m_guest(dom, pgpfn)) | L1_PROT;
-        if ( (addr >= dom->pgtables_seg.vstart) &&
+        if ( (!dom->pvh_enabled)                &&
+             (addr >= dom->pgtables_seg.vstart) &&
              (addr < dom->pgtables_seg.vend) )
             l1tab[l1off] &= ~_PAGE_RW; /* page tables are r/o */
 
@@ -591,22 +592,9 @@ static int vcpu_x86_32(struct xc_dom_image *dom, void *ptr)
 
     DOMPRINTF_CALLED(dom->xch);
 
-    if ( dom->pvh_enabled )
-    {
-        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
-                     "%s: PVH not supported for 32bit guests.", __FUNCTION__);
-        return -1;
-    }
-
     /* clear everything */
     memset(ctxt, 0, sizeof(*ctxt));
 
-    ctxt->user_regs.ds = FLAT_KERNEL_DS_X86_32;
-    ctxt->user_regs.es = FLAT_KERNEL_DS_X86_32;
-    ctxt->user_regs.fs = FLAT_KERNEL_DS_X86_32;
-    ctxt->user_regs.gs = FLAT_KERNEL_DS_X86_32;
-    ctxt->user_regs.ss = FLAT_KERNEL_SS_X86_32;
-    ctxt->user_regs.cs = FLAT_KERNEL_CS_X86_32;
     ctxt->user_regs.eip = dom->parms.virt_entry;
     ctxt->user_regs.esp =
         dom->parms.virt_base + (dom->bootstack_pfn + 1) * PAGE_SIZE_X86;
@@ -614,9 +602,6 @@ static int vcpu_x86_32(struct xc_dom_image *dom, void *ptr)
         dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
     ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
 
-    ctxt->kernel_ss = ctxt->user_regs.ss;
-    ctxt->kernel_sp = ctxt->user_regs.esp;
-
     ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
     if ( dom->parms.pae == 2 /* extended_cr3 */ ||
          dom->parms.pae == 3 /* bimodal */ )
@@ -627,6 +612,19 @@ static int vcpu_x86_32(struct xc_dom_image *dom, void *ptr)
     DOMPRINTF("%s: cr3: pfn 0x%" PRIpfn " mfn 0x%" PRIpfn "",
               __FUNCTION__, dom->pgtables_seg.pfn, cr3_pfn);
 
+    if ( dom->pvh_enabled )
+        return 0;
+
+    ctxt->user_regs.ds = FLAT_KERNEL_DS_X86_32;
+    ctxt->user_regs.es = FLAT_KERNEL_DS_X86_32;
+    ctxt->user_regs.fs = FLAT_KERNEL_DS_X86_32;
+    ctxt->user_regs.gs = FLAT_KERNEL_DS_X86_32;
+    ctxt->user_regs.ss = FLAT_KERNEL_SS_X86_32;
+    ctxt->user_regs.cs = FLAT_KERNEL_CS_X86_32;
+
+    ctxt->kernel_ss = ctxt->user_regs.ss;
+    ctxt->kernel_sp = ctxt->user_regs.esp;
+
     return 0;
 }
 
-- 
1.8.1.4

Re: [PATCH v3 1/4] x86/pvh: Set 32b PVH guest mode in XEN_DOMCTL_set_address_size

[Jan Beulich]

>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Jan Beulich]

>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
> Add is_pvh_32bit_domain() macro and use it alongside is_pv_32bit_domain() 
> where necessary.
> 
> Since PVH guests cannot change execution mode, has_32bit_shinfo is a good
> indicator of whether the guest is PVH and 32-bit.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Relative to what is in the tree right now this is fine, but ...

> @@ -771,7 +771,7 @@ int arch_set_info_guest(
>  
>      /* The context is a compat-mode one if the target domain is compat-mode;
>       * we expect the tools to DTRT even in compat-mode callers. */
> -    compat = is_pv_32bit_domain(d);
> +    compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);

... won't this and ...

> @@ -1203,7 +1204,7 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      unsigned int i;
>      const struct domain *d = v->domain;
> -    bool_t compat = is_pv_32bit_domain(d);
> +    bool_t compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);

... this get in the way of what we called "no-pm" on yesterday's call?
I would assume that for the transitional period both ought to be able
to co-exist.

Plus - is this in line with what the tools are doing? Aren't they
assuming !PV <=> native format context? I.e. don't you need
to treat differently v->domain == current->domain and its
opposite? Roger btw. raised a similar question on IRC earlier
today...

Jan

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Ian Campbell]

On Thu, 2015-07-23 at 08:07 -0600, Jan Beulich wrote:
> what we called "no-pm" on yesterday's call?

FYI it was "no-dm" (no device model).

Ian.

Re: [PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

[Jan Beulich]

>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> Changes in v3:
> * Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t)
>   is not defined in header files so I used 'void' type.

How is it not? It's in compat/xen.h (which is a generated header).

> @@ -4951,6 +4950,29 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>      [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
>  };
>  
> +extern int compat_mmuext_op(XEN_GUEST_HANDLE_PARAM(void) cmp_uops,
> +                            unsigned int count,
> +                            XEN_GUEST_HANDLE_PARAM(uint) pdone,
> +                            unsigned int foreigndom);
> +static hvm_hypercall_t *const pvh_hypercall32_table[NR_hypercalls] = {
> +    HYPERCALL(platform_op),
> +    COMPAT_CALL(memory_op),
> +    HYPERCALL(xen_version),
> +    HYPERCALL(console_io),
> +    [ __HYPERVISOR_grant_table_op ]  =
> +        (hvm_hypercall_t *)hvm_grant_table_op_compat32,
> +    COMPAT_CALL(vcpu_op),
> +    COMPAT_CALL(mmuext_op),
> +    HYPERCALL(xsm_op),
> +    COMPAT_CALL(sched_op),
> +    HYPERCALL(event_channel_op),
> +    [ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op_compat32,
> +    HYPERCALL(hvm_op),
> +    HYPERCALL(sysctl),
> +    HYPERCALL(domctl),
> +    [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation

Looks like you didn't fully sync with staging - did you forget that it
was you who added xenpmu_op to the 64-bit counterpart?
Without that ...

> @@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>          return viridian_hypercall(regs);
>  
>      if ( (eax >= NR_hypercalls) ||
> -         (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax]
> +         (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax]
>                                 : !hvm_hypercall32_table[eax]) )

... this will break (as we're assuming 32- and 64-bit tables to be fully
in sync here; there's still the pending work item of constructing these
tables so that this has a better chance of not getting broken).

Also, since you're touching this here anyway, could I ask you to
adjust this along the lines of the change you do further down,
i.e. to become

         !(is_pvh_domain(currd) ? pvh_hypercall32_table
                                : hvm_hypercall32_table)[eax] )

Thanks, Jan

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Jan Beulich]

>>> On 23.07.15 at 16:13, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-07-23 at 08:07 -0600, Jan Beulich wrote:
>> what we called "no-pm" on yesterday's call?
> 
> FYI it was "no-dm" (no device model).

Of course it was - fingers typing disconnected from brain, I guess.

Jan

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Boris Ostrovsky]

On 07/23/2015 10:07 AM, Jan Beulich wrote:
>>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
>> Add is_pvh_32bit_domain() macro and use it alongside is_pv_32bit_domain()
>> where necessary.
>>
>> Since PVH guests cannot change execution mode, has_32bit_shinfo is a good
>> indicator of whether the guest is PVH and 32-bit.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Relative to what is in the tree right now this is fine, but ...
>
>> @@ -771,7 +771,7 @@ int arch_set_info_guest(
>>   
>>       /* The context is a compat-mode one if the target domain is compat-mode;
>>        * we expect the tools to DTRT even in compat-mode callers. */
>> -    compat = is_pv_32bit_domain(d);
>> +    compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);
> ... won't this and ...
>
>> @@ -1203,7 +1204,7 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>   {
>>       unsigned int i;
>>       const struct domain *d = v->domain;
>> -    bool_t compat = is_pv_32bit_domain(d);
>> +    bool_t compat = is_pv_32bit_domain(d) || is_pvh_32bit_domain(d);
> ... this get in the way of what we called "no-pm" on yesterday's call?

I made no effort to make this workable for "no-dm" case. This will 
probably need to be adjusted when that code gets in. IIRC this new code 
has a flag (or something along those lines) that will allow us to 
distinguish PVH-"classic" vs. "no-dm".

> I would assume that for the transitional period both ought to be able
> to co-exist.


Yes, but I suspect this will not be the only place where we will need to 
have different code paths for the two approaches. hvm_set_cr0(), for 
example, doesn't allow PVH guests to switch modes.


> Plus - is this in line with what the tools are doing? Aren't they
> assuming !PV <=> native format context? I.e. don't you need
> to treat differently v->domain == current->domain and its
> opposite? Roger btw. raised a similar question on IRC earlier
> today...

Not sure I understand this. You mean for copying 64-bit guest's info 
into 32-bit dom0?

-boris

Re: [PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

[Boris Ostrovsky]

On 07/23/2015 10:21 AM, Jan Beulich wrote:
>>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> Changes in v3:
>> * Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t)
>>    is not defined in header files so I used 'void' type.
> How is it not? It's in compat/xen.h (which is a generated header).

compat/xen.h has DEFINE_COMPAT_HANDLE(mmuext_op_compat_t) (which is 
__compat_handle_mmuext_op_compat_t).

We need XEN_GUEST_HANDLE(mmuext_op_compat_t), which is 
__guest_handle_mmuext_op_compat_t. And I wasn't sure it's worth 
explicitly adding it to a header file (like I think what we do for 
vcpu_runstate_info_compat_t in sched.h);


>
>> @@ -4951,6 +4950,29 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>>       [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
>>   };
>>   
>> +extern int compat_mmuext_op(XEN_GUEST_HANDLE_PARAM(void) cmp_uops,
>> +                            unsigned int count,
>> +                            XEN_GUEST_HANDLE_PARAM(uint) pdone,
>> +                            unsigned int foreigndom);
>> +static hvm_hypercall_t *const pvh_hypercall32_table[NR_hypercalls] = {
>> +    HYPERCALL(platform_op),
>> +    COMPAT_CALL(memory_op),
>> +    HYPERCALL(xen_version),
>> +    HYPERCALL(console_io),
>> +    [ __HYPERVISOR_grant_table_op ]  =
>> +        (hvm_hypercall_t *)hvm_grant_table_op_compat32,
>> +    COMPAT_CALL(vcpu_op),
>> +    COMPAT_CALL(mmuext_op),
>> +    HYPERCALL(xsm_op),
>> +    COMPAT_CALL(sched_op),
>> +    HYPERCALL(event_channel_op),
>> +    [ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op_compat32,
>> +    HYPERCALL(hvm_op),
>> +    HYPERCALL(sysctl),
>> +    HYPERCALL(domctl),
>> +    [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
> Looks like you didn't fully sync with staging - did you forget that it
> was you who added xenpmu_op to the 64-bit counterpart?

I think I posted this before VPMU got committed. But yes...

> Without that ...
>
>> @@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>           return viridian_hypercall(regs);
>>   
>>       if ( (eax >= NR_hypercalls) ||
>> -         (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax]
>> +         (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax]
>>                                  : !hvm_hypercall32_table[eax]) )
> ... this will break (as we're assuming 32- and 64-bit tables to be fully
> in sync here; there's still the pending work item of constructing these
> tables so that this has a better chance of not getting broken).

So you prefer to have full check --- explicitly for both 32- and 64-bit, 
right?

-boris

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Jan Beulich]

>>> On 24.07.15 at 19:54, <boris.ostrovsky@oracle.com> wrote:
> On 07/23/2015 10:07 AM, Jan Beulich wrote:
>> Plus - is this in line with what the tools are doing? Aren't they
>> assuming !PV <=> native format context? I.e. don't you need
>> to treat differently v->domain == current->domain and its
>> opposite? Roger btw. raised a similar question on IRC earlier
>> today...
> 
> Not sure I understand this. You mean for copying 64-bit guest's info 
> into 32-bit dom0?

Basically yes - tool stack and guest invocations may need to
behave differently.

Jan

Re: [PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

[Jan Beulich]

>>> On 24.07.15 at 20:35, <boris.ostrovsky@oracle.com> wrote:
> On 07/23/2015 10:21 AM, Jan Beulich wrote:
>>>>> On 11.07.15 at 00:20, <boris.ostrovsky@oracle.com> wrote:
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> ---
>>> Changes in v3:
>>> * Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t)
>>>    is not defined in header files so I used 'void' type.
>> How is it not? It's in compat/xen.h (which is a generated header).
> 
> compat/xen.h has DEFINE_COMPAT_HANDLE(mmuext_op_compat_t) (which is 
> __compat_handle_mmuext_op_compat_t).
> 
> We need XEN_GUEST_HANDLE(mmuext_op_compat_t), which is 
> __guest_handle_mmuext_op_compat_t. And I wasn't sure it's worth 
> explicitly adding it to a header file (like I think what we do for 
> vcpu_runstate_info_compat_t in sched.h);

Hmm, indeed all other compat_..._op()-s use void handles (albeit in
most if not all of the cases their native counterparts do too). So I
guess using void here is fine then, or using COMPAT_HANDLE()
instead. It's not really relevant anyway since COMPAT_CALL()
casts the function pointer to the intended type anyway.

>>> @@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>>           return viridian_hypercall(regs);
>>>   
>>>       if ( (eax >= NR_hypercalls) ||
>>> -         (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax]
>>> +         (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax]
>>>                                  : !hvm_hypercall32_table[eax]) )
>> ... this will break (as we're assuming 32- and 64-bit tables to be fully
>> in sync here; there's still the pending work item of constructing these
>> tables so that this has a better chance of not getting broken).
> 
> So you prefer to have full check --- explicitly for both 32- and 64-bit, 
> right?

No. Just adding the missing operation to the table will deal with it.
I wouldn't like to see more conditionals to be added to this code
path when we can avoid doing so. What we could do is add a
respective ASSERT() to the 64-bit path, albeit the NULL deref
would be observable as a fault without the ASSERT() too (and
adding one wouldn't help release builds [and their security]).

Jan

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Boris Ostrovsky]

On 08/11/2015 05:19 AM, Jan Beulich wrote:
>>>> On 24.07.15 at 19:54, <boris.ostrovsky@oracle.com> wrote:
>> On 07/23/2015 10:07 AM, Jan Beulich wrote:
>>> Plus - is this in line with what the tools are doing? Aren't they
>>> assuming !PV <=> native format context? I.e. don't you need
>>> to treat differently v->domain == current->domain and its
>>> opposite? Roger btw. raised a similar question on IRC earlier
>>> today...
>> Not sure I understand this. You mean for copying 64-bit guest's info
>> into 32-bit dom0?
> Basically yes - tool stack and guest invocations may need to
> behave differently.


This being PVH-"classic" it follows exactly the PV path (both in tools 
and the hypervisor). Wouldn't PV be broken then as well?

-boris

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Jan Beulich]

>>> On 11.08.15 at 19:21, <boris.ostrovsky@oracle.com> wrote:
> On 08/11/2015 05:19 AM, Jan Beulich wrote:
>>>>> On 24.07.15 at 19:54, <boris.ostrovsky@oracle.com> wrote:
>>> On 07/23/2015 10:07 AM, Jan Beulich wrote:
>>>> Plus - is this in line with what the tools are doing? Aren't they
>>>> assuming !PV <=> native format context? I.e. don't you need
>>>> to treat differently v->domain == current->domain and its
>>>> opposite? Roger btw. raised a similar question on IRC earlier
>>>> today...
>>> Not sure I understand this. You mean for copying 64-bit guest's info
>>> into 32-bit dom0?
>> Basically yes - tool stack and guest invocations may need to
>> behave differently.
> 
> This being PVH-"classic" it follows exactly the PV path (both in tools 
> and the hypervisor). Wouldn't PV be broken then as well?

Note that I raised a question originally (still seen above) instead
of asking for a specific change. In the end all I'm asking for is that
you make changes in the hypervisor in a way compaible with tools
expectations, or adjust the tools accordingly. And of course you
should keep in mind what "no-dm" will want (i.e. perhaps sync with
Roger), such that we don't end up with guest exposed interface
behavior not suitable for the long term targets we have.

Jan

Re: [PATCH v3 2/4] x86/compat: Test both PV and PVH guests for compat mode

[Boris Ostrovsky]

On 08/12/2015 02:23 AM, Jan Beulich wrote:
>>>> On 11.08.15 at 19:21, <boris.ostrovsky@oracle.com> wrote:
>> On 08/11/2015 05:19 AM, Jan Beulich wrote:
>>>>>> On 24.07.15 at 19:54, <boris.ostrovsky@oracle.com> wrote:
>>>> On 07/23/2015 10:07 AM, Jan Beulich wrote:
>>>>> Plus - is this in line with what the tools are doing? Aren't they
>>>>> assuming !PV <=> native format context? I.e. don't you need
>>>>> to treat differently v->domain == current->domain and its
>>>>> opposite? Roger btw. raised a similar question on IRC earlier
>>>>> today...
>>>> Not sure I understand this. You mean for copying 64-bit guest's info
>>>> into 32-bit dom0?
>>> Basically yes - tool stack and guest invocations may need to
>>> behave differently.
>> This being PVH-"classic" it follows exactly the PV path (both in tools
>> and the hypervisor). Wouldn't PV be broken then as well?
> Note that I raised a question originally (still seen above) instead
> of asking for a specific change. In the end all I'm asking for is that
> you make changes in the hypervisor in a way compaible with tools
> expectations, or adjust the tools accordingly.

So the tools have been adjusted (in the 32-bit path) to make PVH behave 
similarly to PV. This is obviously not what we need to move forward with 
no-dm PVH but this whole series is really tries to bring 32-bit PVH to 
parity with (existing) 64-bit PVH.

> And of course you
> should keep in mind what "no-dm" will want (i.e. perhaps sync with
> Roger), such that we don't end up with guest exposed interface
> behavior not suitable for the long term targets we have.


I suspect the notion of is_pvh_32bit_domain() (which is what this patch 
adds) will be irrelevant in no-dm world.

-boris