| Message ID | 1d023045be49ae93d41d552f9c9a79972fa4e84b.1744126720.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: introduce basic UART support and interrupts for hypervisor mode | expand |
On 08.04.2025 17:57, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/aplic.c > +++ b/xen/arch/riscv/aplic.c > @@ -9,19 +9,112 @@ > * Copyright (c) 2024-2025 Vates > */ > > +#include <xen/device_tree.h> > #include <xen/errno.h> > #include <xen/init.h> > #include <xen/irq.h> > +#include <xen/mm.h> > #include <xen/sections.h> > #include <xen/types.h> > +#include <xen/vmap.h> > > +#include <asm/aplic.h> > #include <asm/device.h> > +#include <asm/imsic.h> > #include <asm/intc.h> > +#include <asm/riscv_encoding.h> > + > +#define APLIC_DEFAULT_PRIORITY 1 > + > +static struct aplic_priv aplic; > > static struct intc_info __ro_after_init aplic_info = { > .hw_version = INTC_APLIC, > }; > > +static void __init aplic_init_hw_interrupts(void) > +{ > + int i; > + > + /* Disable all interrupts */ > + for ( i = 0; i <= aplic_info.nr_irqs; i += 32 ) > + aplic.regs->clrie[i] = -1U; > + > + /* Set interrupt type and default priority for all interrupts */ > + for ( i = 1; i <= aplic_info.nr_irqs; i++ ) > + { > + aplic.regs->sourcecfg[i - 1] = 0; > + aplic.regs->target[i - 1] = APLIC_DEFAULT_PRIORITY; A field named "target" is written with a priority value? > + } > + > + /* Clear APLIC domaincfg */ > + aplic.regs->domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM; The statement doesn't like like there was any "clearing" here. > +} > + > +static int __init aplic_init(void) > +{ > + int rc; > + dt_phandle imsic_phandle; > + uint32_t irq_range[2]; > + const __be32 *prop; > + uint64_t size, paddr; > + struct dt_device_node *imsic_node; > + const struct dt_device_node *node = aplic_info.node; > + > + /* check for associated imsic node */ Nit: Comment style (also elsewhere). > + 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? > + if ( irq_range[1] == IRQ_M_EXT ) > + /* machine mode imsic node, ignore this aplic node */ > + return 0; > + > + rc = imsic_init(imsic_node); > + if ( rc ) > + panic("%s: Failded to initialize IMSIC\n", node->full_name); > + > + /* Find out number of interrupt sources */ > + rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.nr_irqs); > + if ( !rc ) > + panic("%s: failed to get number of interrupt sources\n", > + node->full_name); > + > + prop = dt_get_property(node, "reg", NULL); > + dt_get_range(&prop, node, &paddr, &size); > + if ( !paddr ) > + panic("%s: first MMIO resource not found\n", node->full_name); > + > + aplic.paddr_start = paddr; > + aplic.paddr_end = paddr + size; > + aplic.size = size; Why do all three need recording? Isn't a (start,size) tuple sufficient (and unambiguous)? > + 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? > --- /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? > --- 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). Jan
On 4/14/25 12:04 PM, Jan Beulich wrote: > On 08.04.2025 17:57, Oleksii Kurochko wrote: >> --- a/xen/arch/riscv/aplic.c >> +++ b/xen/arch/riscv/aplic.c >> @@ -9,19 +9,112 @@ >> * Copyright (c) 2024-2025 Vates >> */ >> >> +#include <xen/device_tree.h> >> #include <xen/errno.h> >> #include <xen/init.h> >> #include <xen/irq.h> >> +#include <xen/mm.h> >> #include <xen/sections.h> >> #include <xen/types.h> >> +#include <xen/vmap.h> >> >> +#include <asm/aplic.h> >> #include <asm/device.h> >> +#include <asm/imsic.h> >> #include <asm/intc.h> >> +#include <asm/riscv_encoding.h> >> + >> +#define APLIC_DEFAULT_PRIORITY 1 >> + >> +static struct aplic_priv aplic; >> >> static struct intc_info __ro_after_init aplic_info = { >> .hw_version = INTC_APLIC, >> }; >> >> +static void __init aplic_init_hw_interrupts(void) >> +{ >> + int i; >> + >> + /* Disable all interrupts */ >> + for ( i = 0; i <= aplic_info.nr_irqs; i += 32 ) >> + aplic.regs->clrie[i] = -1U; >> + >> + /* Set interrupt type and default priority for all interrupts */ >> + for ( i = 1; i <= aplic_info.nr_irqs; i++ ) >> + { >> + aplic.regs->sourcecfg[i - 1] = 0; >> + aplic.regs->target[i - 1] = APLIC_DEFAULT_PRIORITY; > A field named "target" is written with a priority value? Low bits of target register contains Interrupt Priority bits which can't be zero according to AIA spec: ``` 4.5.16.1. Active source, direct delivery mode For an active interrupt source , if the domain is configured in direct delivery mode (domaincfg.DM = 0), then register target[ ] has this format: bits 31:18 Hart Index (WLRL) bits 7:0 IPRIO (WARL) All other register bits are reserved and read as zeros. Hart Index is a WLRL field that specifies the hart to which interrupts from this source will be delivered. Field IPRIO (Interrupt Priority) specifies the priority number for the interrupt source. This field is a WARL unsigned integer of IPRIOLEN bits, where IPRIOLEN is a constant parameter for the given APLIC, in the range of 1 to 8. Only values 1 through are allowed for IPRIO, not zero. A write to a target register sets IPRIO equal to bits :0 of the 32-bit value written, unless those bits are all zeros, in which case the priority number is set to 1 instead. (If IPRIOLEN = 1, these rules cause IPRIO to be effectively read-only with value 1.) ``` > >> + } >> + >> + /* Clear APLIC domaincfg */ >> + aplic.regs->domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM; > The statement doesn't like like there was any "clearing" here. But all other bits, except|APLIC_DOMAINCFG_{IE, DM}|, are set to zero. I think we can remove this comment to avoid confusion. >> + 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. >> + if ( irq_range[1] == IRQ_M_EXT ) >> + /* machine mode imsic node, ignore this aplic node */ >> + return 0; >> + >> + rc = imsic_init(imsic_node); >> + if ( rc ) >> + panic("%s: Failded to initialize IMSIC\n", node->full_name); >> + >> + /* Find out number of interrupt sources */ >> + rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.nr_irqs); >> + if ( !rc ) >> + panic("%s: failed to get number of interrupt sources\n", >> + node->full_name); >> + >> + prop = dt_get_property(node, "reg", NULL); >> + dt_get_range(&prop, node, &paddr, &size); >> + if ( !paddr ) >> + panic("%s: first MMIO resource not found\n", node->full_name); >> + >> + aplic.paddr_start = paddr; >> + aplic.paddr_end = paddr + size; >> + aplic.size = size; > Why do all three need recording? Isn't a (start,size) tuple sufficient > (and unambiguous)? (start,size) will be enough. I'll drop aplic.paddr_end. > >> + 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. 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. > >> --- /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 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. > >> --- 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. ~ Oleksii
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
On 4/16/25 12:30 PM, Jan Beulich wrote: > 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? Because for other entries the IMSIC mode will be the same and the difference will be only in interrupt controller's phandle which we don't care about in this function and cares only about in imisic_init(), look at usage of imsic_get_parent_hartid(). > >>>> + 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. Oh, right, `const` will add the variable to .rodata. Then I think it is enough to have `const` as aplic_ops is going to be initialized once and then only read will happen. > >> 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. Sure, .init section is going to be freed after init-time. ~ Oleksii
On 17.04.2025 17:21, Oleksii Kurochko wrote: > > On 4/16/25 12:30 PM, Jan Beulich wrote: >> 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? > > Because for other entries the IMSIC mode will be the same and the difference will be only in > interrupt controller's phandle And we can blindly take this for granted? Would you mind extending the comment that's there to include this aspect? Jan > which we don't care about in this function and cares only about > in imisic_init(), look at usage of imsic_get_parent_hartid().
On 4/17/25 5:30 PM, Jan Beulich wrote: > On 17.04.2025 17:21, Oleksii Kurochko wrote: >> On 4/16/25 12:30 PM, Jan Beulich wrote: >>> 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? >> Because for other entries the IMSIC mode will be the same and the difference will be only in >> interrupt controller's phandle > And we can blindly take this for granted? Would you mind extending the > comment that's there to include this aspect? I tried to compile dtc with different modes in interrupt-extends and compilation doesn't failed, so it's not really granted. Just to be sure, I'll check all items of interrupts-extend not just the first one. ~ Oleksii >> which we don't care about in this function and cares only about >> in imisic_init(), look at usage of imsic_get_parent_hartid().
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c index 6dc040af6f..d1aa835c3e 100644 --- a/xen/arch/riscv/aplic.c +++ b/xen/arch/riscv/aplic.c @@ -9,19 +9,112 @@ * Copyright (c) 2024-2025 Vates */ +#include <xen/device_tree.h> #include <xen/errno.h> #include <xen/init.h> #include <xen/irq.h> +#include <xen/mm.h> #include <xen/sections.h> #include <xen/types.h> +#include <xen/vmap.h> +#include <asm/aplic.h> #include <asm/device.h> +#include <asm/imsic.h> #include <asm/intc.h> +#include <asm/riscv_encoding.h> + +#define APLIC_DEFAULT_PRIORITY 1 + +static struct aplic_priv aplic; static struct intc_info __ro_after_init aplic_info = { .hw_version = INTC_APLIC, }; +static void __init aplic_init_hw_interrupts(void) +{ + int i; + + /* Disable all interrupts */ + for ( i = 0; i <= aplic_info.nr_irqs; i += 32 ) + aplic.regs->clrie[i] = -1U; + + /* Set interrupt type and default priority for all interrupts */ + for ( i = 1; i <= aplic_info.nr_irqs; i++ ) + { + aplic.regs->sourcecfg[i - 1] = 0; + aplic.regs->target[i - 1] = APLIC_DEFAULT_PRIORITY; + } + + /* Clear APLIC domaincfg */ + aplic.regs->domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM; +} + +static int __init aplic_init(void) +{ + int rc; + dt_phandle imsic_phandle; + uint32_t irq_range[2]; + const __be32 *prop; + uint64_t size, paddr; + struct dt_device_node *imsic_node; + const struct dt_device_node *node = aplic_info.node; + + /* check for associated imsic node */ + 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); + + if ( irq_range[1] == IRQ_M_EXT ) + /* machine mode imsic node, ignore this aplic node */ + return 0; + + rc = imsic_init(imsic_node); + if ( rc ) + panic("%s: Failded to initialize IMSIC\n", node->full_name); + + /* Find out number of interrupt sources */ + rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.nr_irqs); + if ( !rc ) + panic("%s: failed to get number of interrupt sources\n", + node->full_name); + + prop = dt_get_property(node, "reg", NULL); + dt_get_range(&prop, node, &paddr, &size); + if ( !paddr ) + panic("%s: first MMIO resource not found\n", node->full_name); + + aplic.paddr_start = paddr; + aplic.paddr_end = paddr + size; + aplic.size = size; + + 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 = { + .info = &aplic_info, + .init = aplic_init, +}; + static int aplic_irq_xlate(const uint32_t *intspec, unsigned int intsize, unsigned int *out_hwirq, unsigned int *out_type) @@ -52,8 +145,12 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat) aplic_info.node = node; + aplic.imsic_cfg = imsic_get_config(); + dt_irq_xlate = aplic_irq_xlate; + register_intc_ops(&aplic_ops); + return 0; } diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h new file mode 100644 index 0000000000..94b3d0b616 --- /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 */ diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h index 0d498b10f4..db53caa07b 100644 --- a/xen/arch/riscv/include/asm/intc.h +++ b/xen/arch/riscv/include/asm/intc.h @@ -15,6 +15,9 @@ enum intc_version { struct intc_info { enum intc_version hw_version; const struct dt_device_node *node; + + /* number of irqs */ + unsigned int nr_irqs; }; struct intc_hw_operations { diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h index ff1c95e0be..163a478d78 100644 --- 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 #define nr_static_irqs 0 #define arch_hwdom_irqs(domid) 0U
aplic_init() function does the following few things: - checks that IMSIC in device tree node ( by checking msi-parent property in APLIC node ) is present as current one implmenetaion of AIA is supported only MSI method. - initialize IMSIC based on IMSIC device tree node - Read value of APLIC's paddr start/end and size. - Map aplic.regs - Setup APLIC initial state interrupts (disable all interrupts, set interrupt type and default priority, confgifure APLIC domaincfg) by calling aplic_init_hw_interrutps(). aplic_init() is based on the code from [1] and [2]. Since Microchip originally developed aplic.c, an internal discussion with them led to the decision to use the MIT license. [1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d [2] https://gitlab.com/xen-project/people/olkur/xen/-/commit/392a531bfad39bf4656ce8128e004b241b8b3f3e Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/aplic.c | 97 ++++++++++++++++++++++++++++++ xen/arch/riscv/include/asm/aplic.h | 77 ++++++++++++++++++++++++ xen/arch/riscv/include/asm/intc.h | 3 + xen/arch/riscv/include/asm/irq.h | 1 - 4 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 xen/arch/riscv/include/asm/aplic.h