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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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