While all present callers want to act on "current", stack dumping for
HVM vCPU-s will require the function to be able to act on a remote vCPU.
To avoid touching all present callers, convert the existing function to
an inline wrapper around the extend new one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Alternatively the actual dumping patch could avoid using this more
elaborate function and, ignoring access checks, simply add in the SS
segment base itself (if needed in the first place).
---
v3: New.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2526,7 +2526,8 @@ int hvm_set_cr4(unsigned long value, boo
     return X86EMUL_OKAY;
 }
 
-bool_t hvm_virtual_to_linear_addr(
+bool hvm_vcpu_virtual_to_linear(
+    struct vcpu *v,
     enum x86_segment seg,
     const struct segment_register *reg,
     unsigned long offset,
@@ -2535,8 +2536,9 @@ bool_t hvm_virtual_to_linear_addr(
     const struct segment_register *active_cs,
     unsigned long *linear_addr)
 {
-    const struct vcpu *curr = current;
     unsigned long addr = offset, last_byte;
+    const struct cpu_user_regs *regs = v == current ? guest_cpu_user_regs()
+                                                    : &v->arch.user_regs;
     bool_t okay = 0;
 
     /*
@@ -2547,7 +2549,7 @@ bool_t hvm_virtual_to_linear_addr(
      */
     ASSERT(seg < x86_seg_none);
 
-    if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PE) )
+    if ( !(v->arch.hvm.guest_cr[0] & X86_CR0_PE) )
     {
         /*
          * REAL MODE: Don't bother with segment access checks.
@@ -2555,7 +2557,7 @@ bool_t hvm_virtual_to_linear_addr(
          */
         addr = (uint32_t)(addr + reg->base);
     }
-    else if ( (guest_cpu_user_regs()->eflags & X86_EFLAGS_VM) &&
+    else if ( (regs->eflags & X86_EFLAGS_VM) &&
               is_x86_user_segment(seg) )
     {
         /* VM86 MODE: Fixed 64k limits on all user segments. */
@@ -2564,7 +2566,7 @@ bool_t hvm_virtual_to_linear_addr(
         if ( max(offset, last_byte) >> 16 )
             goto out;
     }
-    else if ( hvm_long_mode_active(curr) &&
+    else if ( hvm_long_mode_active(v) &&
               (is_x86_system_segment(seg) || active_cs->l) )
     {
         /*
@@ -2636,7 +2638,7 @@ bool_t hvm_virtual_to_linear_addr(
         else if ( last_byte > reg->limit )
             goto out; /* last byte is beyond limit */
         else if ( last_byte < offset &&
-                  curr->domain->arch.cpuid->x86_vendor == X86_VENDOR_AMD )
+                  v->domain->arch.cpuid->x86_vendor == X86_VENDOR_AMD )
             goto out; /* access wraps */
     }
 
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -314,7 +314,9 @@ enum hvm_access_type {
     hvm_access_read,
     hvm_access_write
 };
-bool_t hvm_virtual_to_linear_addr(
+
+bool hvm_vcpu_virtual_to_linear(
+    struct vcpu *v,
     enum x86_segment seg,
     const struct segment_register *reg,
     unsigned long offset,
@@ -323,6 +325,19 @@ bool_t hvm_virtual_to_linear_addr(
     const struct segment_register *active_cs,
     unsigned long *linear_addr);
 
+static inline bool hvm_virtual_to_linear_addr(
+    enum x86_segment seg,
+    const struct segment_register *reg,
+    unsigned long offset,
+    unsigned int bytes,
+    enum hvm_access_type access_type,
+    const struct segment_register *active_cs,
+    unsigned long *linear)
+{
+    return hvm_vcpu_virtual_to_linear(current, seg, reg, offset, bytes,
+                                      access_type, active_cs, linear);
+}
+
 void *hvm_map_guest_frame_rw(unsigned long gfn, bool_t permanent,
                              bool_t *writable);
 void *hvm_map_guest_frame_ro(unsigned long gfn, bool_t permanent);

On Tue, Sep 21, 2021 at 09:19:37AM +0200, Jan Beulich wrote:
> While all present callers want to act on "current", stack dumping for
> HVM vCPU-s will require the function to be able to act on a remote vCPU.
> To avoid touching all present callers, convert the existing function to
> an inline wrapper around the extend new one.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Alternatively the actual dumping patch could avoid using this more
> elaborate function and, ignoring access checks, simply add in the SS
> segment base itself (if needed in the first place).
> ---
> v3: New.
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2526,7 +2526,8 @@ int hvm_set_cr4(unsigned long value, boo
>      return X86EMUL_OKAY;
>  }
>  
> -bool_t hvm_virtual_to_linear_addr(
> +bool hvm_vcpu_virtual_to_linear(
> +    struct vcpu *v,
>      enum x86_segment seg,
>      const struct segment_register *reg,
>      unsigned long offset,
> @@ -2535,8 +2536,9 @@ bool_t hvm_virtual_to_linear_addr(
>      const struct segment_register *active_cs,
>      unsigned long *linear_addr)
>  {
> -    const struct vcpu *curr = current;
>      unsigned long addr = offset, last_byte;
> +    const struct cpu_user_regs *regs = v == current ? guest_cpu_user_regs()
> +                                                    : &v->arch.user_regs;
>      bool_t okay = 0;

Since you change the function return type to bool, you should also
change the type of the returned variable IMO. It's just a two line
change.

Also is it worth adding some check that the remote vCPU is paused? Or
else you might get inconsistent results by using data that's stale  by
the time Xen acts on it.

Thanks, Roger.

On 23.09.2021 10:09, Roger Pau Monné wrote:
> On Tue, Sep 21, 2021 at 09:19:37AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2526,7 +2526,8 @@ int hvm_set_cr4(unsigned long value, boo
>>      return X86EMUL_OKAY;
>>  }
>>  
>> -bool_t hvm_virtual_to_linear_addr(
>> +bool hvm_vcpu_virtual_to_linear(
>> +    struct vcpu *v,
>>      enum x86_segment seg,
>>      const struct segment_register *reg,
>>      unsigned long offset,
>> @@ -2535,8 +2536,9 @@ bool_t hvm_virtual_to_linear_addr(
>>      const struct segment_register *active_cs,
>>      unsigned long *linear_addr)
>>  {
>> -    const struct vcpu *curr = current;
>>      unsigned long addr = offset, last_byte;
>> +    const struct cpu_user_regs *regs = v == current ? guest_cpu_user_regs()
>> +                                                    : &v->arch.user_regs;
>>      bool_t okay = 0;
> 
> Since you change the function return type to bool, you should also
> change the type of the returned variable IMO. It's just a two line
> change.

Can do; I would have viewed this as an unrelated change: While the
first change needed indeed is on an adjacent line (above), the other
one isn't.

> Also is it worth adding some check that the remote vCPU is paused? Or
> else you might get inconsistent results by using data that's stale  by
> the time Xen acts on it.

I did ask myself the same question. I did conclude that even if the
remote vCPU is paused at the time of the check, it may not be anymore
immediately after, so the information returned might be stale anyway.
I further looked at {hvm,vmx}_get_segment_register() to see what they
did - nothing. It is only now that I've also checked
svm_get_segment_register(), which - interestingly - has such a check.
I will copy the ASSERT() used there.

Jan

On Thu, Sep 23, 2021 at 12:34:48PM +0200, Jan Beulich wrote:
> On 23.09.2021 10:09, Roger Pau Monné wrote:
> > On Tue, Sep 21, 2021 at 09:19:37AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -2526,7 +2526,8 @@ int hvm_set_cr4(unsigned long value, boo
> >>      return X86EMUL_OKAY;
> >>  }
> >>  
> >> -bool_t hvm_virtual_to_linear_addr(
> >> +bool hvm_vcpu_virtual_to_linear(
> >> +    struct vcpu *v,
> >>      enum x86_segment seg,
> >>      const struct segment_register *reg,
> >>      unsigned long offset,
> >> @@ -2535,8 +2536,9 @@ bool_t hvm_virtual_to_linear_addr(
> >>      const struct segment_register *active_cs,
> >>      unsigned long *linear_addr)
> >>  {
> >> -    const struct vcpu *curr = current;
> >>      unsigned long addr = offset, last_byte;
> >> +    const struct cpu_user_regs *regs = v == current ? guest_cpu_user_regs()
> >> +                                                    : &v->arch.user_regs;
> >>      bool_t okay = 0;
> > 
> > Since you change the function return type to bool, you should also
> > change the type of the returned variable IMO. It's just a two line
> > change.
> 
> Can do; I would have viewed this as an unrelated change: While the
> first change needed indeed is on an adjacent line (above), the other
> one isn't.
> 
> > Also is it worth adding some check that the remote vCPU is paused? Or
> > else you might get inconsistent results by using data that's stale  by
> > the time Xen acts on it.
> 
> I did ask myself the same question. I did conclude that even if the
> remote vCPU is paused at the time of the check, it may not be anymore
> immediately after, so the information returned might be stale anyway.
> I further looked at {hvm,vmx}_get_segment_register() to see what they
> did - nothing. It is only now that I've also checked
> svm_get_segment_register(), which - interestingly - has such a check.
> I will copy the ASSERT() used there.

Thanks, with that:

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

Roger.