Re: [PATCH RFC v12 05/21] Introduce pv guest type and has_hvm_container macros

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

On 20/09/13 09:11, Jan Beulich wrote:
>>>> On 19.09.13 at 18:27, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 18/09/13 14:46, Jan Beulich wrote:
>>>>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@eu.citrix.com> 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