From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH RFC v12 05/21] Introduce pv guest type and has_hvm_container macros Date: Fri, 20 Sep 2013 10:23:39 +0100 Message-ID: <523C141B.9040103@eu.citrix.com> References: <1379089521-25720-1-git-send-email-george.dunlap@eu.citrix.com> <1379089521-25720-6-git-send-email-george.dunlap@eu.citrix.com> <5239CAD002000078000F4616@nat28.tlf.novell.com> <523B2609.7030809@eu.citrix.com> <523C1F3502000078000F4D51@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VMwwE-0002Sr-H2 for xen-devel@lists.xenproject.org; Fri, 20 Sep 2013 09:23:46 +0000 In-Reply-To: <523C1F3502000078000F4D51@nat28.tlf.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: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 20/09/13 09:11, Jan Beulich wrote: >>>> On 19.09.13 at 18:27, George Dunlap wrote: >> On 18/09/13 14:46, Jan Beulich wrote: >>>>>> On 13.09.13 at 18:25, George Dunlap wrote: >>>> @@ -72,7 +72,7 @@ void *map_domain_page(unsigned long mfn) >>>> #endif >>>> >>>> v = mapcache_current_vcpu(); >>>> - if ( !v || is_hvm_vcpu(v) ) >>>> + if ( !v || has_hvm_container_vcpu(v) ) >>>> return mfn_to_virt(mfn); >>>> >>>> dcache = &v->domain->arch.pv_domain.mapcache; >>>> @@ -177,7 +177,7 @@ void unmap_domain_page(const void *ptr) >>>> ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END); >>>> >>>> v = mapcache_current_vcpu(); >>>> - ASSERT(v && !is_hvm_vcpu(v)); >>>> + ASSERT(v && is_pv_vcpu(v)); >>> This is inconsistent with the above change to map_domain_page() >>> and the changes later in this file. >> You mean, making this "is_pv_vcpu" instead of >> "!has_hvm_container_vcpu"? I guess that's true. > Actually I meant the cited one above to be converted - the map > cache, following your argumentation further down, is something > that is _not_ needed when there is a HVM container (i.e. external > paging); the need for it is not tied to the PV-ness of a guest. > > But anyway, this is-PV and doesn't-have-HVM-container distinction > is sort of fuzzy anyway, and you seem to half way agree following > later parts of your response. > >>>> --- a/xen/arch/x86/traps.c >>>> +++ b/xen/arch/x86/traps.c >>>> @@ -119,6 +119,7 @@ static void show_guest_stack(struct vcpu *v, struct cpu_user_regs *regs) >>>> unsigned long *stack, addr; >>>> unsigned long mask = STACK_SIZE; >>>> >>>> + /* Avoid HVM as we don't know what the stack looks like */ >>>> if ( is_hvm_vcpu(v) ) >>> So why do you not change this one to !is_pv_vcpu()? In particular >>> the do_page_walk() further down is inherently PV. >> I guess it depends. I was thinking that the reason we didn't do this >> for HVM guests was that there was no guarantee what the stack looked >> like -- it looks like another aspect is that for VMs that are not >> current, whether the PTs are controlled by Xen or the guest is a factor. >> >> But if we make do_page_walk gated on is_pv_vcpu(), then it seems like it >> should work when v==current, and fail gracefully when v!=current. > No, the right (long term) thing would in fact be to do the right > form of page walk in the PVH case here too. After all you're right > that the original reason for filtering out HVM guests was that we > can't make assumptions about their stacks (whether making any > such assumptions of PV guests is really appropriate is another > question). Hence I wouldn't want to see do_page_walk() to be > fiddled with; suppressing the call in show_guest_stack() would be > fine with me. What I meant was, do_page_walk() already had a conditional at the top to return NULL if is_hvm_domain() was true, and callers should all be written to handle that case. If we change that to if(!is_pv_domain()) (which should probably be done anyway), then for PVH domains we'll get a stack for v==current, but not for others. do_page_walk() can be extended for PVH domains at some point in the future. -George