[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 08.07.15 at 16:40, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 07/08/2015 10:08 AM, Jan Beulich wrote:
>>>>> On 08.07.15 at 15:59, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 07/08/2015 02:48 AM, Jan Beulich wrote:
>>>>>>> On 07.07.15 at 19:13, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 07/07/2015 12:15 PM, Jan Beulich wrote:
>>>>>>>>> On 07.07.15 at 17:46, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote:
>>>>>>>>>>> On 29.06.15 at 22:21, <boris.ostrovsky@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.