Re: [PATCH v2 11/36] irqchip/gic-v4.1: VPE table (aka GICR_VPROPBASER) allocation

[Marc Zyngier]

Hi Zenghui,

On 2019-12-24 07:10, Zenghui Yu wrote:
> Hi Marc,
>
> [ +Wudi and Nianyao. As they spotted the following issue but
>  I forgot to send it out. ]
>
> On 2019/10/27 22:42, Marc Zyngier wrote:
> > GICv4.1 defines a new VPE table that is potentially shared between
> > both the ITSs and the redistributors, following complicated affinity
> > rules.
> > To make things more confusing, the programming of this table at
> > the redistributor level is reusing the GICv4.0 GICR_VPROPBASER 
> > register
> > for something completely different.
> > The code flow is somewhat complexified by the need to respect the
> > affinities required by the HW, meaning that tables can either be
> > inherited from a previously discovered ITS or redistributor.
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---[...]
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> > b/drivers/irqchip/irq-gic-v3-its.c
> > index 40912b3fb0e1..478d3678850c 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> [...]
> > @@ -2025,6 +2098,214 @@ static int its_alloc_tables(struct its_node 
> > *its)
> >   	return 0;
> >   }
> >   +static u64 inherit_vpe_l1_table_from_its(void)
> > +{
> > +	struct its_node *its;
> > +	u64 val;
> > +	u32 aff;
> > +
> > +	val = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
> > +	aff = compute_common_aff(val);
> > +
> > +	list_for_each_entry(its, &its_nodes, entry) {
> > +		u64 baser;
> > +
> > +		if (!is_v4_1(its))
> > +			continue;
> > +
> > +		if (!FIELD_GET(GITS_TYPER_SVPET, its->typer))
> > +			continue;
> > +
> > +		if (aff != compute_its_aff(its))
> > +			continue;
> > +
> > +		/* GICv4.1 guarantees that the vPE table is GITS_BASER2 */
> > +		baser = its->tables[2].val;
> > +		if (!(baser & GITS_BASER_VALID))
> > +			continue;
> > +
> > +		/* We have a winner! */
> > +		val  = GICR_VPROPBASER_4_1_VALID;
> > +		if (baser & GITS_BASER_INDIRECT)
> > +			val |= GICR_VPROPBASER_4_1_INDIRECT;
> > +		val |= FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE,
> > +				  FIELD_GET(GITS_BASER_PAGE_SIZE_MASK, baser));
> > +		val |= FIELD_PREP(GICR_VPROPBASER_4_1_ADDR,
> > +				  GITS_BASER_ADDR_48_to_52(baser) >> 12);
>
> We've used GITS_BASER_ADDR_48_to_52() only in the KVM code where the
> pagesize of ITS table is fixed to 64K.
> It may not work when the pagesize is 4K or 16K?

You're absolutely right, we shouldn't mess with the 52bit macros when
PZ isn't set to 64k. I'm adding the following fix to this patch:

diff --git a/drivers/irqchip/irq-gic-v3-its.c 
b/drivers/irqchip/irq-gic-v3-its.c
index e1f8d5f9a0e3..3234bb9fbdbe 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2433,7 +2433,7 @@ static u64 inherit_vpe_l1_table_from_its(void)
 	aff = compute_common_aff(val);

 	list_for_each_entry(its, &its_nodes, entry) {
-		u64 baser;
+		u64 baser, addr;

 		if (!is_v4_1(its))
 			continue;
@@ -2455,8 +2455,15 @@ static u64 inherit_vpe_l1_table_from_its(void)
 			val |= GICR_VPROPBASER_4_1_INDIRECT;
 		val |= FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE,
 				  FIELD_GET(GITS_BASER_PAGE_SIZE_MASK, baser));
-		val |= FIELD_PREP(GICR_VPROPBASER_4_1_ADDR,
-				  GITS_BASER_ADDR_48_to_52(baser) >> 12);
+		switch (FIELD_GET(GITS_BASER_PAGE_SIZE_MASK, baser)) {
+		case GIC_PAGE_SIZE_64K:
+			addr = GITS_BASER_ADDR_48_to_52(baser);
+			break;
+		default:
+			addr = baser & GENMASK_ULL(47, 12);
+			break;
+		}
+		val |= FIELD_PREP(GICR_VPROPBASER_4_1_ADDR, addr >> 12);
 		val |= FIELD_PREP(GICR_VPROPBASER_SHAREABILITY_MASK,
 				  FIELD_GET(GITS_BASER_SHAREABILITY_MASK, baser));
 		val |= FIELD_PREP(GICR_VPROPBASER_INNER_CACHEABILITY_MASK,


Thanks again,

        M.
--
Jazz is not dead. It just smells funny...