Re: [Xen-devel] [PATCH v10 02/32] ARM: GICv3: setup number of LPI bits for a GICv3 guest

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 30/05/17 11:54, Julien Grall wrote:
> Hi Andre,
> 
> On 26/05/17 18:35, 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).
>> DomUs get the legacy number of 10 bits here, since for now it only sees
>> SPIs, so it does not need more. This should be revisited once we get
>> proper DomU ITS support.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/gic-v3.c        |  6 +++++-
>>  xen/arch/arm/vgic-v3.c       | 10 +++++++++-
>>  xen/include/asm-arm/domain.h |  1 +
>>  xen/include/asm-arm/vgic.h   |  3 ++-
>>  4 files changed, 17 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);
>>      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..87f5fb3 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;
>>      }
>>      else
>>      {
>> @@ -1497,6 +1502,9 @@ static int vgic_v3_domain_init(struct domain *d)
>>          d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
>>          d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
>>          d->arch.vgic.rdist_regions[0].first_cpu = 0;
>> +
>> +        /* TODO: only SPIs for now, adjust this when guests need LPIs */
>> +        d->arch.vgic.intid_bits = 10;
> 
> What if the guest support no SPIs? Should not you based this number of
> the number of SPIs supported by the guest?

No, I think this is a misconception in the existing code. The number of
ID bits supported is actually related to the stream protocol, something
which we don't model here. It is independent from the number of actually
implemented SPIs, which is held in the lower 5 bits of GICD_TYPER.
See chapter 2.2 "INTIDs" in the GICv3 spec, where it says:
"If LPIs are not supported, the ID space in the Distributor is limited
to 10 bits."

Cheers,
Andre.

> 
>>      }
>>
>>      ret = vgic_v3_its_init_domain(d);
>> 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,
> 

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