Re: [PATCH v8 01/27] ARM: GICv3: propagate number of host LPIs to GICv3 guest

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

Hi Andre,

On 12/04/17 01:44, Andre Przywara wrote:
> The host supports a certain number of LPI identifiers, as stored in
> the GICD_TYPER register.
> Store this number from the hardware register in vgic_v3_hw to allow
> injecting the very same number into a guest (Dom0).

DOM0 number should be derived from the limitation provided by the user 
(i.e lpi_data.max_host_lpi_ids) and not directly from the hardware.

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-v3.c        | 6 +++++-
>  xen/arch/arm/vgic-v3.c       | 7 ++++++-
>  xen/include/asm-arm/domain.h | 1 +
>  xen/include/asm-arm/vgic.h   | 3 ++-
>  4 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a559e5e..29c8964 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1579,6 +1579,7 @@ static int __init gicv3_init(void)
>  {
>      int res, i;
>      uint32_t reg;
> +    unsigned int intid_bits;
>
>      if ( !cpu_has_gicv3 )
>      {
> @@ -1622,8 +1623,11 @@ static int __init gicv3_init(void)
>                 i, r->base, r->base + r->size);
>      }
>
> +    reg = readl_relaxed(GICD + GICD_TYPER);
> +    intid_bits = GICD_TYPE_ID_BITS(reg);
> +
>      vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> -                     gicv3.rdist_stride);
> +                     gicv3.rdist_stride, intid_bits);

This only works for DOM0, I would expect the number of the guest to come 
up from the toolstack. You may want to have a look to 
xen_arch_domain_config. This is not necessary for this version (*hint* 
this should be listed in the list of TODO in the cover letter *hint*).

>      gicv3_init_v2();
>
>      spin_lock_init(&gicv3.lock);
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d10757a..ebcca22 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -57,18 +57,21 @@ static struct {
>      unsigned int nr_rdist_regions;
>      const struct rdist_region *regions;
>      uint32_t rdist_stride; /* Re-distributor stride */
> +    unsigned int intid_bits;  /* Number of interrupt ID bits */
>  } vgic_v3_hw;
>
>  void vgic_v3_setup_hw(paddr_t dbase,
>                        unsigned int nr_rdist_regions,
>                        const struct rdist_region *regions,
> -                      uint32_t rdist_stride)
> +                      uint32_t rdist_stride,
> +                      unsigned int intid_bits)
>  {
>      vgic_v3_hw.enabled = 1;
>      vgic_v3_hw.dbase = dbase;
>      vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
>      vgic_v3_hw.regions = regions;
>      vgic_v3_hw.rdist_stride = rdist_stride;
> +    vgic_v3_hw.intid_bits = intid_bits;
>  }
>
>  static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain *d, uint64_t irouter)
> @@ -1482,6 +1485,8 @@ static int vgic_v3_domain_init(struct domain *d)
>
>              first_cpu += size / d->arch.vgic.rdist_stride;
>          }
> +
> +        d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;

So intid_bits is based on what the hardware report, however Xen may not 
use ITS. So I would have expected this number to be derived from what is 
actually exposed to the domain. For DOM0 it would be the number of LPIs 
supported in Xen (e.g lpi_data.max_host_lpi_ids) if ITS is enabled.

Looking at the code (patch #12), you are only using intid_bits when ITS 
is enabled which is really surprising as this value should be correct 
even though LPIs are not used by DOM0.

>      }
>      else
>      {
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 6de8082..7c3829d 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -111,6 +111,7 @@ struct arch_domain
>          uint32_t rdist_stride;              /* Re-Distributor stride */
>          struct rb_root its_devices;         /* Devices mapped to an ITS */
>          spinlock_t its_devices_lock;        /* Protects the its_devices tree */
> +        unsigned int intid_bits;
>  #endif
>      } vgic;
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 544867a..df75064 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -346,7 +346,8 @@ struct rdist_region;
>  void vgic_v3_setup_hw(paddr_t dbase,
>                        unsigned int nr_rdist_regions,
>                        const struct rdist_region *regions,
> -                      uint32_t rdist_stride);
> +                      uint32_t rdist_stride,
> +                      unsigned int intid_bits);
>  #endif
>
>  #endif /* __ASM_ARM_VGIC_H__ */
>

Cheers,

-- 
Julien Grall

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