Re: [PATCH 08/13] x86/paravirt: Clean up paravirt_types.h

[Borislav Petkov]

On Wed, Oct 04, 2017 at 10:58:29AM -0500, Josh Poimboeuf wrote:
> Make paravirt_types.h more understandable:
> 
> - Use more consistent and logical naming
> - Simplify interfaces
> - Put related macros together
> - Improve whitespace
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/paravirt_types.h | 104 ++++++++++++++++++----------------
>  1 file changed, 54 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 01f9e10983c1..5656aea79412 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h

...

> @@ -388,13 +361,46 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
>  
>  int paravirt_disable_iospace(void);
>  
> +
>  /*
> - * This generates an indirect call based on the operation type number.
> - * The type number, computed in PARAVIRT_PATCH, is derived from the
> - * offset into the paravirt_patch_template structure, and can therefore be
> - * freely converted back into a structure offset.
> + * Generate some code, and mark it as patchable by apply_paravirt().
>   */
> -#define PARAVIRT_CALL	"call *%c[paravirt_opptr];"
> +#define _PV_SITE(insn_string, type, clobber)				\
> +	"771:\n\t" insn_string "\n" "772:\n"				\

You can merge the last two strings into one.

Also, s/insn_string/insns/ so that it is the same as in paravirt-asm.h
PV_SITE definition. Ditto for s/clobber/clobbers/ I.e., in general,
have the same parameter names in the respective macros so that they can
be parsed visually quickly and reader can say, aha, I see, that's that
param from the asm version of the macro.

> +	".pushsection .parainstructions,\"a\"\n"			\
> +	_ASM_ALIGN "\n"							\
> +	_ASM_PTR " 771b\n"						\
> +	"  .byte " type "\n"						\
> +	"  .byte 772b-771b\n"						\
> +	"  .short " clobber "\n"					\
> +	".popsection\n"
> +
> +#define PARAVIRT_PATCH(x)						\

Btw, that macro's name doesn't tell me anything: it should be
PV_INSN_TYPE or so.

> +	(offsetof(struct paravirt_patch_template, x) / sizeof(void *))
> +
> +#define PV_STRINGIFY(constraint)	"%c[" __stringify(constraint) "]"
> +
> +#define PV_CALL_CONSTRAINT		pv_op_ptr
> +#define PV_TYPE_CONSTRAINT		pv_typenum
> +#define PV_CLBR_CONSTRAINT		pv_clobber
> +
> +#define PV_CALL_CONSTRAINT_STR		PV_STRINGIFY(PV_CALL_CONSTRAINT)
> +#define PV_TYPE_CONSTRAINT_STR		PV_STRINGIFY(PV_TYPE_CONSTRAINT)
> +#define PV_CLBR_CONSTRAINT_STR		PV_STRINGIFY(PV_CLBR_CONSTRAINT)
> +
> +#define PV_CALL_STR			"call *" PV_CALL_CONSTRAINT_STR ";"
> +
> +#define PV_INPUT_CONSTRAINTS(op, clobber)				\
> +	[PV_TYPE_CONSTRAINT] "i" (PARAVIRT_PATCH(op)),			\
> +	[PV_CALL_CONSTRAINT] "i" (&(op)),				\
> +	[PV_CLBR_CONSTRAINT] "i" (clobber)
> +
> +#define PV_SITE(insn_string)						\
> +	_PV_SITE(insn_string, PV_TYPE_CONSTRAINT_STR, PV_CLBR_CONSTRAINT_STR)
> +
> +#define PV_ALT_SITE(oldinstr, newinstr)					\
> +	_PV_ALT_SITE(oldinstr, newinstr, PV_TYPE_CONSTRAINT_STR,	\
> +		     PV_CLBR_CONSTRAINT_STR)
>  
>  /*
>   * These macros are intended to wrap calls through one of the paravirt
> @@ -525,25 +531,24 @@ int paravirt_disable_iospace(void);
>  
>  #define ____PVOP_CALL(rettype, op, clbr, call_clbr, extra_clbr,		\
>  		      pre, post, ...)					\
> -	({								\
> -		rettype __ret;						\
> -		PVOP_CALL_ARGS;						\
> -		PVOP_TEST_NULL(op);					\
> +({									\
> +	rettype __ret;							\
> +	PVOP_CALL_ARGS;							\
> +	PVOP_TEST_NULL(op);						\
>  		asm volatile(pre					\
> -			     paravirt_alt(PARAVIRT_CALL)		\
> +			     PV_SITE(PV_CALL_STR)			\
>  			     post					\
>  			     : call_clbr, ASM_CALL_CONSTRAINT		\
> -			     : paravirt_type(op),			\
> -			       paravirt_clobber(clbr),			\
> +			     : PV_INPUT_CONSTRAINTS(op, clbr),		\
>  			       ##__VA_ARGS__				\
>  			     : "memory", "cc" extra_clbr);		\
> -		if (IS_ENABLED(CONFIG_X86_32) &&			\
> -		    sizeof(rettype) > sizeof(unsigned long))		\
> -			__ret = (rettype)((((u64)__edx) << 32) | __eax);\
> -		else							\
> -			__ret = (rettype)(__eax & PVOP_RETMASK(rettype));\
> -		__ret;							\
> -	})

<---- newline here.

> +	if (IS_ENABLED(CONFIG_X86_32) &&				\
> +	    sizeof(rettype) > sizeof(unsigned long))			\
> +		__ret = (rettype)((((u64)__edx) << 32) | __eax);	\
> +	else								\
> +		__ret = (rettype)(__eax & PVOP_RETMASK(rettype));	\
> +	__ret;								\
> +})
-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.