Re: [PATCH 02/17] KVM: selftests: aarch64: add function for accessing GICv3 dist and redist registers

[Ricardo Koller <ricarkol@google.com>]

On Tue, Nov 23, 2021 at 03:06:08PM +0000, Marc Zyngier wrote:
> On Tue, 09 Nov 2021 02:38:51 +0000,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > Add a generic library function for reading and writing GICv3 distributor
> > and redistributor registers. Then adapt some functions to use it; more
> > will come and use it in the next commit.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../selftests/kvm/lib/aarch64/gic_v3.c        | 124 ++++++++++++++----
> >  1 file changed, 101 insertions(+), 23 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > index 2dbf3339b62e..00e944fd8148 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > @@ -19,7 +19,8 @@ struct gicv3_data {
> >  	unsigned int nr_spis;
> >  };
> >  
> > -#define sgi_base_from_redist(redist_base) (redist_base + SZ_64K)
> > +#define sgi_base_from_redist(redist_base) 	(redist_base + SZ_64K)
> > +#define DIST_BIT				(1U << 31)
> >  
> >  enum gicv3_intid_range {
> >  	SGI_RANGE,
> > @@ -50,6 +51,14 @@ static void gicv3_gicr_wait_for_rwp(void *redist_base)
> >  	}
> >  }
> >  
> > +static void gicv3_wait_for_rwp(uint32_t cpu_or_dist)
> > +{
> > +	if (cpu_or_dist & DIST_BIT)
> > +		gicv3_gicd_wait_for_rwp();
> > +	else
> > +		gicv3_gicr_wait_for_rwp(gicv3_data.redist_base[cpu_or_dist]);
> > +}
> > +
> >  static enum gicv3_intid_range get_intid_range(unsigned int intid)
> >  {
> >  	switch (intid) {
> > @@ -81,39 +90,108 @@ static void gicv3_write_eoir(uint32_t irq)
> >  	isb();
> >  }
> >  
> > -static void
> > -gicv3_config_irq(unsigned int intid, unsigned int offset)
> > +uint32_t gicv3_reg_readl(uint32_t cpu_or_dist, uint64_t offset)
> > +{
> > +	void *base = cpu_or_dist & DIST_BIT ? gicv3_data.dist_base
> > +		: sgi_base_from_redist(gicv3_data.redist_base[cpu_or_dist]);
> > +	return readl(base + offset);
> > +}
> > +
> > +void gicv3_reg_writel(uint32_t cpu_or_dist, uint64_t offset, uint32_t reg_val)
> > +{
> > +	void *base = cpu_or_dist & DIST_BIT ? gicv3_data.dist_base
> > +		: sgi_base_from_redist(gicv3_data.redist_base[cpu_or_dist]);
> > +	writel(reg_val, base + offset);
> > +}
> > +
> > +uint32_t gicv3_getl_fields(uint32_t cpu_or_dist, uint64_t offset, uint32_t mask)
> > +{
> > +	return gicv3_reg_readl(cpu_or_dist, offset) & mask;
> > +}
> > +
> > +void gicv3_setl_fields(uint32_t cpu_or_dist, uint64_t offset,
> > +		uint32_t mask, uint32_t reg_val)
> > +{
> > +	uint32_t tmp = gicv3_reg_readl(cpu_or_dist, offset) & ~mask;
> > +
> > +	tmp |= (reg_val & mask);
> > +	gicv3_reg_writel(cpu_or_dist, offset, tmp);
> > +}
> > +
> > +/*
> > + * We use a single offset for the distributor and redistributor maps as they
> > + * have the same value in both. The only exceptions are registers that only
> > + * exist in one and not the other, like GICR_WAKER that doesn't exist in the
> > + * distributor map. Such registers are conveniently marked as reserved in the
> > + * map that doesn't implement it; like GICR_WAKER's offset of 0x0014 being
> > + * marked as "Reserved" in the Distributor map.
> > + */
> > +static void gicv3_access_reg(uint32_t intid, uint64_t offset,
> > +		uint32_t reg_bits, uint32_t bits_per_field,
> > +		bool write, uint32_t *val)
> >  {
> >  	uint32_t cpu = guest_get_vcpuid();
> > -	uint32_t mask = 1 << (intid % 32);
> >  	enum gicv3_intid_range intid_range = get_intid_range(intid);
> > -	void *reg;
> > -
> > -	/* We care about 'cpu' only for SGIs or PPIs */
> > -	if (intid_range == SGI_RANGE || intid_range == PPI_RANGE) {
> > -		GUEST_ASSERT(cpu < gicv3_data.nr_cpus);
> > -
> > -		reg = sgi_base_from_redist(gicv3_data.redist_base[cpu]) +
> > -			offset;
> > -		writel(mask, reg);
> > -		gicv3_gicr_wait_for_rwp(gicv3_data.redist_base[cpu]);
> > -	} else if (intid_range == SPI_RANGE) {
> > -		reg = gicv3_data.dist_base + offset + (intid / 32) * 4;
> > -		writel(mask, reg);
> > -		gicv3_gicd_wait_for_rwp();
> > -	} else {
> > -		GUEST_ASSERT(0);
> > -	}
> > +	uint32_t fields_per_reg, index, mask, shift;
> > +	uint32_t cpu_or_dist;
> > +
> > +	GUEST_ASSERT(bits_per_field <= reg_bits);
> > +	GUEST_ASSERT(*val < (1U << bits_per_field));
> > +	/* Some registers like IROUTER are 64 bit long. Those are currently not
> > +	 * supported by readl nor writel, so just asserting here until then.
> > +	 */
> > +	GUEST_ASSERT(reg_bits == 32);
> 
> IROUTER *does* support 32bit accesses. There are no 64bit MMIO
> registers in the GIC architecture that do not support 32bit accesses,
> if only because there is no guarantee about the width of the MMIO bus
> itself (not to mention the existence of 32bit CPUs!).
> 
> See 12.1.3 ("GIC memory-mapped register access") in the GICv3 arch
> spec.

I see, thanks for the pointer. Will update the comment in v2. Although I
might keep the assert as this function doesn't support 64bit accesses
(yet).

Thanks,
Ricardo

> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm