Re: [PATCH 13/26] hw/intc/arm_gicv3_its: Use FIELD macros for CTEs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Use FIELD macros to handle CTEs, rather than ad-hoc mask-and-shift.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/gicv3_internal.h | 3 ++-
>  hw/intc/arm_gicv3_its.c  | 7 ++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> index 14e8ef68e02..1eeb99035da 100644
> --- a/hw/intc/gicv3_internal.h
> +++ b/hw/intc/gicv3_internal.h
> @@ -403,7 +403,8 @@ FIELD(DTE, ITTADDR, 6, 44)
>   * Valid = 1 bit, RDBase = 16 bits
>   */
>  #define GITS_CTE_SIZE                 (0x8ULL)
> -#define GITS_CTE_RDBASE_PROCNUM_MASK  MAKE_64BIT_MASK(1, 
> RDBASE_PROCNUM_LENGTH)
> +FIELD(CTE, VALID, 0, 1)
> +FIELD(CTE, RDBASE, 1, RDBASE_PROCNUM_LENGTH)
>  
>  /* Special interrupt IDs */
>  #define INTID_SECURE 1020
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index d6637229479..ab6ce09dbc2 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -104,7 +104,7 @@ static bool get_cte(GICv3ITSState *s, uint16_t icid, 
> uint64_t *cte,
>                                        MEMTXATTRS_UNSPECIFIED, res);
>      }
>  
> -    return (*cte & TABLE_ENTRY_VALID_MASK) != 0;
> +    return FIELD_EX64(*cte, CTE, VALID);
>  }
>  
>  static bool update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
> @@ -308,7 +308,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t 
> value, uint32_t offset,
>           * Current implementation only supports rdbase == procnum
>           * Hence rdbase physical address is ignored
>           */
> -        rdbase = (cte & GITS_CTE_RDBASE_PROCNUM_MASK) >> 1U;
> +        rdbase = FIELD_EX64(cte, CTE, RDBASE);
>  
>          if (rdbase >= s->gicv3->num_cpu) {
>              return result;
> @@ -426,7 +426,8 @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, 
> bool valid,
>  
>      if (valid) {
>          /* add mapping entry to collection table */
> -        cte = (valid & TABLE_ENTRY_VALID_MASK) | (rdbase << 1ULL);
> +        cte = FIELD_DP64(cte, CTE, VALID, 1);
> +        cte = FIELD_DP64(cte, CTE, RDBASE, rdbase);

I almost flagged this until I realised the double deposit are additive
and the same as the bare |

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée