Re: [PATCH v2 20/26] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h.

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/3/26 5:36 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/aplic.h
> > +++ b/xen/arch/riscv/include/asm/aplic.h
> > @@ -15,6 +15,11 @@
> >   
> >   #include <asm/imsic.h>
> >   
> > +#define APLIC_REG_OFFSET_MASK   0x3fff
> This I can see this as wanting to live separately. Yet ...
>
> > +#define APLIC_TARGET_IPRIO_MASK 0xff
This could be dropped as I don't use this mask anymore.

> > +#define APLIC_TARGET_GUEST_IDX_SHIFT 12
> > +#define APLIC_TARGET_EIID_MASK  0x7ff
> ... what (set of) register(s) do these apply to? Perhaps ...
>
> > @@ -26,6 +31,36 @@
> >   #define APLIC_SOURCECFG_SM_LEVEL_HIGH   0x6
> >   #define APLIC_SOURCECFG_SM_LEVEL_LOW    0x7
> >   
> > +#define APLIC_DOMAINCFG         0x0000
> > +#define APLIC_SOURCECFG_BASE    0x0004
> > +#define APLIC_SOURCECFG_LAST    0x0ffc
> > +
> > +#define APLIC_SMSICFGADDR       0x1bc8
> > +#define APLIC_SMSICFGADDRH      0x1bcc
> > +
> > +#define APLIC_SETIP_BASE        0x1c00
> > +#define APLIC_SETIP_LAST        0x1c7c
> > +#define APLIC_SETIPNUM          0x1cdc
> > +
> > +#define APLIC_CLRIP_BASE        0x1d00
> > +#define APLIC_CLRIP_LAST        0x1d7c
> > +#define APLIC_CLRIPNUM          0x1ddc
> > +
> > +#define APLIC_SETIE_BASE        0x1e00
> > +#define APLIC_SETIE_LAST        0x1e7c
> > +#define APLIC_SETIENUM          0x1edc
> > +
> > +#define APLIC_CLRIE_BASE        0x1f00
> > +#define APLIC_CLRIE_LAST        0x1f7c
> > +#define APLIC_CLRIENUM          0x1fdc
> > +
> > +#define APLIC_SETIPNUM_LE       0x2000
> > +
> > +#define APLIC_GENMSI            0x3000
> > +
> > +#define APLIC_TARGET_BASE       0x3004
> > +#define APLIC_TARGET_LAST       0x3ffc
> ... these? And then is ...
Yes, it is applied to target register.

> >   #define APLIC_TARGET_HART_IDX_SHIFT 18
> ... this also covering some part of them? Can't they (a) live together and (b)
> have some kind of connection to what they apply to?
Agreed, they could live together. The intent was simply to keep all MMIO 
definitions in sequence, but I'm okay with grouping macros related to a 
specific register together.
> And then why is there again a mix of *_SHIFT and *_MASK?
This APLIC_TARGET_GUEST_IDX_SHIFT - could be dropped I've already 
reworked that in different branch and use *_MASK instead so I will 
re-apply that changes in this patch. But APLIC_TARGET_HART_IDX_SHIFT is 
used in to not open-code 12 in calculation of shift value for group_index:
static void cf_check aplic_set_irq_affinity(struct irq_desc *desc, const 
cpumask_t *mask)
{
...
    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT);

Thanks.

~ Oleksii