|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 10/14] xen/riscv: implementation of aplic and imsic operations
On 4/15/25 2:46 PM, Jan Beulich wrote:
On 08.04.2025 17:57, Oleksii Kurochko wrote: They will be called by intc_route_irq_to_xen() from setup_irq() during firt time the IRQ is setup.
Interrupt 0 isn't possible based on the spec: ``` Each of an APLIC’s interrupt sources has a fixed unique identity number in the range 1 to N, where N is the total number of sources at the APLIC. The number zero is not a valid interrupt identity number at an APLIC. The maximum number of interrupt sources an APLIC may support is 1023. ``` and interrupt 1 will correspond to bit 0 in sourcecfg[] register, interrupt 2 ->sourcecfg[1] and so on. And that is the reason why we need -1. + aplic.regs->sourcecfg[irq] = APLIC_SOURCECFG_SM_EDGE_FALL; + break; + case IRQ_TYPE_LEVEL_HIGH: + aplic.regs->sourcecfg[irq] = APLIC_SOURCECFG_SM_LEVEL_HIGH; + break; + case IRQ_TYPE_LEVEL_LOW: + aplic.regs->sourcecfg[irq] = APLIC_SOURCECFG_SM_LEVEL_LOW; + break; + default: + aplic.regs->sourcecfg[irq] = APLIC_SOURCECFG_SM_INACTIVE; + break;Is the default: label legitimate to be reached? From the spec: ``` 0 Inactive Inactive in this domain (and not delegated) 1 Detached Active, detached from the source wire 2–3 — Reserved 4 Edge1 Active, edge-sensitive; interrupt asserted on rising edge 5 Edge0 Active, edge-sensitive; interrupt asserted on falling edge 6 Level1 Active, level-sensitive; interrupt asserted when high 7 Level0 Active, level-sensitive; interrupt asserted when low ``` It seems to me like APLIC_SOURCECFG_SM_INACTIVE just covers cases (0-3) and inactive IRQ pretty safe to as a default value.
I thought about that too, but it could be some cases when the stub is introduced
with temporary BUG_ON("unimplemented") inside just to not miss to implement it
when it will be necessary.
If we will have only the caller check then we could miss to implement such stubs.
in aplic_init() there is:
/* 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);
So we will have panic() anyway if MSI mode isn't supported. As an option we
can just drop the ASSERT.
Or introduce static variable in aplic.c `aplic_mode`, init it in aplic_init()
and use it in ASSERT().
+ ASSERT(spin_is_locked(&desc->lock));If this lock (which is an IRQ-safe one) is necessarily held, ...+ spin_lock_irqsave(&aplic.lock, flags);... you can use just spin_lock() here.+ clear_bit(_IRQ_DISABLED, &desc->status);Why an atomic bitop when desc is locked? (And yes, I ought to raise the same question on Arm code also doing so.) I haven't thought about that. Likely non-atomic bitop could be used here. I'm uncertain about this bit setting anyway - on x86 we would only fiddle with it for IRQs not in use, not while enabling/disabling one. In any event this can be done outside of the APLIC-locked region, I think. Considering that we are doing that under desc->lock, agree we can move that outside the APLIC-locked region. + imsic_irq_enable(desc->irq); + + /* enable interrupt in APLIC */ + aplic.regs->setienum = desc->irq;Are you sure you want to use plain assignments for MMIO accesses? I'd have expected writel() to be used here. (And only later I realized that I didn't spot the same already higher up from here.) Good point. I have to update that with writel()... >From the vague understanding I've gained so far: Isn't the APLIC closer to the CPU and the IMSIC closer to the device? If so, wouldn't you want to enable at the APLIC before enabling at the IMSIC? But of course that also depends on what exactly happens in the window while one is already enabled and the other is still disabled. (Later) From the code you add to imsic.c it looks like it's the other way around, as the IMSIC is accessed through CSRs. From the AIA spec: ``` An Incoming MSI Controller (IMSIC) is an optional RISC-V hardware component that is closely coupled with a hart, one IMSIC per hart. An IMSIC receives and records incoming message-signaled interrupts (MSIs) for a hart, and signals to the hart when there are pending and enabled interrupts to be serviced. ``` Based on the figure 2 (Interrupt delivery by MSIs when harts have IMSICs for receiving them) of AIA spec https://github.com/riscv/riscv-aia/blob/main/src/intrsWithIMSICs.png IMSIC is more close to CPU and APLIC is more close to the device. The external interrupt controller is APLIC and it only sends a MSI message for a CPU. The logical flow of an interrupt to a hart with an IMSIC would be: 1. A physical interrupt signal arrives at the APLIC. 2. The APLIC, if configured for MSI delivery mode (domaincfg.DM = 1) and if the specific interrupt source is active and enabled within its domain (controlled by sourcecfg[i] and the global Interrupt Enable bit IE in domaincfg), will generate an MSI. 3. This MSI is then sent to the target hart's IMSIC. The APLIC needs to know the MSI target address for each hart, which can be hardwired or configured through registers like mmsiaddrcfg and mmsiaddrcfgh. 4. The receiving hart's IMSIC records this MSI as a pending interrupt. 5. If the corresponding interrupt identity is enabled within the IMSIC's interrupt file, the IMSIC will signal the hart, typically by setting the MEIP or SEIP bit in the mip CSR (or sip CSR). Generally, I think that the order in which enable interrupts doesn't really matter as if you were to enable the IMSIC to receive a certain interrupt before the APLIC was configured to send it (or had a pending interrupt from the device), the IMSIC would simply be waiting for an MSI that wouldn't arrive. Similarly, if the APLIC sends an MSI for an interrupt that is not enabled in the IMSIC, the interrupt would remain pending in the IMSIC but wouldn't trigger an interrupt at the hart. IMO, the order which is used now in the code is pretty logical. Does it make sense?
With such implementation it is really not needed to have a hook so I will drop it.
It was copied that from Arm and my understanding that it means
Xen-related IRQ as they also have:
```
/* XXX different for level vs edge */
static hw_irq_controller gicv2_host_irq_type = {
...
.end = gicv2_host_irq_end,
...
};
static hw_irq_controller gicv2_guest_irq_type = {
...
.end = gicv2_guest_irq_end,
...
};
```
Yes, it could be dropped. + cpumask_t possible_mask; + + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); + cpu = cpumask_any(&possible_mask);Why would you use cpu_possible_map here? That includes any offline CPUs. I think you need to use cpu_online_map here. It makes sense, Ill switch to cpu_online_map.
What do you mean by local here? + ASSERT(!cpumask_empty(mask)); + + spin_lock(&aplic.lock);Aiui the lock can be acquired quite a bit later. It ought to be needed only around the actual write to the hardware register.+ cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask)); + hhxw = imsic->group_index_bits; + lhxw = imsic->hart_index_bits; + hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2; + base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT; + + /* update hart and EEID in the target register */ + group_index = (base_ppn >> (hhxs + 12)) & (BIT(hhxw, UL) - 1);What's this magic 12 in here? Not IMSIC_MMIO_PAGE_SHIFT I suppose? In the AIA spec they are using 12 explicitly: https://github.com/riscv/riscv-aia/blob/main/src/AdvPLIC.adoc#AdvPLIC-MSIAddrs
Good point. I will double-check.
From the spec:
```
When an interrupt file supports distinct interrupt identities, valid identity numbers are between 1
and inclusive. The identity numbers within this range are said to be implemented by the interrupt
file; numbers outside this range are not implemented. The number zero is never a valid interrupt
identity.
...
It is defined in imsic.c:
```
void imsic_ids_local_delivery(bool enable)
{
if ( enable )
{
imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD);
imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_ENABLE_EIDELIVERY);
}
else
{
imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_DISABLE_EITHRESHOLD);
imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_DISABLE_EIDELIVERY);
}
}
```
Thanks for review.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |
