Re: [Xen-devel] [PATCH] x86/xpti: avoid copying L4 page table contents when possible

[Juergen Gross <jgross@xxxxxxxx>]

On 19/02/18 17:59, Jan Beulich wrote:
>>>> On 15.02.18 at 13:52, <jgross@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -510,6 +510,8 @@ void make_cr3(struct vcpu *v, mfn_t mfn)
>>  void write_ptbase(struct vcpu *v)
>>  {
>>      write_cr3(v->arch.cr3);
>> +    /* Setting copy_l4 unconditionally does no harm. */
>> +    get_cpu_info()->copy_l4 = true;
> 
> To limit code churn when 5-level page tables get introduced, can
> you please avoid using l4 when you really mean the top level page
> table (irrespective of how many levels there are)? I'm also not
> convinced of using "copy" in the field name - you want to indicate
> that the page table changed, yet what action the consumer of the
> flag takes doesn't really matter elsewhere. What about
> "root_pgt_changed"?

Works for me.

> Additionally - why would you set this for a 32-bit vCPU? Further,
> what about clearing the flag when context switching out a PV
> vCPU? I realize both are benign with just the single current
> consumer plus the fact that the flag will always be set when
> context switching in a (64-bit) PV vCPU, but the value of the
> flag could be misleading during debugging, and could become an
> actual problem if another consumer appeared.

In fact I have another patch pending which requires to special
case the 64-bit PV domain here, resulting in setting the bit
for those doamins only. I can move that special casing into this
patch and add resetting the flag for other domains.

> 
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -46,10 +46,13 @@ restore_all_guest:
>>  .Lrag_cr3_start:
>>          mov   VCPU_cr3(%rbx), %r9
>>          GET_STACK_END(dx)
>> -        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
>> +        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rax
>> +        cmpb  $0, STACK_CPUINFO_FIELD(copy_l4)(%rdx)
>> +        je    .Lrag_copyend
>> +        movb  $0, STACK_CPUINFO_FIELD(copy_l4)(%rdx)
> 
> Use %bl instead of $0 in both cases (with a comment)?

Do you really think we should add such a micro optimization relying
on the vcpu pointer being aligned to page boundary? And are you sure
adding the register dependency isn't hurting more than we will gain
from saving an instruction byte?

> 
>> @@ -65,6 +68,7 @@ restore_all_guest:
>>          sub   $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>>                  ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
>>          rep movsq
>> +.Lrag_copyend:
> 
> .Lrag_copy_end (or .Lrag_copy_done)

Okay.

> 
>> --- a/xen/include/asm-x86/flushtlb.h
>> +++ b/xen/include/asm-x86/flushtlb.h
>> @@ -103,6 +103,8 @@ void write_cr3(unsigned long cr3);
>>  #define FLUSH_VA_VALID   0x800
>>   /* Flush CPU state */
>>  #define FLUSH_VCPU_STATE 0x1000
>> + /* Update XPTI root page table */
>> +#define XPTI_L4_UPDATE   0x2000
> 
> Please keep this in line with its siblings, i.e. have it have a FLUSH_
> prefix.

Okay.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel