From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes Date: Thu, 8 Aug 2013 17:12:55 -0700 Message-ID: <20130808171255.1daa3b6a@mantra.us.oracle.com> References: <1374631171-15224-1-git-send-email-mukesh.rathor@oracle.com> <1374631171-15224-11-git-send-email-mukesh.rathor@oracle.com> <20130807184006.14862c33@mantra.us.oracle.com> <520364F702000078000EA213@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <520364F702000078000EA213@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: George Dunlap , "keir.xen@gmail.com" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On Thu, 08 Aug 2013 08:29:27 +0100 "Jan Beulich" wrote: > >>> On 08.08.13 at 03:40, Mukesh Rathor > >>> wrote: > > On Wed, 7 Aug 2013 11:24:42 +0100 > > George Dunlap wrote: > > ......... > >> > index 412971e..ece11e4 100644 > >> > --- a/xen/arch/x86/mm.c > >> > +++ b/xen/arch/x86/mm.c > >> > @@ -4334,6 +4334,9 @@ void destroy_gdt(struct vcpu *v) > >> > int i; > >> > unsigned long pfn; > >> > > >> > + if ( is_pvh_vcpu(v) ) > >> > + return; > >> > >> There seems to be some inconsistency with where this is supposed > >> to be checked -- in domain_relinquish_resources(), destroy_gdt() > >> is only called for pv domains (gated on is_pv_domain); but in > >> arch_set_info_guest(), it *was* gated on being PV, but with the PVH > >> changes it's still being called. > >> > >> Either this should only be called for PV domains (and this check > >> should be an ASSERT), or it should be called regardless of the > >> type of domain. I prefer the first if possible. > > > > In the original version it was being called for pv domains only, > > and I had checks in the caller. But, Jan preferred the check in > > destroy_gdt() so I moved it to destroy_gdt(). > > But that perspective may have changed with other code changes: > If all callers now suppress the call for PVH guests, this should > indeed be an assertion (if anything). If all but one caller checks > for PV (or are in PV only code paths), the better approach now may > still be to have the one odd caller do the check and have an > assertion in the function. Iirc it was at least two call sites you > had to adjust originally, which then warranted to do the check in > just one place (in the function itself). The only change I see relevent to this between my patch and now, is fewer callers in arch_set_info_guest(). Change is small enough and I'll make it so that destroy_gdt() gets called for PV only, ie, the check is in the caller. thanks mukesh