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

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v3 6/9] x86/HVM: convert hvm_virtual_to_linear_addr() to be remote-capable
Date: Thu, 23 Sep 2021 12:34:48 +0200	[thread overview]
Message-ID: <46f90ac6-a4f0-d00a-2956-a0e4b351f698@suse.com> (raw)
In-Reply-To: <YUw2RLtI73QNjaPE@MacBook-Air-de-Roger.local>

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