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

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

[Boris Ostrovsky]

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

Changes in v4:
* Add xenpmu_op hypercall to pvh_hypercall32_table[] (patch 3)
* Adjust 'is_pvh_domain(currd)' test to match a similar test further in the
  routine (patch 3)

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        | 60 ++++++++++++++++++++++++++++++++++++++-----
 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, 113 insertions(+), 41 deletions(-)

-- 
1.8.1.4

[PATCH v4 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 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 045f6ff..7fa8b9c 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 707ad86..2434564 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2424,7 +2424,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;
@@ -6814,6 +6813,29 @@ bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
     return 0;
 }
 
+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 a0a97e7..08f2078 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1160,7 +1160,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 c32d863..0000d5b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1883,6 +1883,24 @@ static bool_t vmx_vcpu_emulate_ve(struct vcpu *v)
     return rc;
 }
 
+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,
@@ -1942,6 +1960,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,
     .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
     .altp2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
     .altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 3cac64f..2c8ee0d 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -206,6 +206,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);
 
     /* Alternate p2m */
     void (*altp2m_vcpu_update_p2m)(struct vcpu *v);
@@ -246,6 +247,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 v4 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>
---
 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 7fa8b9c..327b13f 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;
@@ -777,7 +777,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 bf62a88..6172c0d 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 0fce09e..6f73d08 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

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

[Jan Beulich]

>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
> @@ -777,7 +777,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);

I continue to think that this should include a v->domain ==
current->domain check (to match behavior for HVM guests
from the tool stack perspective). Having looked at patch 4, I also
can't see how the tool stack is being made expect a non-native
guest context record in the 32-bit PVH case (i.e. I'd appreciate
if you could point out where that hides).

> @@ -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);

Same here then naturally.

Jan

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

[Boris Ostrovsky]

On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>> @@ -777,7 +777,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);
> I continue to think that this should include a v->domain ==
> current->domain check (to match behavior for HVM guests
> from the tool stack perspective). Having looked at patch 4, I also
> can't see how the tool stack is being made expect a non-native
> guest context record in the 32-bit PVH case (i.e. I'd appreciate
> if you could point out where that hides).

For vcpu 0 current->domain is dom0 so I am not sure how this check would 
work.

For a 32-bit PVH guest the toolstack will place data into 
vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor, 
knowing that the guest is a compat one (based on the test above), will 
access appropriate fields.

This is not how HVM guests are started --- "classic" PVH behaves very 
much like a PV guest, unlike what we are doing with no-dm PVH.

-boris

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

[Jan Beulich]

>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 09/02/15 2:55 AM >>>
>On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -777,7 +777,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);
>> I continue to think that this should include a v->domain ==
>> current->domain check (to match behavior for HVM guests
>> from the tool stack perspective). Having looked at patch 4, I also
>> can't see how the tool stack is being made expect a non-native
>> guest context record in the 32-bit PVH case (i.e. I'd appreciate
>> if you could point out where that hides).
>
>For vcpu 0 current->domain is dom0 so I am not sure how this check would 
>work.
>
>For a 32-bit PVH guest the toolstack will place data into 
>vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor, 
>knowing that the guest is a compat one (based on the test above), will 
>access appropriate fields.
>
>This is not how HVM guests are started --- "classic" PVH behaves very 
>much like a PV guest, unlike what we are doing with no-dm PVH.

And I believe this to be wrong, and potentially getting in the way of the no-dm
work - Roger?

