Re: [PATCH for-4.7 4/4] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/05/16 14:57, Jan Beulich wrote:
>>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>>  
>>  static void svm_invlpg_intercept(unsigned long vaddr)
>>  {
>> -    struct vcpu *curr = current;
>>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
>> -    paging_invlpg(curr, vaddr);
>> -    svm_asid_g_invlpg(curr, vaddr);
>> +    paging_invlpg(current, vaddr);
>> +}
>> +
>> +static void svm_invlpg(unsigned long vaddr)
>> +{
>> +    svm_asid_g_invlpg(current, vaddr);
> Since paging_invlpg() already gets passed a struct vcpu *, having
> it hand it to ->invlpg() would seem quite natural.

Ok.

>
>> @@ -2432,10 +2432,14 @@ static void vmx_dr_access(unsigned long 
>> exit_qualification,
>>  
>>  static void vmx_invlpg_intercept(unsigned long vaddr)
>>  {
>> -    struct vcpu *curr = current;
>>      HVMTRACE_LONG_2D(INVLPG, /*invlpga=*/ 0, TRC_PAR_LONG(vaddr));
>> -    if ( paging_invlpg(curr, vaddr) && cpu_has_vmx_vpid )
> So the previous dependency on paging_invlpg()'s return value gets
> moved into that function (making the call here conditional). While
> that's fine for VMX, SVM previously didn't pay attention to that
> return value, and hence you're altering behavior in a way not
> spelled out in the description (and to be honest I'm also not 100%
> certain that change is correct).

I thought I had included this in the commit message, but it appears it
got lost.

AMD is over-the-top with its TLB flushing generally, and will allocate a
new ASID rather than shooting a single mapping out of the current ASID. 
The new svm_invlpg() should be a simple invlpga() call with the in-use
ASID.  However, the instruction emulator forwards invlpga to invlpg and
loses the ASID, meaning that the only reason it functions is because
this case over-flushes at the moment.  I opted not to break that usecase.

However, for the cases where paging_invlpg() returns 0, there is no need
to allocate an ASID at all, as there are no mapping needing invalidation
which could have been cached in the TLB.  As such, this change is a
performance improvement, albeit a minor one as the common case is dealt
with directly in hardware without Xen's interaction.

>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -6478,6 +6478,19 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
>>      return bad_pages;
>>  }
>>  
>> +bool_t paging_invlpg(struct vcpu *v, unsigned long va)
>> +{
>> +    bool_t invl = paging_mode_external(v->domain)
>> +        ? is_canonical_address(va) : __addr_ok(va);
>> +
>> +    if ( invl )
>> +        invl = paging_get_hostmode(v)->invlpg(v, va);
>> +    if ( invl && is_hvm_domain(v->domain) )
> has_hvm_container_domain() (or paging_mode_external(),
> implying the former)?

I could have sworn I already fixed this...  Will do for v2.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel