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

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

[Boris Ostrovsky]

Changes in v2:
* Only set domain's has_32bit_shinfo, not is_32bit_pv. This adds patch 1 that
  replaces some of is_pv_32bit_domain()/is_pv_32bit_vcpu()/is_pv_32on64_vcpu()
  tests with has_32bit_shinfo().
  Note that I only replaced what I thought was necessary to boot the guest.
  MCA and tracing might also need similar changes (and maybe something else)
* Set up PVH VCPUs for 64-bit mode (i.e. keep setup the way it is now) and
  change mode to 32-bit later if necessary. (patch 2)
* Minor cleanup in hvm_do_hypercall() (patch 3)
* Drop 2 patches (one that set vstart address to __HYPERVISOR_COMPAT_VIRT_START
  and the other that became unnecessary due to the first change above)

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

Boris Ostrovsky (4):
  x86/compat: Test whether guest has 32b shinfo instead of being a PV
    32b domain
  x86/pvh: Set 32b PVH guest mode in XEN_DOMCTL_set_address_size
  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         | 30 +++++++++++++---------------
 xen/arch/x86/domctl.c         |  4 ++--
 xen/arch/x86/hvm/hvm.c        | 46 +++++++++++++++++++++++++++++++++++++------
 xen/arch/x86/hvm/vmx/vmcs.c   |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c    | 17 ++++++++++++++++
 xen/common/domctl.c           |  4 ++--
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 8 files changed, 93 insertions(+), 44 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

In preparation for enabling 32-bit PVH guests replace a number of guest mode's
tests that assume a PV guest with has_32bit_shinfo() that can be applicable to
both PV and PVH guests.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* New patch

 xen/arch/x86/domain.c | 10 ++++------
 xen/arch/x86/domctl.c |  4 ++--
 xen/common/domctl.c   |  4 ++--
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a8fe046..b87f642 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_32on64_domain(d) )
+    if ( !has_32bit_shinfo(d) )
         return 0;
 
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
@@ -386,7 +386,7 @@ int switch_compat(struct domain *d)
 
     if ( !may_switch_mode(d) )
         return -EACCES;
-    if ( is_pv_32on64_domain(d) )
+    if ( has_32bit_shinfo(d) )
         return 0;
 
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 1;
@@ -737,7 +737,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_32on64_domain(d);
+    compat = has_32bit_shinfo(d);
 
 #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
     flags = c(flags);
