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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>> --- 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?

>>>   #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?

Jan