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

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



_______________________________________________
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®.