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é <[email protected]>
Roger.
