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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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"?

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.

> --- 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)?

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

> --- 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.

Jan


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