Re: [Xen-devel] [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
> With future changes, altinstruction_entry is going to become more complicated
> to use.  Furthermore, there are already ALTERNATIVE* macros which can be used
> to avoid opencoding the creation of replacement information.
> 
> For ASM_STAC, ASM_CLAC and CR4_PV32_RESTORE, this means the removal of all
> hardocded label numbers.  For the cr4_pv32 alternatives, this means hardcoding
> the extra space required in the original patch site, but the hardcoding will
> be removed by a later patch.
> 
> No change to any functionality, but the handling of nops inside the original
> patch sites are a bit different.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

In general I try to align the line breaks '\' of macros, but I don't
think that's used consistently across the code at all.

Again just one nit below.

> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 58f652d..bd3819a 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -557,23 +557,9 @@ handle_exception_saved:
>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>          jz    exception_with_ints_disabled
>  
> -.Lcr4_pv32_orig:
> -        jmp   .Lcr4_pv32_done
> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 
> 0xcc
> -        .pushsection .altinstr_replacement, "ax"
> -.Lcr4_pv32_alt:
> -        mov   VCPU_domain(%rbx),%rax
> -.Lcr4_pv32_alt_end:
> -        .section .altinstructions, "a"
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMEP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMAP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        .popsection
> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP

What's the point of using __stringify here, isn't it clearer to just
use "mov ..."?

Thanks, Roger.

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