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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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