Re: [PATCH v5 17/26] xen/riscv: introduce minimal virtual APLIC (vAPLIC) infrastructure

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/13/26 8:41 AM, Jan Beulich wrote:
> On 10.07.2026 17:52, Oleksii Kurochko wrote:
> > On 7/9/26 5:39 PM, Jan Beulich wrote:
> > > On 06.07.2026 17:57, Oleksii Kurochko wrote:
> > > > At the current development stage, only domain vINTC init and deinit
> > > > operations are required, so implement those first.
> > > >
> > > > Initialize vAPLIC's domaincfg to with the interrupt-enable bit set and
> > > > MSI delivery mode selected as the current solution is exepcted to have
> > > > always IMSIC, and initialize vintc->ops.
> > > How would domaincfg be initialized on real hardware?
> > Xen will initialize that in aplic_init_hw_interrupts():
> > writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
> I.e. it is very much something the OS should do.
>
> > I can see that maybe
> > > firmware would have to set DM suitably (and you may mean to take firmware's
> > > role here).
> > I don't think that firmware will do that (and OpenSBI for example
> > doesn't do that). If firmware can do that we for sure want to control in
> > Xen what is written to ->domaincfg.
> >
> > > But isn't setting at least IE entirely the OSes responsibility?
> > At least, Linux setups ->domaincfg once at the boot time:
> >
> >         /* Setup APLIC domaincfg register */
> >         val = readl(priv->regs + APLIC_DOMAINCFG);
> >         val |= APLIC_DOMAINCFG_IE;
> >         if (msi_mode)
> >                 val |= APLIC_DOMAINCFG_DM;
> >         writel(val, priv->regs + APLIC_DOMAINCFG);
> >         if (readl(priv->regs + APLIC_DOMAINCFG) != val)
> >                 dev_warn(priv->dev, "unable to write 0x%x in domaincfg\n", val);
> >
> > And don't touch this register anymore, even for interrupt disablement it
> > isn't used.
> >
> > So Xen can just does once:
> > writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
> >
> > and then just properly handle access of a guest to domaincfg.
> Xen can do this for itself, sure. But shouldn't domaincfg as seen by guests
> start out 0 then?
Now I think I understand your point.

I agree that it should start from 0 (or from 0x80000000, since bits 
31:24 are read-only and fixed to 0x80). All other bits should be set or 
cleared by the guest and written to vaplic->regs.domaincfg when the 
guest accesses the vAPLIC domaincfg register.
In that case, domain_vaplic_init() should initialize ->domaincfg as:
  vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO;

instead of:
  vaplic->regs.domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM |
                         APLIC_DOMAINCFG_RO;

> > > > --- a/xen/arch/riscv/include/asm/aplic.h
> > > > +++ b/xen/arch/riscv/include/asm/aplic.h
> > > > @@ -15,6 +15,8 @@
> > > >    
> > > >    #include <asm/imsic.h>
> > > >    
> > > > +/* domaincfg bits 31:24 are read-only 0x80 */
> > > > +#define APLIC_DOMAINCFG_RO      (0x80U << 24)
> > > Bit 7 is also documented as read-only 0. Wouldn't the comment better reflect
> > > that as well?
> > Not sure, bits 31:24 are read-only *0x80* but bit 7 is read-only *0*.
> And would it hurt if the comment said so, to avoid any ambiguity?
Of course, it won't I will update the comment.

> > > >    #define APLIC_DOMAINCFG_IE      BIT(8, U)
> > > >    #define APLIC_DOMAINCFG_DM      BIT(2, U)
> > > Wouldn't you better spell out BE as well?
> > I can add:
> >     #define APLIC_DOMAINCFG_BE       BIT(0, U)
> >
> > But it isn't used at the moment (Linux also defines it but never
> > actually using it). Do you want still to add that now?
> Imo it would be better to have a complete set of definitions. If you
> don't allow guests to set this bit, perhaps to emit a sufficiently
> informative debug log messages you may want to use the #define?
Agree, it make sense.

Thanks.

~ Oleksii