Re: [PATCH v3 2/5] x86: use PDEP/PEXT for maddr/direct-map-offset conversion when available

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

On 17/08/2018 08:21, Jan Beulich wrote:
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -186,6 +186,20 @@ void ret_from_intr(void);
>          UNLIKELY_END_SECTION "\n"          \
>          ".Llikely." #tag ".%=:"
>  
> +#define LINKONCE_PROLOGUE(sym)                    \
> +        ".ifndef " sym() "\n\t"                   \
> +        ".pushsection " sym(.gnu.linkonce.t.) "," \

This definitely warrants a comment and a change of name, seeing as sym
isn't a symbol.  Its a macro which gives you a string back.

> +                      "\"ax\",@progbits\n\t"      \
> +        ".p2align 4\n"                            \
> +        sym() ":"
> +
> +#define LINKONCE_EPILOGUE(sym)                    \
> +        ".weak " sym() "\n\t"                     \
> +        ".type " sym() ", @function\n\t"          \
> +        ".size " sym() ", . - " sym() "\n\t"      \
> +        ".popsection\n\t"                         \
> +        ".endif"
> +
>  #endif
>  
>  /* "Raw" instruction opcodes */
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -57,8 +64,10 @@ extern unsigned long xen_virt_end;
>  #define pdx_to_virt(pdx) ((void *)(DIRECTMAP_VIRT_START + \
>                                     ((unsigned long)(pdx) << PAGE_SHIFT)))
>  
> -static inline unsigned long __virt_to_maddr(unsigned long va)
> +static always_inline paddr_t __virt_to_maddr(unsigned long va)
>  {
> +    paddr_t ma;
> +
>      ASSERT(va < DIRECTMAP_VIRT_END);
>      if ( va >= DIRECTMAP_VIRT_START )
>          va -= DIRECTMAP_VIRT_START;
> @@ -71,16 +80,77 @@ static inline unsigned long __virt_to_ma
>  
>          va += xen_phys_start - XEN_VIRT_START;
>      }
> -    return (va & ma_va_bottom_mask) |
> -           ((va << pfn_pdx_hole_shift) & ma_top_mask);
> +
> +#ifdef CONFIG_INDIRECT_THUNK /* V modifier available? */
> +#define SYMNAME(pfx...) #pfx "do2ma_%V[ma]_%V[off]"
> +    alternative_io("call " SYMNAME() "\n\t"
> +                   LINKONCE_PROLOGUE(SYMNAME) "\n\t"
> +                   "mov %[shift], %%ecx\n\t"
> +                   "mov %[off], %[ma]\n\t"
> +                   "and %[bmask], %[ma]\n\t"
> +                   "shl %%cl, %[off]\n\t"
> +                   "and %[tmask], %[off]\n\t"
> +                   "or %[off], %[ma]\n\t"
> +                   "ret\n\t"
> +                   LINKONCE_EPILOGUE(SYMNAME),
> +                   "pdep %[mask], %[off], %[ma]", X86_FEATURE_BMI2,

The compiler understanding V doesn't imply that the assembler
understands pdep

> +                   ASM_OUTPUT2([ma] "=&r" (ma), [off] "+r" (va)),
> +                   [mask] "m" (ma_real_mask),
> +                   [shift] "m" (pfn_pdx_hole_shift),
> +                   [bmask] "m" (ma_va_bottom_mask),
> +                   [tmask] "m" (ma_top_mask)
> +                   : "ecx");
> +#undef SYMNAME
> +#else
> +    alternative_io("call do2ma",
> +                   /* pdep ma_real_mask(%rip), %rdi, %rax */
> +                   ".byte 0xc4, 0xe2, 0xc3, 0xf5, 0x05\n\t"
> +                   ".long ma_real_mask - 4 - .",
> +                   X86_FEATURE_BMI2,
> +                   ASM_OUTPUT2("=a" (ma), "+D" (va)), "m" (ma_real_mask)
> +                   : "rcx", "rdx", "rsi", "r8", "r9", "r10", "r11");
> +#endif

This is a massive clobber list in a function you've forced always
inline, and I can't see it doing nice things to the callsites.  TBH,
this still feels over-complicated for what it wants to be.

Why not implement one single function in assembly that doesn't have
usual C calling conventions and can clobber %ecx and one other, and use
that?

It avoids the need for potentially 256 almost-identical copies of the
function in the linkonce section, and avoids having the multiple
implementations in C/asm, avoids the need for any logic derived from
CONFIG_INDIRECT_THUNK, and avoids the need for massive clobber lists.

~Andrew

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