As to the reference to vcpu_x86_32() - by its name alone it is already clear
that this is after the determination of what bit width a guest to deal with, and
looking at xc_dom_32_pae I still can't see why PVH guests would (intentionally
and legitimately) be treated like PV rather than HVM ones (not to speak of the
fact that I don't think 32-bit PVH is in any way limited to PAE).

Jan

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

[Boris Ostrovsky]

On 09/02/2015 04:08 AM, Jan Beulich wrote:
>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 09/02/15 2:55 AM >>>
>> On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>>>> @@ -777,7 +777,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);
>>> I continue to think that this should include a v->domain ==
>>> current->domain check (to match behavior for HVM guests
>>> from the tool stack perspective). Having looked at patch 4, I also
>>> can't see how the tool stack is being made expect a non-native
>>> guest context record in the 32-bit PVH case (i.e. I'd appreciate
>>> if you could point out where that hides).
>> For vcpu 0 current->domain is dom0 so I am not sure how this check would
>> work.
>>
>> For a 32-bit PVH guest the toolstack will place data into
>> vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor,
>> knowing that the guest is a compat one (based on the test above), will
>> access appropriate fields.
>>
>> This is not how HVM guests are started --- "classic" PVH behaves very
>> much like a PV guest, unlike what we are doing with no-dm PVH.
> And I believe this to be wrong, and potentially getting in the way of the no-dm
> work - Roger?
>
> As to the reference to vcpu_x86_32() - by its name alone it is already clear
> that this is after the determination of what bit width a guest to deal with, and
> looking at xc_dom_32_pae I still can't see why PVH guests would (intentionally
> and legitimately) be treated like PV rather than HVM ones (not to speak of the
> fact that I don't think 32-bit PVH is in any way limited to PAE).

The purpose of this series is to get 32-bit guests to parity with 
classic PVH with minimal changes and then move on to no-dm. Not getting 
in the way of no-dm is obviously important but making classic behave 
like no-dm (which is, to certain extent, is what you are suggesting) is 
out of scope.

(I haven't considered non-PAE case, TBH)

-boris

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

[Jan Beulich]

>>> On 02.09.15 at 14:31, <boris.ostrovsky@oracle.com> wrote:
> On 09/02/2015 04:08 AM, Jan Beulich wrote:
>>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 09/02/15 2:55 AM >>>
>>> On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>>>>> @@ -777,7 +777,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);
>>>> I continue to think that this should include a v->domain ==
>>>> current->domain check (to match behavior for HVM guests
>>>> from the tool stack perspective). Having looked at patch 4, I also
>>>> can't see how the tool stack is being made expect a non-native
>>>> guest context record in the 32-bit PVH case (i.e. I'd appreciate
>>>> if you could point out where that hides).
>>> For vcpu 0 current->domain is dom0 so I am not sure how this check would
>>> work.
>>>
>>> For a 32-bit PVH guest the toolstack will place data into
>>> vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor,
>>> knowing that the guest is a compat one (based on the test above), will
>>> access appropriate fields.
>>>
>>> This is not how HVM guests are started --- "classic" PVH behaves very
>>> much like a PV guest, unlike what we are doing with no-dm PVH.
>> And I believe this to be wrong, and potentially getting in the way of the 
> no-dm
>> work - Roger?
>>
>> As to the reference to vcpu_x86_32() - by its name alone it is already clear
>> that this is after the determination of what bit width a guest to deal with, and
>> looking at xc_dom_32_pae I still can't see why PVH guests would (intentionally
>> and legitimately) be treated like PV rather than HVM ones (not to speak of the
>> fact that I don't think 32-bit PVH is in any way limited to PAE).
> 
> The purpose of this series is to get 32-bit guests to parity with 
> classic PVH with minimal changes and then move on to no-dm. Not getting 
> in the way of no-dm is obviously important but making classic behave 
> like no-dm (which is, to certain extent, is what you are suggesting) is 
> out of scope.

Well, okay. But are you saying then that 64-bit PVH also just
_happens_ to be treated like PV in the tool stack? IOW I'm still missing
the explicit tool stack adjustment that makes it use a guest-bit-width
context for PVH guests.

> (I haven't considered non-PAE case, TBH)

But I'm afraid you need to.

Jan

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

[Boris Ostrovsky]

On 09/02/2015 09:13 AM, Jan Beulich wrote:
>>>> On 02.09.15 at 14:31, <boris.ostrovsky@oracle.com> wrote:
>> On 09/02/2015 04:08 AM, Jan Beulich wrote:
>>>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 09/02/15 2:55 AM >>>
>>>> On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>>>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>>>>>> @@ -777,7 +777,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);
>>>>> I continue to think that this should include a v->domain ==
>>>>> current->domain check (to match behavior for HVM guests
>>>>> from the tool stack perspective). Having looked at patch 4, I also
>>>>> can't see how the tool stack is being made expect a non-native
>>>>> guest context record in the 32-bit PVH case (i.e. I'd appreciate
>>>>> if you could point out where that hides).
>>>> For vcpu 0 current->domain is dom0 so I am not sure how this check would
>>>> work.
>>>>
>>>> For a 32-bit PVH guest the toolstack will place data into
>>>> vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor,
>>>> knowing that the guest is a compat one (based on the test above), will
>>>> access appropriate fields.
>>>>
>>>> This is not how HVM guests are started --- "classic" PVH behaves very
>>>> much like a PV guest, unlike what we are doing with no-dm PVH.
>>> And I believe this to be wrong, and potentially getting in the way of the
>> no-dm
>>> work - Roger?
>>>
>>> As to the reference to vcpu_x86_32() - by its name alone it is already clear
>>> that this is after the determination of what bit width a guest to deal with, and
>>> looking at xc_dom_32_pae I still can't see why PVH guests would (intentionally
>>> and legitimately) be treated like PV rather than HVM ones (not to speak of the
>>> fact that I don't think 32-bit PVH is in any way limited to PAE).
>> The purpose of this series is to get 32-bit guests to parity with
>> classic PVH with minimal changes and then move on to no-dm. Not getting
>> in the way of no-dm is obviously important but making classic behave
>> like no-dm (which is, to certain extent, is what you are suggesting) is
>> out of scope.
> Well, okay. But are you saying then that 64-bit PVH also just
> _happens_ to be treated like PV in the tool stack?

Yes. They both are processed by tollstack's elf parser (and the size is 
determined by xc_dom_guest_type()).

