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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 08/08/13 08:29, Jan Beulich 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).

Overall I think checking before calling would make the code easier to 
understand.  All the functions which call this have is_foo_domain() 
sprinkled all over anyway, and so it's easier for someone reading the 
code to understand immediately that HVM and PVH guests don't need their 
gdt destroyed.  But my main point was that if we check inside the 
function, we should avoid checking outside the function for consistency.

  -George