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

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

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.

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
-        if ( !is_pv_32on64_domain(d) )
+        if ( !has_32bit_shinfo(d) )
                ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
                ret = copy_from_guest(c.cmp,
@@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
-        if ( !is_pv_32on64_domain(d) )
+        if ( !has_32bit_shinfo(d) )
                ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
                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
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).
No, they use the 'else' part, I just confirmed it. 'd' in
is_pv_32on64_domain() is domain for which domctl is being called, not
domain that is making the call (which is what I suspect the original
intent was).
Oh, yes, of course they do - how did I overlook the "!" ? Yet
that doesn't help me understand the question: Isn't it obvious
that if libxc expects vcpu_guest_context_x86_32_t, then the
hypervisor also needs to supply that one (and not the 64-bit
counterpart)? Or are you asking why the format matches the
subject domain's word width, not the calling domain's?

Yes, this was the question.

This has
historical reasons: A 32-bit domain saved on a 64-bit hypervisor
needed to be restorable by a 32-bit hypervisor when that still
existed. This could likely be changed nowadays; ARM and the
HVM case must be dealt with in the tools somehow anyway.

OK, then I don't need those two changes in do_domctl().


Xen-devel mailing list



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