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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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