Re: [PATCH v1 09/14] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.04.2025 12:15, Oleksii Kurochko wrote:
> On 4/14/25 12:04 PM, Jan Beulich wrote:
>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>> +    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
>>> +    if ( !rc )
>>> +        panic("%s: IDC mode not supported\n", node->full_name);
>>> +
>>> +    imsic_node = dt_find_node_by_phandle(imsic_phandle);
>>> +    if ( !imsic_node )
>>> +        panic("%s: unable to find IMSIC node\n", node->full_name);
>>> +
>>> +    /* check imsic mode */
>>> +    rc = dt_property_read_u32_array(imsic_node, "interrupts-extended",
>>> +                                    irq_range, ARRAY_SIZE(irq_range));
>>> +    if ( rc && (rc != -EOVERFLOW) )
>>> +        panic("%s: unable to find interrupt-extended in %s node\n",
>>> +               node->full_name, imsic_node->full_name);
>> Why exactly is EOVERFLOW tolerable here?
> 
> QEMU generates two IMSIC device tree nodes: one for M-mode and one for S-mode.
> For the hypervisor, we don’t really care about the M-mode IMSIC node — we're 
> only
> interested in the S-mode IMSIC node.
> 
> The IMSIC node includes this information in the|"interrupts-extended"| 
> property,
> which has the following format:
>    interrupt-extended = {<interrupt-controller-phandle>, <machine_mode>},...
> The number of such|<phandle, mode>| pairs depends on the number of CPUs the 
> platform has.
> 
> For our purposes, to determine whether the IMSIC node corresponds to M-mode 
> or not, it’s sufficient to read only the first pair and check the mode like 
> this:
> 
>    if ( irq_range[1] == IRQ_M_EXT )
> 
> Thereby dt_property_read_u32_array() will return -EOVERFLOW in the case when 
> a platfrom
> has more then one CPU as we passed irq_range[2] as an argument but the amount 
> of values
> in "interrupts-extended" property will be (2 * CPUS_NUM).
> 
> I can update the comment above dt_property_read_u32_array() for more 
> clearness.

Yet my question remains: Why would it be okay to ignore the remaining entries,
and hence accept -EOVERFLOW as kind-of-success?

>>> +    aplic.regs = ioremap(paddr, size);
>>> +    if ( !aplic.regs )
>>> +        panic("%s: unable to map\n", node->full_name);
>>> +
>>> +    /* Setup initial state APLIC interrupts */
>>> +    aplic_init_hw_interrupts();
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct intc_hw_operations __ro_after_init aplic_ops = {
>> const or __ro_after_init?
> 
> What’s wrong with using both?|const| ensures the variable can't be changed at 
> compile time,
> while|__ro_after_init| makes it read-only at runtime after initialization is 
> complete.

No, const makes it read-only at compile- and run-time. __ro_after_init,
putting the item into a special section, makes it writable at init-time.
Due to the const, the compiler wouldn't emit any writes. But we can
also avoid stray writes by having the item live in .rodata.

> Probably,|__initconst| would be a better fit:
>    static const struct intc_hw_operations __initconst aplic_ops = {
> 
> Or even|__initconstrel|, since the|struct intc_hw_operations| contains 
> pointers.

Well, if this variable isn't accessed post-init, sure. That seems pretty
unlikely though, considering it contains pointers to hook functions.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/aplic.h
>>> @@ -0,0 +1,77 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +
>>> +/*
>>> + * xen/arch/riscv/aplic.h
>>> + *
>>> + * RISC-V Advanced Platform-Level Interrupt Controller support
>>> + *
>>> + * Copyright (c) 2023 Microchip.
>>> + */
>>> +
>>> +#ifndef ASM__RISCV__APLIC_H
>>> +#define ASM__RISCV__APLIC_H
>>> +
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/imsic.h>
>>> +
>>> +#define APLIC_DOMAINCFG_IE      BIT(8, UL)
>>> +#define APLIC_DOMAINCFG_DM      BIT(2, UL)
>>> +
>>> +struct aplic_regs {
>>> +    uint32_t domaincfg;
>>> +    uint32_t sourcecfg[1023];
>>> +    uint8_t _reserved1[0xBC0];
>>> +
>>> +    uint32_t mmsiaddrcfg;
>>> +    uint32_t mmsiaddrcfgh;
>>> +    uint32_t smsiaddrcfg;
>>> +    uint32_t smsiaddrcfgh;
>>> +    uint8_t _reserved2[0x30];
>>> +
>>> +    uint32_t setip[32];
>>> +    uint8_t _reserved3[92];
>>> +
>>> +    uint32_t setipnum;
>>> +    uint8_t _reserved4[0x20];
>>> +
>>> +    uint32_t in_clrip[32];
>>> +    uint8_t _reserved5[92];
>>> +
>>> +    uint32_t clripnum;
>>> +    uint8_t _reserved6[32];
>>> +
>>> +    uint32_t setie[32];
>>> +    uint8_t _reserved7[92];
>>> +
>>> +    uint32_t setienum;
>>> +    uint8_t _reserved8[32];
>>> +
>>> +    uint32_t clrie[32];
>>> +    uint8_t _reserved9[92];
>>> +
>>> +    uint32_t clrienum;
>>> +    uint8_t _reserved10[32];
>>> +
>>> +    uint32_t setipnum_le;
>>> +    uint32_t setipnum_be;
>>> +    uint8_t _reserved11[4088];
>>> +
>>> +    uint32_t genmsi;
>>> +    uint32_t target[1023];
>>> +};
>>> +
>>> +struct aplic_priv {
>>> +    /* base physical address and size */
>>> +    paddr_t paddr_start;
>>> +    paddr_t paddr_end;
>>> +    size_t  size;
>>> +
>>> +    /* registers */
>>> +    volatile struct aplic_regs *regs;
>>> +
>>> +    /* imsic configuration */
>>> +    const struct imsic_config *imsic_cfg;
>>> +};
>>> +
>>> +#endif /* ASM__RISCV__APLIC_H */
>> Does all of this really need to live in a non-private header?
> 
> struct aplic_priv is used in different files:
> - in aplic.c to define `aplic` variable.
> - in vaplic.c (which isn't intoduced yet) is used in several 
> places:https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/vaplic.c#L41

Which would still call for a private header (xen/arch/riscv/aplic.h).

> struct aplic_regs is used only in aplic.c (at least, at the moment) so could 
> be moved to
> aplic.c, but I don't see too much sense.

It is generally good practice to limit the scope of things as much as
possible. Just to avoid (or make more noticeable) mis-uses or layering
violations, for example.

>>> --- a/xen/arch/riscv/include/asm/irq.h
>>> +++ b/xen/arch/riscv/include/asm/irq.h
>>> @@ -27,7 +27,6 @@
>>>   #define IRQ_TYPE_INVALID        DT_IRQ_TYPE_INVALID
>>>   
>>>   /* TODO */
>>> -#define nr_irqs 0U
>> How come this is simply no longer needed, i.e. without any replacement?
>> Hmm, looks like the only use in common code has gone away. Yet then this
>> still doesn't really look to belong here (especially if not mentioned in
>> the description).
> 
> I missed that it is used in xen/common/domain.c when CONFIG_HAS_PIRQ=y, but 
> this
> config isn't selected for RISC-V.
> I think that I have to revert this change.

I don't think you need to, as long as you don't mean to select HAS_PIRQ
for RISC-V. It's just that the change looks entirely unrelated _here_.

Jan