[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] xen/arm: vgic: Use 'unsigned int' rather than 'int' whenever it is possible



On Thu, 17 Aug 2023, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Switch to unsigned int for the return/parameters of the following
> functions:
>     * REG_RANK_NR(): 'b' (number of bits) and the return is always positive.
>       'n' doesn't need to be size specific.
>     * vgic_rank_offset(): 'b' (number of bits), 'n' (register index),
>       's' (size of the access) are always positive.
>     * vgic_{enable, disable}_irqs(): 'n' (rank index) is always positive
>     * vgic_get_virq_type(): 'n' (rank index) and 'index' (register
>       index) are always positive.
> 
> Take the opportunity to propogate the unsignedness to the local
> variable used for the arguments.
> 
> This will remove some of the warning reported by GCC 12.2.1 when
> passing the flags -Wsign-conversion/-Wconversion.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  xen/arch/arm/include/asm/vgic.h | 11 +++++++----
>  xen/arch/arm/vgic-v2.c          | 12 ++++++++++--
>  xen/arch/arm/vgic.c             | 21 ++++++++++++---------
>  3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 6901a05c0669..922779ce146a 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -252,7 +252,7 @@ struct vgic_ops {
>   * Rank containing GICD_<FOO><n> for GICD_<FOO> with
>   * <b>-bits-per-interrupt
>   */
> -static inline int REG_RANK_NR(int b, uint32_t n)
> +static inline unsigned int REG_RANK_NR(unsigned int b, unsigned int n)
>  {
>      switch ( b )
>      {
> @@ -297,10 +297,13 @@ extern void gic_remove_from_lr_pending(struct vcpu *v, 
> struct pending_irq *p);
>  extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
>  extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
>  extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int 
> irq);
> -extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, 
> int s);
> +extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v,
> +                                              unsigned int b,
> +                                              unsigned int n,
> +                                              unsigned int s);
>  extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
> -extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
> -extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
> +extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
> +extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
>  extern void vgic_set_irqs_pending(struct vcpu *v, uint32_t r,
>                                    unsigned int rank);
>  extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 2a2eda2e6f4c..0aa10fff0f10 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -161,7 +161,11 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct vgic_irq_rank *rank;
> -    int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> +    /*
> +     * gpa/dbase are paddr_t which size may be higher than 32-bit. Yet
> +     * the difference will always be smaller than 32-bit.
> +     */
> +    unsigned int gicd_reg = info->gpa - v->domain->arch.vgic.dbase;
>      unsigned long flags;
>  
>      perfc_incr(vgicd_reads);
> @@ -403,7 +407,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct vgic_irq_rank *rank;
> -    int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> +    /*
> +     * gpa/dbase are paddr_t which size may be higher than 32-bit. Yet
> +     * the difference will always be smaller than 32-bit.
> +     */
> +    unsigned int gicd_reg = info->gpa - v->domain->arch.vgic.dbase;
>      uint32_t tr;
>      unsigned long flags;
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index afcac791fe4b..269b804974e0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -24,7 +24,8 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> -static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)
> +static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> +                                                  unsigned int rank)
>  {
>      if ( rank == 0 )
>          return v->arch.vgic.private_irqs;
> @@ -38,17 +39,17 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct 
> vcpu *v, int rank)
>   * Returns rank corresponding to a GICD_<FOO><n> register for
>   * GICD_<FOO> with <b>-bits-per-interrupt.
>   */
> -struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
> -                                              int s)
> +struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, unsigned int b,
> +                                       unsigned int n, unsigned int s)
>  {
> -    int rank = REG_RANK_NR(b, (n >> s));
> +    unsigned int rank = REG_RANK_NR(b, (n >> s));
>  
>      return vgic_get_rank(v, rank);
>  }
>  
>  struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>  {
> -    int rank = irq/32;
> +    unsigned int rank = irq / 32;
>  
>      return vgic_get_rank(v, rank);
>  }
> @@ -324,14 +325,14 @@ void arch_move_irqs(struct vcpu *v)
>      }
>  }
>  
> -void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> +void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
>  {
>      const unsigned long mask = r;
>      struct pending_irq *p;
>      struct irq_desc *desc;
>      unsigned int irq;
>      unsigned long flags;
> -    int i = 0;
> +    unsigned int i = 0;
>      struct vcpu *v_target;
>  
>      /* LPIs will never be disabled via this function. */
> @@ -361,7 +362,9 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  #define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1))
>  
>  /* The function should be called with the rank lock taken */
> -static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int 
> index)
> +static inline unsigned int vgic_get_virq_type(struct vcpu *v,
> +                                              unsigned int n,
> +                                              unsigned int index)
>  {
>      struct vgic_irq_rank *r = vgic_get_rank(v, n);
>      uint32_t tr = r->icfg[index >> 4];
> @@ -374,7 +377,7 @@ static inline unsigned int vgic_get_virq_type(struct vcpu 
> *v, int n, int index)
>          return IRQ_TYPE_LEVEL_HIGH;
>  }
>  
> -void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> +void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
>  {
>      const unsigned long mask = r;
>      struct pending_irq *p;

It looks like you could also switch 'i' to unsigned int in
vgic_enable_irqs like you did in vgic_disable_irqs

You could do that on commit:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.