It's not really needed and has been misleading me more than once to try
and spot its "actual" use(s). It should really have been dropped when
the 32-bit specific logic was purged from here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
 
     if ( v != current )
     {
-        struct vcpu *vcpu;
-
         if ( !guest_kernel_mode(v, regs) )
         {
             printk("User mode stack\n");
             return;
         }
 
-        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
-        if ( !vcpu )
+        if ( maddr_get_owner(read_cr3()) != v->domain )
         {
             stack_page = stack = do_page_walk(v, (unsigned long)stack);
             if ( (unsigned long)stack < PAGE_SIZE )

On Wed, Sep 29, 2021 at 11:43:32AM +0200, Jan Beulich wrote:
> It's not really needed and has been misleading me more than once to try
> and spot its "actual" use(s). It should really have been dropped when
> the 32-bit specific logic was purged from here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

As it makes the current code clearer. I have one question/concern
below.

> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
>  
>      if ( v != current )
>      {
> -        struct vcpu *vcpu;
> -
>          if ( !guest_kernel_mode(v, regs) )
>          {
>              printk("User mode stack\n");
>              return;
>          }
>  
> -        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
> -        if ( !vcpu )
> +        if ( maddr_get_owner(read_cr3()) != v->domain )

Wouldn't it be more accurate to check that the current loaded cr3
matches the one used by v?

AFAICT we don't load the cr3 from v, so it's still possible to have
diverging per-vcpu page tables and thus end up dumping wrong data?

Thanks, Roger.

On 18.10.2021 16:35, Roger Pau Monné wrote:
> On Wed, Sep 29, 2021 at 11:43:32AM +0200, Jan Beulich wrote:
>> It's not really needed and has been misleading me more than once to try
>> and spot its "actual" use(s). It should really have been dropped when
>> the 32-bit specific logic was purged from here.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> As it makes the current code clearer. I have one question/concern
> below.
> 
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
>>  
>>      if ( v != current )
>>      {
>> -        struct vcpu *vcpu;
>> -
>>          if ( !guest_kernel_mode(v, regs) )
>>          {
>>              printk("User mode stack\n");
>>              return;
>>          }
>>  
>> -        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
>> -        if ( !vcpu )
>> +        if ( maddr_get_owner(read_cr3()) != v->domain )
> 
> Wouldn't it be more accurate to check that the current loaded cr3
> matches the one used by v?
> 
> AFAICT we don't load the cr3 from v, so it's still possible to have
> diverging per-vcpu page tables and thus end up dumping wrong data?

I think truly per-CPU page tables in a PV guest would be quite
cumbersome to have. And iirc (it's been over 12 years since introducing
that logic) the check also isn't about the CR3 we're on being the one
associated with the present vCPU, but merely whether we can expect that
page to be mapped. This would also fall apart when a remote vCPU's
stack isn't accessible on the current CPU for whichever other reason.
I'd be inclined to further tighten this (and force more cases through
do_page_walk()) when we see evidence that we need doing so.

Jan