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/08/2015 10:50 AM, Jan Beulich wrote:
> > > > 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.

As I started to update the patches I realized that in some cases 
(especially in arch_do_domctl():XEN_DOMCTL_get_address_size) we don't 
have VCPU (which is what hvm_guest_x86_mode() wants) but rather only the 
domain. d->vcpu[0] should work. Otherwise I'll either need a new field 
in struct domain or wrap has_32bit_shinfo into something PVH-specific, 
like is_32bit_pvh_vcpu().
-boris

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