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