On 16.04.2025 21:05, Oleksii Kurochko wrote:
> On 4/15/25 2:46 PM, Jan Beulich wrote:
>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>> Introduce interrupt controller descriptor for host APLIC to describe
>>> the low-lovel hardare. It includes implementation of the following
>>> functions:
>>> - aplic_irq_startup()
>>> - aplic_irq_shutdown()
>>> - aplic_irq_enable()
>>> - aplic_irq_disable()
>>> - aplic_irq_ack()
>>> - aplic_host_irq_end()
>>> - aplic_set_irq_affinity()
>>>
>>> As APLIC is used in MSI mode it requires to enable/disable interrupts not
>>> only for APLIC but also for IMSIC. Thereby for the purpose of
>>> aplic_irq_{enable,disable}() it is introduced imsic_irq_{enable,disable)().
>>>
>>> For the purpose of aplic_set_irq_affinity() aplic_get_cpu_from_mask() is
>>> introduced to get hart id.
>>>
>>> Also, introduce additional interrupt controller h/w operations and
>>> host_irq_type for APLIC:
>>> - aplic_host_irq_type
>>> - aplic_set_irq_priority()
>>> - aplic_set_irq_type()
>> Yet these two functions nor the hooks they're used to populate are entirely
>> unused here. Since they're also outside of the common IRQ handling machinery,
>> it's unclear how one would sanely ack such a change.
>
> They will be called by intc_route_irq_to_xen() from setup_irq() during firt
> time
> the IRQ is setup.
Perhaps move their introduction to there then? We don't do any Misra checking
yet lon RISC-V, but imo it's still good practice to avoid introducing new
violations, even if only temporarily.
>>> --- a/xen/arch/riscv/aplic.c
>>> +++ b/xen/arch/riscv/aplic.c
>>> @@ -15,6 +15,7 @@
>>> #include <xen/irq.h>
>>> #include <xen/mm.h>
>>> #include <xen/sections.h>
>>> +#include <xen/spinlock.h>
>>> #include <xen/types.h>
>>> #include <xen/vmap.h>
>>>
>>> @@ -110,9 +111,173 @@ static int __init aplic_init(void)
>>> return 0;
>>> }
>>>
>>> -static const struct intc_hw_operations __ro_after_init aplic_ops = {
>>> +static void aplic_set_irq_type(struct irq_desc *desc, unsigned int type)
>>> +{
>>> + unsigned int irq = desc->irq - 1;
>> Why this adjustment by 1 (and yet both items being named "irq")?
>
> 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.
Okay, fine. But what about the part of the question in parentheses?
>>> + 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 fear this doesn't answer my question, which is to a large part related
to the Xen code, and only to some degree to the spec.
>>> +static void aplic_set_irq_priority(struct irq_desc *desc,
>>> + unsigned int priority)
>>> +{
>>> + /* No priority, do nothing */
>>> +}
>> Since the function dopes nothing, wouldn't it be better to omit it and have
>> the (future) caller check for a NULL pointer ahead of making the (indirect)
>> call? Same remark for other handlers (below) which also do nothing.
>
> 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.
I guess I don't understand the concern.
>>> +static void aplic_irq_enable(struct irq_desc *desc)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + /*
>>> + * TODO: Currently, APLIC is supported only with MSI interrupts.
>>> + * If APLIC without MSI interrupts is required in the future,
>>> + * this function will need to be updated accordingly.
>>> + */
>>> + ASSERT(aplic.imsic_cfg->is_used);
>> Such an extra field, used only for assertions, is pretty odd. Can't you
>> use any of the other fields to achieve the same effect?
>
> 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.
Since they serve primarily as a reminder where changes would need making,
I'd prefer if they could be kept.
> Or introduce static variable in aplic.c `aplic_mode`, init it in aplic_init()
> and use it in ASSERT().
This would then again be used solely for assertions, aiui? As said, I
think it would be preferable if some already existing indicator could be
used for this purpose.
>>> + 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.
And then - does it need to be a bitop? Aiui that's what Arm uses, while x86
doesn't. And I see no reason to use other than plain C operators here. If
Arm was switched, presumably all the redundant (and misnamed) _IRQ_*
constants could go away, with just the IRQ_* ones left.
>> 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.
What about this part?
>> 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
> spechttps://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?
Except for the "doesn't really matter" - yes. In a reply to a later patch I
indicated I realized that IMSIC is what's closer to the CPU (and hence later
in the chain of interrupt delivery actions).
>>> + spin_unlock_irqrestore(&aplic.lock, flags);
>>> +}
>>> +
>>> +static void aplic_irq_disable(struct irq_desc *desc)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + /*
>>> + * TODO: Currently, APLIC is supported only with MSI interrupts.
>>> + * If APLIC without MSI interrupts is required in the future,
>>> + * this function will need to be updated accordingly.
>>> + */
>>> + ASSERT(aplic.imsic_cfg->is_used);
>>> +
>>> + ASSERT(spin_is_locked(&desc->lock));
>>> +
>>> + spin_lock_irqsave(&aplic.lock, flags);
>>> +
>>> + set_bit(_IRQ_DISABLED, &desc->status);
>>> +
>>> + /* disable interrupt in APLIC */
>>> + aplic.regs->clrienum = desc->irq;
>>> +
>>> + /* disable interrupt in IMSIC */
>>> + imsic_irq_disable(desc->irq);
>>> +
>>> + spin_unlock_irqrestore(&aplic.lock, flags);
>>> +}
>>> +
>>> +static unsigned int aplic_irq_startup(struct irq_desc *desc)
>>> +{
>>> + aplic_irq_enable(desc);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void aplic_irq_shutdown(struct irq_desc *desc)
>>> +{
>>> + aplic_irq_disable(desc);
>>> +}
>> You don't really need a separate hook function here, do you?
>
> With such implementation it is really not needed to have a hook so
> I will drop it.
>
>>> +static void aplic_irq_ack(struct irq_desc *desc)
>>> +{
>>> + /* nothing to do */
>>> +}
>>> +
>>> +static void aplic_host_irq_end(struct irq_desc *desc)
>> What's the "host" in the identifier about?
>
> 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,
> ...
> };
> ```
And you expect to end up with a similar distinction on RISC-V? There's
nothing like that on x86, just to mention it.
>>> +static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t
>>> *mask)
>>> +{
>>> + unsigned int cpu;
>>> + uint64_t group_index, base_ppn;
>>> + uint32_t hhxw, lhxw ,hhxs, value;
>>> + const struct imsic_config *imsic = aplic.imsic_cfg;
>>> +
>>> + /*
>>> + * TODO: Currently, APLIC is supported only with MSI interrupts.
>>> + * If APLIC without MSI interrupts is required in the future,
>>> + * this function will need to be updated accordingly.
>>> + */
>>> + ASSERT(aplic.imsic_cfg->is_used);
>> Use the local variable you have made yourself?
>
> What do you mean by local here?
Just a few lines up you latch aplic.imsic_cfg into the local "imsic".
>>> + 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
In the spec that's fine, but please make yourself a constant with a suitable
name then, to be used here. Just consider what would happen if we used literal
12 everywhere PAGE_SHIFT was meant.
>>> +void imsic_irq_enable(unsigned int hwirq)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&imsic_cfg.lock, flags);
>>> + imsic_local_eix_update(hwirq, 1, false, true);
>> No subtraction of 1 here? Also, why "hwirq" and not just "irq"?
>
> 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.
> ...
>
> Bit positions in a valid eiek register that don’t correspond to a
> supported interrupt identity (such as bit 0 of eie0) are read-only zeros.
>
>
> ```
>
> So in EIx registers interrupt i corresponds to bit i in comparison wiht
> APLIC's sourcecfg which starts from 0.
Confusing, but what do you do.
>>> @@ -277,6 +333,13 @@ int __init imsic_init(struct dt_device_node *node)
>>> goto imsic_init_err;
>>> }
>>>
>>> + spin_lock_init(&imsic_cfg.lock);
>>> +
>>> + /* Enable local interrupt delivery */
>>> + imsic_ids_local_delivery(true);
>> What's this? I can't find the function/macro here, nor in patch 08, nor in
>> staging.
>
> 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);
> }
> }
> ```
No, it's not. As noted in the reply to a later patch, it's only introduced
there. Hence the build will break between the two patches.
Jan
