Re: [Xen-devel] [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper

[Julien Grall <julien.grall@xxxxxxx>]

On 11/23/18 4:52 PM, Andrew Cooper wrote:
> ... rather than a macro which writes to its parameter by name.  A consequence
> of this change is that the local variables in local_*_is_enabled() can be
> dropped.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>   xen/arch/x86/io_apic.c             |  2 +-
>   xen/include/asm-arm/arm32/system.h | 30 +++++++++++++-----------------
>   xen/include/asm-arm/arm64/system.h | 32 ++++++++++++--------------------
>   xen/include/asm-arm/system.h       |  6 +-----
>   xen/include/asm-x86/system.h       | 22 +++++++++++-----------
>   5 files changed, 38 insertions(+), 54 deletions(-)
>
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index daa5e9e..aca4f63 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1486,7 +1486,7 @@ static int __init timer_irq_works(void)
>   
>       t1 = ACCESS_ONCE(pit0_ticks);
>   
> -    local_save_flags(flags);
> +    flags = local_save_flags();
>       local_irq_enable();
>       /* Let ten ticks pass... */
>       mdelay((10 * 1000) / HZ);
> diff --git a/xen/include/asm-arm/arm32/system.h 
> b/xen/include/asm-arm/arm32/system.h
> index 58c8fb3..cbfa91d 100644
> --- a/xen/include/asm-arm/arm32/system.h
> +++ b/xen/include/asm-arm/arm32/system.h
> @@ -7,15 +7,19 @@
>   #define local_irq_disable() asm volatile ( "cpsid i @ local_irq_disable\n" : : : 
> "cc" )
>   #define local_irq_enable()  asm volatile ( "cpsie i @ local_irq_enable\n" : : : 
> "cc" )
>   
> -#define local_save_flags(x)                                      \
> -({                                                               \
> -    BUILD_BUG_ON(sizeof(x) != sizeof(long));                     \
> -    asm volatile ( "mrs %0, cpsr     @ local_save_flags\n"       \
> -                  : "=r" (x) :: "memory", "cc" );                \
> -})
> +static inline unsigned long local_save_flags(void)
> +{
> +    unsigned long flags;
> +
> +    asm volatile ( "mrs %0, cpsr     @ local_SAVE_flags\n"
Any reason for writing "save" in uppercase?

> +                   : "=r" (flags) :: "memory", "cc" );
> +
> +    return flags;
> +}
> +
[...]


The rest of the code looks good to me.

Cheers,

--
Julien Grall

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