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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3232 bytes --]


On 4/28/25 10:54 AM, Jan Beulich wrote:
>>>>>> +    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.

Okay, then at the moment there is no such cases and I'll use plain C operator instead of
clear/set_bit().

>
> 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.

On Arm, the physical interrupts would be enabled when the interrupt is initially routed and in case guest
is booting with interrupt disabled, it could introduce a problem when guest enabled interrupts it will
already have a pending interrupt for which it isn't ready.

How is it handled the case when a device isn't quiescing at the boot time in x86?

But I just realized the way how interrupts are enabled in RISC-V for guest won't lead to such case. The interrupt
will be enabled only when guest's device driver will request that. So this setting/clearing of IRQ_DISABLED could
be dropped for RISC-V.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 5092 bytes --]