Re: [PATCH v3 6/9] x86/HVM: convert hvm_virtual_to_linear_addr() to be remote-capable

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>

Roger.