Re: [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 08 Aug 2013 08:29:27 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 08.08.13 at 03:40, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Wed, 7 Aug 2013 11:24:42 +0100
> > George Dunlap <George.Dunlap@eu.citrix.com> 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