> IOW I'm still missing
> the explicit tool stack adjustment that makes it use a guest-bit-width
> context for PVH guests.
>
>> (I haven't considered non-PAE case, TBH)
> But I'm afraid you need to.
>

Actually, at least for Linux, CONFIG_XEN 'depends on X86_64 || (X86_32 
&& X86_PAE)'. And I don't know whether there are any other OSs (i.e. 
FreeBSD) that are going to ever support 32-bit PVH-classic (Roger might 
correct me on that).

-boris

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

[Roger Pau Monné]

El 02/09/15 a les 16.01, Boris Ostrovsky ha escrit:
> Actually, at least for Linux, CONFIG_XEN 'depends on X86_64 || (X86_32
> && X86_PAE)'. And I don't know whether there are any other OSs (i.e.
> FreeBSD) that are going to ever support 32-bit PVH-classic (Roger might
> correct me on that).

No, I don't plan to add 32bit PVH-classic support to FreeBSD, and adding
32bit no-dm support to FreeBSD is also quite low in my priority list
right now.

Roger.

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

[Jan Beulich]

>>> On 02.09.15 at 16:01, <boris.ostrovsky@oracle.com> wrote:
> On 09/02/2015 09:13 AM, Jan Beulich wrote:
>>>>> On 02.09.15 at 14:31, <boris.ostrovsky@oracle.com> wrote:
>>> (I haven't considered non-PAE case, TBH)
>> But I'm afraid you need to.
> 
> Actually, at least for Linux, CONFIG_XEN 'depends on X86_64 || (X86_32 
> && X86_PAE)'. And I don't know whether there are any other OSs (i.e. 
> FreeBSD) that are going to ever support 32-bit PVH-classic (Roger might 
> correct me on that).

That's fine, but then at least you should make sure 32-bit PVH
guests can't leave PAE mode.

Jan

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

[Roger Pau Monné]

El 02/09/15 a les 10.08, Jan Beulich ha escrit:
>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 09/02/15 2:55 AM >>>
>> On 08/27/2015 12:01 PM, Jan Beulich wrote:
>>>>>> On 13.08.15 at 20:12, <boris.ostrovsky@oracle.com> wrote:
>>>> @@ -777,7 +777,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);
>>> I continue to think that this should include a v->domain ==
>>> current->domain check (to match behavior for HVM guests
>>> from the tool stack perspective). Having looked at patch 4, I also
>>> can't see how the tool stack is being made expect a non-native
>>> guest context record in the 32-bit PVH case (i.e. I'd appreciate
>>> if you could point out where that hides).
>>
>> For vcpu 0 current->domain is dom0 so I am not sure how this check would 
>> work.
>>
>> For a 32-bit PVH guest the toolstack will place data into 
>> vcpu_guest_context_x86_32_t (in vcpu_x86_32()) and so the hypervisor, 
>> knowing that the guest is a compat one (based on the test above), will 
>> access appropriate fields.
>>
>> This is not how HVM guests are started --- "classic" PVH behaves very 
>> much like a PV guest, unlike what we are doing with no-dm PVH.
> 
> And I believe this to be wrong, and potentially getting in the way of the no-dm
> work - Roger?

The no-dm work doesn't use arch_set_info_guest, it uses a new function
created specifically for no-dm guests with a new structure also, see:

http://marc.info/?l=xen-devel&m=144017739531928

So I don't think this code is explicitly getting in the way of no-dm.
Another thing to consider might be if it's worth to add this when no-dm
is going in a complete different direction regarding AP startup. I
certainly don't plan to add classic PVH 32bit support to FreeBSD.

Roger.

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

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/hvm.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2434564..96de1bf 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5094,7 +5094,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),
@@ -5114,6 +5113,30 @@ 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),
+    HYPERCALL(xenpmu_op),
+    [ __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)
@@ -5144,8 +5167,8 @@ 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]
-                               : !hvm_hypercall32_table[eax]) )
+         !(is_pvh_domain(currd) ? pvh_hypercall32_table[eax]
+                                : hvm_hypercall32_table[eax]) )
     {
         regs->eax = -ENOSYS;
         return HVM_HCALL_completed;
@@ -5200,8 +5223,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;
@@ -5227,7 +5248,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

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

[Jan Beulich]

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

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

[PATCH v4 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 3d40fa4..05fb0ce 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -300,7 +300,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 */
 
@@ -590,22 +591,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;
@@ -613,9 +601,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 */ )
@@ -626,6 +611,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