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: Wed, 08 Jul 2015 16:57:28 -0400 Message-ID: <559D8EB8.9040804@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> <559BF471.7080904@oracle.com> <559C173F020000780008DBAD@mail.emea.novell.com> <559C08AD.106@oracle.com> <559CE3DD020000780008DEAA@mail.emea.novell.com> <559D2CD8.7050807@oracle.com> <559D4AF2020000780008E463@mail.emea.novell.com> <559D3662.4070803@oracle.com> <559D54E2020000780008E4D7@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: <559D54E2020000780008E4D7@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/08/2015 10:50 AM, Jan Beulich wrote: >>>> On 08.07.15 at 16:40, wrote: >> On 07/08/2015 10:08 AM, Jan Beulich wrote: >>>>>> On 08.07.15 at 15:59, wrote: >>>> On 07/08/2015 02:48 AM, Jan Beulich wrote: >>>>>>>> On 07.07.15 at 19:13, wrote: >>>>>> On 07/07/2015 12:15 PM, Jan Beulich wrote: >>>>>>>>>> On 07.07.15 at 17:46, wrote: >>>>>>>> On 07/07/2015 05:11 AM, Jan Beulich wrote: >>>>>>>>>>>> On 29.06.15 at 22:21, 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