@@ -1721,9 +1721,7 @@ unsigned long hypercall_create_continuation(
         else
             curr->arch.hvm_vcpu.hcall_preempted = 1;
 
-        if ( is_pv_vcpu(curr) ?
-             !is_pv_32bit_vcpu(curr) :
-             curr->arch.hvm_vcpu.hcall_64bit )
+        if ( !has_32bit_shinfo(curr->domain) )
         {
             for ( i = 0; *p != '\0'; i++ )
             {
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 82bd818..57f8535 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -349,7 +349,7 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_get_address_size:
         domctl->u.address_size.size =
-            is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
+            has_32bit_shinfo(d) ? 32 : BITS_PER_LONG;
         copyback = 1;
         break;
 
@@ -1183,7 +1183,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_32on64_domain(d);
+    bool_t compat = has_32bit_shinfo(d);
 #define c(fld) (!compat ? (c.nat->fld) : (c.cmp->fld))
 
     if ( !is_pv_domain(d) )
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 2a2d203..66fe668 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             break;
 
 #ifdef CONFIG_COMPAT
-        if ( !is_pv_32on64_domain(d) )
+        if ( !has_32bit_shinfo(d) )
             ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
         else
             ret = copy_from_guest(c.cmp,
@@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         vcpu_unpause(v);
 
 #ifdef CONFIG_COMPAT
-        if ( !is_pv_32on64_domain(d) )
+        if ( !has_32bit_shinfo(d) )
             ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
         else
             ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
-- 
1.8.1.4

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
> In preparation for enabling 32-bit PVH guests replace a number of guest mode's
> tests that assume a PV guest with has_32bit_shinfo() that can be applicable 
> to
> both PV and PVH guests.

Apart from this apparently needing re-basing, I also think this should
be swapped with patch 2, as right now it doesn't make much sense to
distinguish the two checks.

> @@ -737,7 +737,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_32on64_domain(d);
> +    compat = has_32bit_shinfo(d);

Furthermore, looking at uses like this, tying such decisions to the
shared info layout looks kind of odd. I think for documentation
purposes we may need a differently named alias.

> @@ -1721,9 +1721,7 @@ unsigned long hypercall_create_continuation(
>          else
>              curr->arch.hvm_vcpu.hcall_preempted = 1;
>  
> -        if ( is_pv_vcpu(curr) ?
> -             !is_pv_32bit_vcpu(curr) :
> -             curr->arch.hvm_vcpu.hcall_64bit )
> +        if ( !has_32bit_shinfo(curr->domain) )

This is not a valid replacement - hcall_64bit depends on the mode
the vCPU currently is in, while has_32bit_shinfo() doesn't.

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              break;
>  
>  #ifdef CONFIG_COMPAT
> -        if ( !is_pv_32on64_domain(d) )
> +        if ( !has_32bit_shinfo(d) )
>              ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>          else
>              ret = copy_from_guest(c.cmp,
> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          vcpu_unpause(v);
>  
>  #ifdef CONFIG_COMPAT
> -        if ( !is_pv_32on64_domain(d) )
> +        if ( !has_32bit_shinfo(d) )
>              ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>          else
>              ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,

Where is it written down what format 32-bit PVH guests' vCPU
contexts get passed in? It would seem to me that it would be
rather more natural for them to use the 64-bit layout. Or else
how do you intend to suppress them being able to enter 64-bit
mode?

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>> In preparation for enabling 32-bit PVH guests replace a number of guest mode's
>> tests that assume a PV guest with has_32bit_shinfo() that can be applicable
>> to
>> both PV and PVH guests.
> Apart from this apparently needing re-basing, I also think this should
> be swapped with patch 2, as right now it doesn't make much sense to
> distinguish the two checks.
>
>> @@ -737,7 +737,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_32on64_domain(d);
>> +    compat = has_32bit_shinfo(d);
> Furthermore, looking at uses like this, tying such decisions to the
> shared info layout looks kind of odd. I think for documentation
> purposes we may need a differently named alias.

Yes, it does look odd, which is why I was asking in another thread about 
having another field in domain structure (well, I was asking about 
replacing has_32bit_shinfo but I think I can see now that wouldn't be 
right).

Are you suggesting a new macro, e.g.
#define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)

or would it better to add new field? Or get_mode() hvm op, similar to 
set_mode(), which can look, say, at EFER?


>
>> @@ -1721,9 +1721,7 @@ unsigned long hypercall_create_continuation(
>>           else
>>               curr->arch.hvm_vcpu.hcall_preempted = 1;
>>   
>> -        if ( is_pv_vcpu(curr) ?
>> -             !is_pv_32bit_vcpu(curr) :
>> -             curr->arch.hvm_vcpu.hcall_64bit )
>> +        if ( !has_32bit_shinfo(curr->domain) )
> This is not a valid replacement - hcall_64bit depends on the mode
> the vCPU currently is in, while has_32bit_shinfo() doesn't.

Right, and I don't think this change is needed anyway since 
hvm_do_hypercall() will set hcall_64bit for PVH guests as well (when the 
guest is in 64-bit mode)

>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>               break;
>>   
>>   #ifdef CONFIG_COMPAT
>> -        if ( !is_pv_32on64_domain(d) )
>> +        if ( !has_32bit_shinfo(d) )
>>               ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>>           else
>>               ret = copy_from_guest(c.cmp,
>> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>           vcpu_unpause(v);
>>   
>>   #ifdef CONFIG_COMPAT
>> -        if ( !is_pv_32on64_domain(d) )
>> +        if ( !has_32bit_shinfo(d) )
>>               ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>>           else
>>               ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
> Where is it written down what format 32-bit PVH guests' vCPU
> contexts get passed in? It would seem to me that it would be
> rather more natural for them to use the 64-bit layout. Or else
> how do you intend to suppress them being able to enter 64-bit
> mode?

So why do we use the 'else' clause for 32b PV guests when they also use 
the same vcpu_guest_context_x86_32_t in libxc/xc_dom_x86.c:vcpu_x86_32()?

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -737,7 +737,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_32on64_domain(d);
>>> +    compat = has_32bit_shinfo(d);
>> Furthermore, looking at uses like this, tying such decisions to the
>> shared info layout looks kind of odd. I think for documentation
>> purposes we may need a differently named alias.
> 
> Yes, it does look odd, which is why I was asking in another thread about 
> having another field in domain structure (well, I was asking about 
> replacing has_32bit_shinfo but I think I can see now that wouldn't be 
> right).
> 
> Are you suggesting a new macro, e.g.
> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
> 
> or would it better to add new field? Or get_mode() hvm op, similar to 
> set_mode(), which can look, say, at EFER?

If looking at EFER (plus perhaps CS) is right in all the cases you
care about, then yes. And remember we already have
hvm_guest_x86_mode().

>>> @@ -1721,9 +1721,7 @@ unsigned long hypercall_create_continuation(
>>>           else
>>>               curr->arch.hvm_vcpu.hcall_preempted = 1;
>>>   
>>> -        if ( is_pv_vcpu(curr) ?
>>> -             !is_pv_32bit_vcpu(curr) :
>>> -             curr->arch.hvm_vcpu.hcall_64bit )
>>> +        if ( !has_32bit_shinfo(curr->domain) )
>> This is not a valid replacement - hcall_64bit depends on the mode
>> the vCPU currently is in, while has_32bit_shinfo() doesn't.
> 
> Right, and I don't think this change is needed anyway since 
> hvm_do_hypercall() will set hcall_64bit for PVH guests as well (when the 
> guest is in 64-bit mode)

Right - I was about to say that.

>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>               break;
>>>   
>>>   #ifdef CONFIG_COMPAT
>>> -        if ( !is_pv_32on64_domain(d) )
>>> +        if ( !has_32bit_shinfo(d) )
>>>               ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>>>           else
>>>               ret = copy_from_guest(c.cmp,
>>> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>           vcpu_unpause(v);
>>>   
>>>   #ifdef CONFIG_COMPAT
>>> -        if ( !is_pv_32on64_domain(d) )
>>> +        if ( !has_32bit_shinfo(d) )
>>>               ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>>>           else
>>>               ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
>> Where is it written down what format 32-bit PVH guests' vCPU
>> contexts get passed in? It would seem to me that it would be
>> rather more natural for them to use the 64-bit layout. Or else
>> how do you intend to suppress them being able to enter 64-bit
>> mode?
> 
> So why do we use the 'else' clause for 32b PV guests when they also use 
> the same vcpu_guest_context_x86_32_t in libxc/xc_dom_x86.c:vcpu_x86_32()?

32bit PV guests use the if() branch afaict (as they use the 32-bit
shared info layout).

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>> +    compat = has_32bit_shinfo(d);
>>> Furthermore, looking at uses like this, tying such decisions to the
>>> shared info layout looks kind of odd. I think for documentation
>>> purposes we may need a differently named alias.
>> Yes, it does look odd, which is why I was asking in another thread about
>> having another field in domain structure (well, I was asking about
>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>> right).
>>
>> Are you suggesting a new macro, e.g.
>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>
>> or would it better to add new field? Or get_mode() hvm op, similar to
>> set_mode(), which can look, say, at EFER?
> If looking at EFER (plus perhaps CS) is right in all the cases you
> care about, then yes. And remember we already have
> hvm_guest_x86_mode().

Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding 
new op just because of that seems to be an overkill since it would 
essentially do what .guest_x86_mode() does. How about 
hvm_guest_x86_mode_unsafe() (with a better name) and wrap 
hvm_guest_x86_mode() with the ASSERT around it?

>
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>                break;
>>>>    
>>>>    #ifdef CONFIG_COMPAT
>>>> -        if ( !is_pv_32on64_domain(d) )
>>>> +        if ( !has_32bit_shinfo(d) )
>>>>                ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>>>>            else
>>>>                ret = copy_from_guest(c.cmp,
>>>> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>            vcpu_unpause(v);
>>>>    
>>>>    #ifdef CONFIG_COMPAT
>>>> -        if ( !is_pv_32on64_domain(d) )
>>>> +        if ( !has_32bit_shinfo(d) )
>>>>                ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>>>>            else
>>>>                ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
>>> Where is it written down what format 32-bit PVH guests' vCPU
>>> contexts get passed in? It would seem to me that it would be
>>> rather more natural for them to use the 64-bit layout. Or else
>>> how do you intend to suppress them being able to enter 64-bit
>>> mode?
>> So why do we use the 'else' clause for 32b PV guests when they also use
>> the same vcpu_guest_context_x86_32_t in libxc/xc_dom_x86.c:vcpu_x86_32()?
> 32bit PV guests use the if() branch afaict (as they use the 32-bit
> shared info layout).

No, they use the 'else' part, I just confirmed it. 'd' in 
is_pv_32on64_domain() is domain for which domctl is being called, not 
domain that is making the call (which is what I suspect the original 
intent was).

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>> +    compat = has_32bit_shinfo(d);
>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>> shared info layout looks kind of odd. I think for documentation
>>>> purposes we may need a differently named alias.
>>> Yes, it does look odd, which is why I was asking in another thread about
>>> having another field in domain structure (well, I was asking about
>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>> right).
>>>
>>> Are you suggesting a new macro, e.g.
>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>
>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>> set_mode(), which can look, say, at EFER?
>> If looking at EFER (plus perhaps CS) is right in all the cases you
>> care about, then yes. And remember we already have
>> hvm_guest_x86_mode().
> 
> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding 
> new op just because of that seems to be an overkill since it would 
> essentially do what .guest_x86_mode() does. How about 
> hvm_guest_x86_mode_unsafe() (with a better name) and wrap 
> hvm_guest_x86_mode() with the ASSERT around it?

svm_guest_x86_mode() doesn't depend on v == current, but
vmx_guest_x86_mode() would first need to be made safe (or
get an "unsafe" sibling implementation). With that, the ASSERT()
could then check for current or non-running vCPU.

>>>>> --- a/xen/common/domctl.c
>>>>> +++ b/xen/common/domctl.c
>>>>> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>>>>>                break;
>>>>>    
>>>>>    #ifdef CONFIG_COMPAT
>>>>> -        if ( !is_pv_32on64_domain(d) )
>>>>> +        if ( !has_32bit_shinfo(d) )
>>>>>                ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>>>>>            else
>>>>>                ret = copy_from_guest(c.cmp,
>>>>> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>>>>>            vcpu_unpause(v);
>>>>>    
>>>>>    #ifdef CONFIG_COMPAT
>>>>> -        if ( !is_pv_32on64_domain(d) )
>>>>> +        if ( !has_32bit_shinfo(d) )
>>>>>                ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>>>>>            else
>>>>>                ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
>>>> Where is it written down what format 32-bit PVH guests' vCPU
>>>> contexts get passed in? It would seem to me that it would be
>>>> rather more natural for them to use the 64-bit layout. Or else
>>>> how do you intend to suppress them being able to enter 64-bit
>>>> mode?
>>> So why do we use the 'else' clause for 32b PV guests when they also use
>>> the same vcpu_guest_context_x86_32_t in libxc/xc_dom_x86.c:vcpu_x86_32()?
>> 32bit PV guests use the if() branch afaict (as they use the 32-bit
>> shared info layout).
> 
> No, they use the 'else' part, I just confirmed it. 'd' in 
> is_pv_32on64_domain() is domain for which domctl is being called, not 
> domain that is making the call (which is what I suspect the original 
> intent was).

Oh, yes, of course they do - how did I overlook the "!" ? Yet
that doesn't help me understand the question: Isn't it obvious
that if libxc expects vcpu_guest_context_x86_32_t, then the
hypervisor also needs to supply that one (and not the 64-bit
counterpart)? Or are you asking why the format matches the
subject domain's word width, not the calling domain's? This has
historical reasons: A 32-bit domain saved on a 64-bit hypervisor
needed to be restorable by a 32-bit hypervisor when that still
existed. This could likely be changed nowadays; ARM and the
HVM case must be dealt with in the tools somehow anyway.

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>>> +    compat = has_32bit_shinfo(d);
>>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>>> shared info layout looks kind of odd. I think for documentation
>>>>> purposes we may need a differently named alias.
>>>> Yes, it does look odd, which is why I was asking in another thread about
>>>> having another field in domain structure (well, I was asking about
>>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>>> right).
>>>>
>>>> Are you suggesting a new macro, e.g.
>>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>>
>>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>>> set_mode(), which can look, say, at EFER?
>>> If looking at EFER (plus perhaps CS) is right in all the cases you
>>> care about, then yes. And remember we already have
>>> hvm_guest_x86_mode().
>> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding
>> new op just because of that seems to be an overkill since it would
>> essentially do what .guest_x86_mode() does. How about
>> hvm_guest_x86_mode_unsafe() (with a better name) and wrap
>> hvm_guest_x86_mode() with the ASSERT around it?
> svm_guest_x86_mode() doesn't depend on v == current, but
> vmx_guest_x86_mode() would first need to be made safe (or
> get an "unsafe" sibling implementation). With that, the ASSERT()
> could then check for current or non-running vCPU.

By checking for non-running you mean v->is_running? I am not sure it's 
safe to do since is_running is set in context switch before VMCS is 
loaded later, in vmx_do_resume().

OTOH, current itself is set before VMCS is loaded so I am not sure 
whether the ASSERT in hvm_guest_x86_mode() is completely effective in 
catching "bad" invocations anyway.

I think we need vmx_vmcs_enter/exit in vmx_guest_x86_mode() regardless 
of what current is. And drop the ASSERT.

>
>>>>>> --- a/xen/common/domctl.c
>>>>>> +++ b/xen/common/domctl.c
>>>>>> @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>>>>>>                 break;
>>>>>>     
>>>>>>     #ifdef CONFIG_COMPAT
>>>>>> -        if ( !is_pv_32on64_domain(d) )
>>>>>> +        if ( !has_32bit_shinfo(d) )
>>>>>>                 ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
>>>>>>             else
>>>>>>                 ret = copy_from_guest(c.cmp,
>>>>>> @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>>>>>>             vcpu_unpause(v);
>>>>>>     
>>>>>>     #ifdef CONFIG_COMPAT
>>>>>> -        if ( !is_pv_32on64_domain(d) )
>>>>>> +        if ( !has_32bit_shinfo(d) )
>>>>>>                 ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
>>>>>>             else
>>>>>>                 ret = copy_to_guest(guest_handle_cast(op->u.vcpucontext.ctxt,
>>>>> Where is it written down what format 32-bit PVH guests' vCPU
>>>>> contexts get passed in? It would seem to me that it would be
>>>>> rather more natural for them to use the 64-bit layout. Or else
>>>>> how do you intend to suppress them being able to enter 64-bit
>>>>> mode?
>>>> So why do we use the 'else' clause for 32b PV guests when they also use
>>>> the same vcpu_guest_context_x86_32_t in libxc/xc_dom_x86.c:vcpu_x86_32()?
>>> 32bit PV guests use the if() branch afaict (as they use the 32-bit
>>> shared info layout).
>> No, they use the 'else' part, I just confirmed it. 'd' in
>> is_pv_32on64_domain() is domain for which domctl is being called, not
>> domain that is making the call (which is what I suspect the original
>> intent was).
> Oh, yes, of course they do - how did I overlook the "!" ? Yet
> that doesn't help me understand the question: Isn't it obvious
> that if libxc expects vcpu_guest_context_x86_32_t, then the
> hypervisor also needs to supply that one (and not the 64-bit
> counterpart)? Or are you asking why the format matches the
> subject domain's word width, not the calling domain's?

Yes, this was the question.

> This has
> historical reasons: A 32-bit domain saved on a 64-bit hypervisor
> needed to be restorable by a 32-bit hypervisor when that still
> existed. This could likely be changed nowadays; ARM and the
> HVM case must be dealt with in the tools somehow anyway.


OK, then I don't need those two changes in do_domctl().

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 08.07.15 at 15:59, <boris.ostrovsky@oracle.com> wrote:
> On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
>>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>>>> +    compat = has_32bit_shinfo(d);
>>>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>>>> shared info layout looks kind of odd. I think for documentation
>>>>>> purposes we may need a differently named alias.
>>>>> Yes, it does look odd, which is why I was asking in another thread about
>>>>> having another field in domain structure (well, I was asking about
>>>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>>>> right).
>>>>>
>>>>> Are you suggesting a new macro, e.g.
>>>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>>>
>>>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>>>> set_mode(), which can look, say, at EFER?
>>>> If looking at EFER (plus perhaps CS) is right in all the cases you
>>>> care about, then yes. And remember we already have
>>>> hvm_guest_x86_mode().
>>> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding
>>> new op just because of that seems to be an overkill since it would
>>> essentially do what .guest_x86_mode() does. How about
>>> hvm_guest_x86_mode_unsafe() (with a better name) and wrap
>>> hvm_guest_x86_mode() with the ASSERT around it?
>> svm_guest_x86_mode() doesn't depend on v == current, but
>> vmx_guest_x86_mode() would first need to be made safe (or
>> get an "unsafe" sibling implementation). With that, the ASSERT()
>> could then check for current or non-running vCPU.
> 
> By checking for non-running you mean v->is_running? I am not sure it's 
> safe to do since is_running is set in context switch before VMCS is 
> loaded later, in vmx_do_resume().

No, I rather thought about making sure the vCPU is paused (i.e.
can't become running under your feet).

> OTOH, current itself is set before VMCS is loaded so I am not sure 
> whether the ASSERT in hvm_guest_x86_mode() is completely effective in 
> catching "bad" invocations anyway.
> 
> I think we need vmx_vmcs_enter/exit in vmx_guest_x86_mode() regardless 
> of what current is. And drop the ASSERT.

That's an option, but the current uses don't require that (and hence
a change like that may be considered harming performance if some
caller sits on a hot path).

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/08/2015 10:08 AM, Jan Beulich wrote:
>>>> On 08.07.15 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>> On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
>>>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>>>>> +    compat = has_32bit_shinfo(d);
>>>>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>>>>> shared info layout looks kind of odd. I think for documentation
>>>>>>> purposes we may need a differently named alias.
>>>>>> Yes, it does look odd, which is why I was asking in another thread about
>>>>>> having another field in domain structure (well, I was asking about
>>>>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>>>>> right).
>>>>>>
>>>>>> Are you suggesting a new macro, e.g.
>>>>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>>>>
>>>>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>>>>> set_mode(), which can look, say, at EFER?
>>>>> If looking at EFER (plus perhaps CS) is right in all the cases you
>>>>> care about, then yes. And remember we already have
>>>>> hvm_guest_x86_mode().
>>>> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding
>>>> new op just because of that seems to be an overkill since it would
>>>> essentially do what .guest_x86_mode() does. How about
>>>> hvm_guest_x86_mode_unsafe() (with a better name) and wrap
>>>> hvm_guest_x86_mode() with the ASSERT around it?
>>> svm_guest_x86_mode() doesn't depend on v == current, but
>>> vmx_guest_x86_mode() would first need to be made safe (or
>>> get an "unsafe" sibling implementation). With that, the ASSERT()
>>> could then check for current or non-running vCPU.
>> By checking for non-running you mean v->is_running? I am not sure it's
>> safe to do since is_running is set in context switch before VMCS is
>> loaded later, in vmx_do_resume().
> No, I rather thought about making sure the vCPU is paused (i.e.
> can't become running under your feet).

What would prevent it from becoming running if it is paused, right after 
the ASSERT?

-boris


>
>> OTOH, current itself is set before VMCS is loaded so I am not sure
>> whether the ASSERT in hvm_guest_x86_mode() is completely effective in
>> catching "bad" invocations anyway.
>>
>> I think we need vmx_vmcs_enter/exit in vmx_guest_x86_mode() regardless
>> of what current is. And drop the ASSERT.
> That's an option, but the current uses don't require that (and hence
> a change like that may be considered harming performance if some
> caller sits on a hot path).

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 08.07.15 at 16:40, <boris.ostrovsky@oracle.com> wrote:
> On 07/08/2015 10:08 AM, Jan Beulich wrote:
>>>>> On 08.07.15 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>>> On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>>>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>>>>>> +    compat = has_32bit_shinfo(d);
>>>>>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>>>>>> shared info layout looks kind of odd. I think for documentation
>>>>>>>> purposes we may need a differently named alias.
>>>>>>> Yes, it does look odd, which is why I was asking in another thread about
>>>>>>> having another field in domain structure (well, I was asking about
>>>>>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>>>>>> right).
>>>>>>>
>>>>>>> Are you suggesting a new macro, e.g.
>>>>>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>>>>>
>>>>>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>>>>>> set_mode(), which can look, say, at EFER?
>>>>>> If looking at EFER (plus perhaps CS) is right in all the cases you
>>>>>> care about, then yes. And remember we already have
>>>>>> hvm_guest_x86_mode().
>>>>> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding
>>>>> new op just because of that seems to be an overkill since it would
>>>>> essentially do what .guest_x86_mode() does. How about
>>>>> hvm_guest_x86_mode_unsafe() (with a better name) and wrap
>>>>> hvm_guest_x86_mode() with the ASSERT around it?
>>>> svm_guest_x86_mode() doesn't depend on v == current, but
>>>> vmx_guest_x86_mode() would first need to be made safe (or
>>>> get an "unsafe" sibling implementation). With that, the ASSERT()
>>>> could then check for current or non-running vCPU.
>>> By checking for non-running you mean v->is_running? I am not sure it's
>>> safe to do since is_running is set in context switch before VMCS is
>>> loaded later, in vmx_do_resume().
>> No, I rather thought about making sure the vCPU is paused (i.e.
>> can't become running under your feet).
> 
> What would prevent it from becoming running if it is paused, right after 
> the ASSERT?

True. I'm fine with dropping the ASSERT() after having done the
proposed adjustment to the VMX side, provided the VMX maintainers
don't object to the latter. Alternatively, make the operation
acceptable only for v == current || !d->tot_pages (matching
may_switch_mode()), which implies the vCPU can't be running.

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/08/2015 10:50 AM, Jan Beulich wrote:
>>>> On 08.07.15 at 16:40, <boris.ostrovsky@oracle.com> wrote:
>> On 07/08/2015 10:08 AM, Jan Beulich wrote:
>>>>>> On 08.07.15 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>>>> On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>>>>>> On 07.07.15 at 19:13, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>>>>>>>>> @@ -737,7 +737,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_32on64_domain(d);
>>>>>>>>>> +    compat = has_32bit_shinfo(d);
>>>>>>>>> Furthermore, looking at uses like this, tying such decisions to the
>>>>>>>>> shared info layout looks kind of odd. I think for documentation
>>>>>>>>> purposes we may need a differently named alias.
>>>>>>>> Yes, it does look odd, which is why I was asking in another thread about
>>>>>>>> having another field in domain structure (well, I was asking about
>>>>>>>> replacing has_32bit_shinfo but I think I can see now that wouldn't be
>>>>>>>> right).
>>>>>>>>
>>>>>>>> Are you suggesting a new macro, e.g.
>>>>>>>> #define is_32b_mode(d)    ((d)->arch.has_32bit_shinfo)
>>>>>>>>
>>>>>>>> or would it better to add new field? Or get_mode() hvm op, similar to
>>>>>>>> set_mode(), which can look, say, at EFER?
>>>>>>> If looking at EFER (plus perhaps CS) is right in all the cases you
>>>>>>> care about, then yes. And remember we already have
>>>>>>> hvm_guest_x86_mode().
>>>>>> Can't use hvm_guest_x86_mode(), it asserts on 'v != current'. But adding
>>>>>> new op just because of that seems to be an overkill since it would
>>>>>> essentially do what .guest_x86_mode() does. How about
>>>>>> hvm_guest_x86_mode_unsafe() (with a better name) and wrap
>>>>>> hvm_guest_x86_mode() with the ASSERT around it?
>>>>> svm_guest_x86_mode() doesn't depend on v == current, but
>>>>> vmx_guest_x86_mode() would first need to be made safe (or
>>>>> get an "unsafe" sibling implementation). With that, the ASSERT()
>>>>> could then check for current or non-running vCPU.
>>>> By checking for non-running you mean v->is_running? I am not sure it's
>>>> safe to do since is_running is set in context switch before VMCS is
>>>> loaded later, in vmx_do_resume().
>>> No, I rather thought about making sure the vCPU is paused (i.e.
>>> can't become running under your feet).
>> What would prevent it from becoming running if it is paused, right after
>> the ASSERT?
> True. I'm fine with dropping the ASSERT() after having done the
> proposed adjustment to the VMX side, provided the VMX maintainers
> don't object to the latter. Alternatively, make the operation
> acceptable only for v == current || !d->tot_pages (matching
> may_switch_mode()), which implies the vCPU can't be running.


As I started to update the patches I realized that in some cases 
(especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't 
have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the 
domain. d->vcpu[0] should work. Otherwise I'll either need a new field 
in struct domain or wrap has_32bit_shinfo into something PVH-specific, 
like is_32bit_pvh_vcpu().

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
> As I started to update the patches I realized that in some cases 
> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't 
> have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the 
> domain. d->vcpu[0] should work. Otherwise I'll either need a new field 
> in struct domain or wrap has_32bit_shinfo into something PVH-specific, 
> like is_32bit_pvh_vcpu().

Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
for PVH, especially if you also intend the tools to use the 64-bit
guest context variant even for 32-bit PVH? Once again - are you
intending to prohibit 32-bit PVH switching to 64-bit mode (which
would seem both wrong and possibly cumbersome to me)?

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/09/2015 03:02 AM, Jan Beulich wrote:
>>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
>> As I started to update the patches I realized that in some cases
>> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't
>> have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the
>> domain. d->vcpu[0] should work. Otherwise I'll either need a new field
>> in struct domain or wrap has_32bit_shinfo into something PVH-specific,
>> like is_32bit_pvh_vcpu().
> Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
> for PVH, especially if you also intend the tools to use the 64-bit
> guest context variant even for 32-bit PVH? Once again - are you
> intending to prohibit 32-bit PVH switching to 64-bit mode (which
> would seem both wrong and possibly cumbersome to me)?

With current PVH implementation I don't think we can switch. We are 
starting the guest in very much PV-like fashion. That's why we are 
getting into switch_compat() --- via XEN_DOMCTL_set_address_size.

For XEN_DOMCTL_get_address_size specifically we need to be able to 
figure out the mode *before* the guest is running because we use it to 
set cpuid bits in xc_cpuid_pv_policy(). So just that means we can't 
change the mode.

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 09.07.15 at 16:10, <boris.ostrovsky@oracle.com> wrote:
> On 07/09/2015 03:02 AM, Jan Beulich wrote:
>>>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
>>> As I started to update the patches I realized that in some cases
>>> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't
>>> have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the
>>> domain. d->vcpu[0] should work. Otherwise I'll either need a new field
>>> in struct domain or wrap has_32bit_shinfo into something PVH-specific,
>>> like is_32bit_pvh_vcpu().
>> Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
>> for PVH, especially if you also intend the tools to use the 64-bit
>> guest context variant even for 32-bit PVH? Once again - are you
>> intending to prohibit 32-bit PVH switching to 64-bit mode (which
>> would seem both wrong and possibly cumbersome to me)?
> 
> With current PVH implementation I don't think we can switch. We are 
> starting the guest in very much PV-like fashion. That's why we are 
> getting into switch_compat() --- via XEN_DOMCTL_set_address_size.
> 
> For XEN_DOMCTL_get_address_size specifically we need to be able to 
> figure out the mode *before* the guest is running because we use it to 
> set cpuid bits in xc_cpuid_pv_policy(). So just that means we can't 
> change the mode.

Okay - but is there code (being put) in place to refuse switch
attempts?

Jan

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/09/2015 10:17 AM, Jan Beulich wrote:
>>>> On 09.07.15 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>> On 07/09/2015 03:02 AM, Jan Beulich wrote:
>>>>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
>>>> As I started to update the patches I realized that in some cases
>>>> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't
>>>> have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the
>>>> domain. d->vcpu[0] should work. Otherwise I'll either need a new field
>>>> in struct domain or wrap has_32bit_shinfo into something PVH-specific,
>>>> like is_32bit_pvh_vcpu().
>>> Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
>>> for PVH, especially if you also intend the tools to use the 64-bit
>>> guest context variant even for 32-bit PVH? Once again - are you
>>> intending to prohibit 32-bit PVH switching to 64-bit mode (which
>>> would seem both wrong and possibly cumbersome to me)?
>> With current PVH implementation I don't think we can switch. We are
>> starting the guest in very much PV-like fashion. That's why we are
>> getting into switch_compat() --- via XEN_DOMCTL_set_address_size.
>>
>> For XEN_DOMCTL_get_address_size specifically we need to be able to
>> figure out the mode *before* the guest is running because we use it to
>> set cpuid bits in xc_cpuid_pv_policy(). So just that means we can't
>> change the mode.
> Okay - but is there code (being put) in place to refuse switch
> attempts?

No, I should add code to deal with this.


-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Boris Ostrovsky]

On 07/09/2015 10:30 AM, Boris Ostrovsky wrote:
> On 07/09/2015 10:17 AM, Jan Beulich wrote:
>>>>> On 09.07.15 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>> On 07/09/2015 03:02 AM, Jan Beulich wrote:
>>>>>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
>>>>> As I started to update the patches I realized that in some cases
>>>>> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't
>>>>> have VCPU (which is what hvm_guest_x86_mode() wants) but rather 
>>>>> only the
>>>>> domain. d->vcpu[0] should work. Otherwise I'll either need a new 
>>>>> field
>>>>> in struct domain or wrap has_32bit_shinfo into something 
>>>>> PVH-specific,
>>>>> like is_32bit_pvh_vcpu().
>>>> Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
>>>> for PVH, especially if you also intend the tools to use the 64-bit
>>>> guest context variant even for 32-bit PVH? Once again - are you
>>>> intending to prohibit 32-bit PVH switching to 64-bit mode (which
>>>> would seem both wrong and possibly cumbersome to me)?
>>> With current PVH implementation I don't think we can switch. We are
>>> starting the guest in very much PV-like fashion. That's why we are
>>> getting into switch_compat() --- via XEN_DOMCTL_set_address_size.
>>>
>>> For XEN_DOMCTL_get_address_size specifically we need to be able to
>>> figure out the mode *before* the guest is running because we use it to
>>> set cpuid bits in xc_cpuid_pv_policy(). So just that means we can't
>>> change the mode.
>> Okay - but is there code (being put) in place to refuse switch
>> attempts?
>
> No, I should add code to deal with this.

Forgot to include here --- so what is your preference wrt what I am 
asking in the first paragraph? d->vcpu[0], new field (or maybe a flag 
with bits per 32bit-pv and 32bit-pvh), or a PVH-wrapper for 
has_32bit_shinfo?

-boris

-boris

Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain

[Jan Beulich]

>>> On 09.07.15 at 18:05, <boris.ostrovsky@oracle.com> wrote:
> On 07/09/2015 10:30 AM, Boris Ostrovsky wrote:
>> On 07/09/2015 10:17 AM, Jan Beulich wrote:
>>>>>> On 09.07.15 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>>> On 07/09/2015 03:02 AM, Jan Beulich wrote:
>>>>>>>> On 08.07.15 at 22:57, <boris.ostrovsky@oracle.com> wrote:
>>>>>> As I started to update the patches I realized that in some cases
>>>>>> (especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't
>>>>>> have VCPU (which is what hvm_guest_x86_mode() wants) but rather 
>>>>>> only the
>>>>>> domain. d->vcpu[0] should work. Otherwise I'll either need a new 
>>>>>> field
>>>>>> in struct domain or wrap has_32bit_shinfo into something 
>>>>>> PVH-specific,
>>>>>> like is_32bit_pvh_vcpu().
>>>>> Shouldn't XEN_DOMCTL_get_address_size be handled HVM-like
>>>>> for PVH, especially if you also intend the tools to use the 64-bit
>>>>> guest context variant even for 32-bit PVH? Once again - are you
>>>>> intending to prohibit 32-bit PVH switching to 64-bit mode (which
>>>>> would seem both wrong and possibly cumbersome to me)?
>>>> With current PVH implementation I don't think we can switch. We are
>>>> starting the guest in very much PV-like fashion. That's why we are
>>>> getting into switch_compat() --- via XEN_DOMCTL_set_address_size.
>>>>
>>>> For XEN_DOMCTL_get_address_size specifically we need to be able to
>>>> figure out the mode *before* the guest is running because we use it to
>>>> set cpuid bits in xc_cpuid_pv_policy(). So just that means we can't
>>>> change the mode.
>>> Okay - but is there code (being put) in place to refuse switch
>>> attempts?
>>
>> No, I should add code to deal with this.
> 
> Forgot to include here --- so what is your preference wrt what I am 
> asking in the first paragraph? d->vcpu[0], new field (or maybe a flag 
> with bits per 32bit-pv and 32bit-pvh), or a PVH-wrapper for 
> has_32bit_shinfo?

To be honest, my preference would be to have none of those, and
have the whole thing more HVM-like from the beginning. Considering
that this isn't realistic, a PVH alias for has_32bit_shinfo() would seem
least ugly to me.

Jan

[PATCH v2 2/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 v2:
* Keep 64-bit initialization, change the mode to 32b if requested

 xen/arch/x86/domain.c         | 20 ++++++++++----------
 xen/arch/x86/hvm/hvm.c        | 15 ++++++++++++++-
 xen/arch/x86/hvm/vmx/vmcs.c   |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c    | 17 +++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 5 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b87f642..a0134d4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -377,25 +377,25 @@ 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 ( has_32bit_shinfo(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 +410,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 535d622..483d317 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2321,7 +2321,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;
@@ -6491,6 +6490,20 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
     return hvm_funcs.nhvm_intr_blocked(v);
 }
 
+int hvm_set_mode(struct vcpu *v, int mode)
+{
+    if ( mode == 4 )
+    {
+        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
+        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 fc29b89..8dcce3b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1763,6 +1763,22 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
                                          MSR_TYPE_W);
 }
 
+int vmx_set_mode(struct vcpu *v, int mode)
+{
+
+    if ( !is_pvh_vcpu(v) )
+        return 0;
+
+    if ( mode == 4 )
+    {
+        vmx_vmcs_enter(v);
+        __vmwrite(GUEST_CS_AR_BYTES, 0xc09b);
+        vmx_vmcs_exit(v);
+    }
+
+    return 0;
+}
+
 static struct hvm_function_table __initdata vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
@@ -1822,6 +1838,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
     .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf,
     .enable_msr_exit_interception = vmx_enable_msr_exit_interception,
+    .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 57f9605..b3b2d51 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -207,6 +207,7 @@ struct hvm_function_table {
                                   uint32_t *ecx, uint32_t *edx);
 
     void (*enable_msr_exit_interception)(struct domain *d);
+    int (*set_mode)(struct vcpu *v, int mode);
 };
 
 extern struct hvm_function_table hvm_funcs;
@@ -242,6 +243,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

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

[Jan Beulich]

>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -377,25 +377,25 @@ 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 ( has_32bit_shinfo(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 +410,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);
>      }
>  

And no respective change to switch_native()?

> @@ -6491,6 +6490,20 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>      return hvm_funcs.nhvm_intr_blocked(v);
>  }
>  
> +int hvm_set_mode(struct vcpu *v, int mode)
> +{
> +    if ( mode == 4 )
> +    {
> +        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
> +        hvm_update_guest_efer(v);
> +    }
> +
> +    if ( hvm_funcs.set_mode )
> +        return hvm_funcs.set_mode(v, mode);
> +
> +    return 0;
> +}

-EOPNOTSUPP?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1763,6 +1763,22 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>                                           MSR_TYPE_W);
>  }
>  
> +int vmx_set_mode(struct vcpu *v, int mode)
> +{
> +
> +    if ( !is_pvh_vcpu(v) )
> +        return 0;
> +
> +    if ( mode == 4 )
> +    {
> +        vmx_vmcs_enter(v);
> +        __vmwrite(GUEST_CS_AR_BYTES, 0xc09b);
> +        vmx_vmcs_exit(v);
> +    }
> +
> +    return 0;
> +}

Again -EOPNOTSUPP?

Jan

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

[Boris Ostrovsky]

On 07/07/2015 05:15 AM, Jan Beulich wrote:
>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -377,25 +377,25 @@ 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 ( has_32bit_shinfo(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 +410,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);
>>       }
>>   
> And no respective change to switch_native()?



>
>> @@ -6491,6 +6490,20 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>>       return hvm_funcs.nhvm_intr_blocked(v);
>>   }
>>   
>> +int hvm_set_mode(struct vcpu *v, int mode)
>> +{
>> +    if ( mode == 4 )
>> +    {
>> +        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
>> +        hvm_update_guest_efer(v);
>> +    }
>> +
>> +    if ( hvm_funcs.set_mode )
>> +        return hvm_funcs.set_mode(v, mode);
>> +
>> +    return 0;
>> +}
> -EOPNOTSUPP?

Why do you think this should be an error? I probably will need to update 
this to handle mode==8 for calls from switch_native() as you pointed out 
above but in general it seems to me it should be OK if this procedure 
doesn't do anything. Below too.

-boris

>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1763,6 +1763,22 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>>                                            MSR_TYPE_W);
>>   }
>>   
>> +int vmx_set_mode(struct vcpu *v, int mode)
>> +{
>> +
>> +    if ( !is_pvh_vcpu(v) )
>> +        return 0;
>> +
>> +    if ( mode == 4 )
>> +    {
>> +        vmx_vmcs_enter(v);
>> +        __vmwrite(GUEST_CS_AR_BYTES, 0xc09b);
>> +        vmx_vmcs_exit(v);
>> +    }
>> +
>> +    return 0;
>> +}
> Again -EOPNOTSUPP?
>
> Jan
>

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

[Jan Beulich]

>>> On 07.07.15 at 17:53, <boris.ostrovsky@oracle.com> wrote:
> On 07/07/2015 05:15 AM, Jan Beulich wrote:
>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -6491,6 +6490,20 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>>>       return hvm_funcs.nhvm_intr_blocked(v);
>>>   }
>>>   
>>> +int hvm_set_mode(struct vcpu *v, int mode)
>>> +{
>>> +    if ( mode == 4 )
>>> +    {
>>> +        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
>>> +        hvm_update_guest_efer(v);
>>> +    }
>>> +
>>> +    if ( hvm_funcs.set_mode )
>>> +        return hvm_funcs.set_mode(v, mode);
>>> +
>>> +    return 0;
>>> +}
>> -EOPNOTSUPP?
> 
> Why do you think this should be an error? I probably will need to update 
> this to handle mode==8 for calls from switch_native() as you pointed out 
> above but in general it seems to me it should be OK if this procedure 
> doesn't do anything. Below too.

Because the function then didn't do what it was asked for.

Jan

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

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* Replace is_pvh_vcpu() with is_pvh_domain()
* Only check that 32b versions of hypercalls are available in hvm_do_hypercall()

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

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 483d317..84d623d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4910,7 +4910,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),
@@ -4929,6 +4928,27 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
     [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
 };
 
+extern void compat_mmuext_op(void); /* XXX: need proper declaration */
+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)
@@ -4959,7 +4979,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;
@@ -5015,8 +5035,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;
@@ -5042,7 +5060,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 v2 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

[Jan Beulich]

>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
> @@ -4929,6 +4928,27 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>      [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
>  };
>  
> +extern void compat_mmuext_op(void); /* XXX: need proper declaration */

No XXX like this please. Just add a suitable declaration. And in fact
adding it here (with correct arguments, and without the comment)
would seem quite fine to me.

> +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
> +};
> +
> +

Just one blank line please.

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

Jan

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

[Boris Ostrovsky]

On 07/07/2015 05:20 AM, Jan Beulich wrote:
>>>> On 29.06.15 at 22:21, <boris.ostrovsky@oracle.com> wrote:
>> @@ -4929,6 +4928,27 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>>       [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
>>   };
>>   
>> +extern void compat_mmuext_op(void); /* XXX: need proper declaration */
> No XXX like this please. Just add a suitable declaration. And in fact
> adding it here (with correct arguments, and without the comment)
> would seem quite fine to me.

The XXX here was meant for eliciting comments from reviewers ;-)

-boris

[PATCH v2 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 920fe42..43e79f4 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