Re: [PATCH 02/12 v3] xen/arm: vpl011: Define generic vreg_reg* access functions in vreg.h

[Julien Grall <julien.grall@arm.com>]

Hi Bhupinder,

On 10/05/17 15:24, Bhupinder Thakur wrote:
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index c838298..75c716e 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -172,18 +172,6 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>      }
>  }
>
> -/*
> - * 64 bits registers are only supported on platform with 64-bit long.
> - * This is also allow us to optimize the 32 bit case by using
> - * unsigned long rather than uint64_t
> - */
> -#if BITS_PER_LONG == 64
> -VGIC_REG_HELPERS(64, 0x7);
> -#endif
> -VGIC_REG_HELPERS(32, 0x3);
> -
> -#undef VGIC_REG_HELPERS
> -

Why this is moved out here? And not in patch #1?

>  enum gic_sgi_mode;
>
>  /*
> diff --git a/xen/include/asm-arm/vreg.h b/xen/include/asm-arm/vreg.h
> index 1442c58..e127114 100644
> --- a/xen/include/asm-arm/vreg.h
> +++ b/xen/include/asm-arm/vreg.h

[...]

> -{                                                                       \
> -    unsigned long tmp = *reg;                                           \
> -                                                                        \
> -    vgic_reg_clearbits(&tmp, bits, info->gpa & offmask,                 \
> -                       info->dabt.size);                                \
> -                                                                        \
> -    *reg = tmp;                                                         \
> +#define DEFINE_VREG_REG_HELPERS(sz, offmask)                                    \

Nack. There are no reason to rename the name of the macro more than 
s/vgic/vreg/. It also makes to patch diff more difficult to read.

Cheers,

-- 
Julien Grall

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