Re: [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> "unsigned long long" is defined as 64 bits on AArch64 and AArch32
> Thus, one should this instead of "unsigned long" which is 32 bits
> on AArch32.
> 
> Also use 'PRIx64' instead of 'lx' or 'llx' to print uint64_t.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 
> Changed from :-
> v1 - 1. Replace PRIu64 with PRIx64 so that the values are printed in hex as
> desired.
> 2. Use ULL in GITS_BASER_RO_MASK as MMIO registers are always unsigned.
> 
> v2 - 1. Removed changes to ITS and LPI as they are not supported for AArch32.
> 
>  xen/arch/arm/gic-v3.c                  | 4 ++--
>  xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4722bb4daf..6457e7033c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
>      if ( v == current )
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
> +            printk("   HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
>      }
>      else
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
> +            printk("   VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
You changed these specifiers to be llx in patch no.7 so such a change (using 
PRIx64) should
be done in that patch. Generally there is no need for a patch to fix something 
that you
introduced earlier in the series. It should be done in one step. Having said 
that...

>      }
>  }
>  
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h 
> b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 743ebb20fd..227533868f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -195,7 +195,7 @@
>  
>  #define ICH_SGI_IRQMODE_SHIFT        40
>  #define ICH_SGI_IRQMODE_MASK         0x1
> -#define ICH_SGI_TARGET_OTHERS        1UL
> +#define ICH_SGI_TARGET_OTHERS        1ULL
>  #define ICH_SGI_TARGET_LIST          0
>  #define ICH_SGI_IRQ_SHIFT            24
>  #define ICH_SGI_IRQ_MASK             0xf

adding a patch for just this macro is not very useful and you could take the 
opportunity
to modify it in any of your patches.

~Michal