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
