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: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).