Re: [PATCH v1 10/14] xen/riscv: implementation of aplic and imsic operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2025 10:12, Oleksii Kurochko wrote:
> On 4/17/25 8:25 AM, Jan Beulich wrote:
>> 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.
> 
> Okay, I will move their introduction to there.
> 
> Btw, what is needed to add Misra checking for RISC-V? I started to think 
> that, probably,
> it will make sense to do that from the start.

Best I can do is point you at what is done for Arm and x86. You may want to
ask people more familiar with the CI aspects involved here.

>>>>> +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.
> 
> for example, if we will have the following code:
>    void some_caller(struct irq_desc *desc)
>    {
>        if ( desc->handler->set_affinity )
>            desc->handler->set_affinity(desc, cpu_mask);
>    }
> 
> Then we will skip the call of handler->set_affinity() (if it was just 
> initialized with
> .set_affinity = NULL) without any notification. And it is fine specifically 
> in this
> case as aplic_set_irq_priority() does nothing.
> 
> But what about the cases if it is a function which will have some 
> implementation in the
> future but doesn't have implementation for now. Then without notification 
> that this
> function is unimplemented we could skip something what really matters.
> 
> But I think that your initial comment was just about the function which 
> basically
> does nothing. Am i right?

Since indirect calls are not only more expensive (often; not sure about
RISC-V) but also pose speculative concerns, having such just to do nothing
simply seems like moving in the wrong direction.

>>>>> +    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.
> 
> The reason for a bitop in Arm is explained in this 
> commithttps://gitlab.com/xen-project/xen/-/commit/50d8fe8fcbab2440cfeeb65c4765868398652473
> but all the places where plain C operators were changed to bitops are 
> actually executed under|spin_lock_irqsave(&desc->lock, flags). By quick look 
> I found only two 
> places one in __setup_irq() but it is called by the functions which do 
> ||spin_lock_irqsave(&desc->lock, flags) and in vgic_v2_fold_lr_state(). 
> Maybe, I'm missing something.|
> |RISC-V won't have something similar to ||vgic_v2_fold_lr_state|(), but 
> __setup_irq() is used in a similar way. It can be added 
> ASSERT(spin_is_lock(&desc->lock))
> and then it will also safe to use non-bitop function.
> Probably, it is a little bit safer to use always bitops for desc->status.
> ||

I question that. If any accesses outside of locked regions were needed (as the
description of that commit suggests), then the situation would be different.

Btw, you not wrapping lines and you adding strange | instances doesn't help
readability of your replies.

>>>> 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?
> 
> As I understand, based on Arm, code then Xen enables interrupts corresponding 
> to devices assigned
> to dom0/domU before booting dom0/domU, resulting in the possibility of 
> receiving an interrupt
> and not knowing what to do with it. So it is needed for enablement of IRQs 
> when the guest
> requests it and not unconditionally at boot time.

I fear I don't understand this. The way we do things on x86 doesn't leave us
in such a situation.

Jan