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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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?
> > > > --- a/xen/common/domctl.c
> > > > +++ b/xen/common/domctl.c
> > > > @@ -496,7 +496,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > > > u_domctl)
> > > >                break;
> > > >    
> > > >    #ifdef CONFIG_COMPAT
> > > > -        if ( !is_pv_32on64_domain(d) )
> > > > +        if ( !has_32bit_shinfo(d) )
> > > >                ret = copy_from_guest(c.nat, op->u.vcpucontext.ctxt, 1);
> > > >            else
> > > >                ret = copy_from_guest(c.cmp,
> > > > @@ -902,7 +902,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > > > u_domctl)
> > > >            vcpu_unpause(v);
> > > >    
> > > >    #ifdef CONFIG_COMPAT
> > > > -        if ( !is_pv_32on64_domain(d) )
> > > > +        if ( !has_32bit_shinfo(d) )
> > > >                ret = copy_to_guest(op->u.vcpucontext.ctxt, c.nat, 1);
> > > >            else
> > > >                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
> > > mode?
> > 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).
-boris

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