Re: [PATCH v3 11/14] xen/riscv: implementation of aplic and imsic operations

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/22/25 5:55 PM, Jan Beulich wrote:

> On 21.05.2025 18:03, Oleksii Kurochko wrote:
> > +static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t *mask)
> > +{
> > +    unsigned int cpu;
> > +    uint64_t group_index, base_ppn;
> > +    uint32_t hhxw, lhxw ,hhxs, value;
> Nit: Comma vs blank placement.
>
> > +    const struct imsic_config *imsic = aplic.imsic_cfg;
> > +
> > +    /*
> > +     * TODO: Currently, APLIC is supported only with MSI interrupts.
> > +     *       If APLIC without MSI interrupts is required in the future,
> > +     *       this function will need to be updated accordingly.
> > +     */
> > +    ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
> > +
> > +    ASSERT(!cpumask_empty(mask));
> > +
> > +    ASSERT(spin_is_locked(&desc->lock));
> > +
> > +    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
> > +    hhxw = imsic->group_index_bits;
> > +    lhxw = imsic->hart_index_bits;
> > +    hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2;
> > +    base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT;
> > +
> > +    /* Update hart and EEID in the target register */
> > +    group_index = (base_ppn >> (hhxs + IMSIC_MMIO_PAGE_SHIFT)) & (BIT(hhxw, UL) - 1);
> Nit: Line length.
>
> I'm also puzzled by the various uses of IMSIC_MMIO_PAGE_SHIFT. Why do you
> subtract double the value when calculating hhxs, just to add the value
> back in here? There's no other usee of the variable afaics.
>     
To follow AIA spec:
  MSI_address = ((Base_PPN | (group_index << (HHXS + 12)) | (hart_index << LHXS) | guest_index) << 12)
  Represented in the terms of Section 3.6, HHXW = j, LHXW = k, HHXS = E - 24, LHXS = D - 12, Base PPN = B >> 12.

Specifically, in this case it is possible to calculate as hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT,
and then drop "+ IMSIC_MMIO_PAGE_SHIFT" in (hhxs + IMSIC_MMIO_PAGE_SHIFT), but then it could be harder to
understand a formula when you look into AIA spec and then what is in code.

Also, possible one day hhxs will be used somewhere else, for example, to verify target base physical PPN as Linux
kernel does:
	tppn = msg_addr >> APLIC_xMSICFGADDR_PPN_SHIFT;

	/* Compute target HART Base PPN */
	tbppn = tppn;
	tbppn &= ~APLIC_xMSICFGADDR_PPN_HART(mc->lhxs);
	tbppn &= ~APLIC_xMSICFGADDR_PPN_LHX(mc->lhxw, mc->lhxs);
	tbppn &= ~APLIC_xMSICFGADDR_PPN_HHX(mc->hhxw, mc->hhxs);
	WARN_ON(tbppn != mc->base_ppn);

	/* Compute target group and hart indexes */
	group_index = (tppn >> APLIC_xMSICFGADDR_PPN_HHX_SHIFT(mc->hhxs)) &
		     APLIC_xMSICFGADDR_PPN_HHX_MASK(mc->hhxw);
        ...

At the moment, I can add the comment above hhxs (and/or group_index) that it is calculated in this way and
isn't simplified to "hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2" to follow a formula
from AIA spec.

~ Oleksii