From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 1/4] x86/compat: Test whether guest has 32b shinfo instead of being a PV 32b domain Date: Tue, 07 Jul 2015 11:46:57 -0400 Message-ID: <559BF471.7080904@oracle.com> References: <1435609282-1383-1-git-send-email-boris.ostrovsky@oracle.com> <1435609282-1383-2-git-send-email-boris.ostrovsky@oracle.com> <559BB3DF020000780008D3A6@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559BB3DF020000780008D3A6@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: elena.ufimtseva@oracle.com, tim@xen.org, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, roger.pau@citrix.com List-Id: xen-devel@lists.xenproject.org On 07/07/2015 05:11 AM, Jan Beulich wrote: >>>> On 29.06.15 at 22:21, wrote: >> In preparation for enabling 32-bit PVH guests replace a number of guest mode's >> tests that assume a PV guest with has_32bit_shinfo() that can be applicable >> to >> both PV and PVH guests. > Apart from this apparently needing re-basing, I also think this should > be swapped with patch 2, as right now it doesn't make much sense to > distinguish the two checks. > >> @@ -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? > >> @@ -1721,9 +1721,7 @@ unsigned long hypercall_create_continuation( >> else >> curr->arch.hvm_vcpu.hcall_preempted = 1; >> >> - if ( is_pv_vcpu(curr) ? >> - !is_pv_32bit_vcpu(curr) : >> - curr->arch.hvm_vcpu.hcall_64bit ) >> + if ( !has_32bit_shinfo(curr->domain) ) > This is not a valid replacement - hcall_64bit depends on the mode > the vCPU currently is in, while has_32bit_shinfo() doesn't. Right, and I don't think this change is needed anyway since hvm_do_hypercall() will set hcall_64bit for PVH guests as well (when the guest is in 64-bit mode) > >> --- 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()? -boris