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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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