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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel