Re: [PATCH v9 16/20] xen/riscv: implement IRQ routing for device passthrough

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2026 12:54, Oleksii Kurochko wrote:
> On 9/10/26 2:53 PM, Jan Beulich wrote:
>> On 09.09.2026 17:07, Oleksii Kurochko wrote:
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -205,7 +205,7 @@ void __init init_IRQ(void)
>>>   static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>>>   {
>>>       ASSERT(spin_is_locked(&desc->lock));
>>> -    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>>> +    ASSERT(desc->status & IRQ_GUEST);
>>>       ASSERT(desc->action != NULL);
>>>   
>>>       return desc->action->dev_id;
>>
>> Why did an Arm change slip into a RISC-V patch?
> 
> Considering that this code is executed under the spinlock it is fine to 
> not use test_bit() so it is part of dropping of bitops functions.

Perhaps, but then in a separate patch and changing all places consistently
(I didn't go check, but I'm pretty sure there are more).

>>> --- a/xen/arch/riscv/aplic.c
>>> +++ b/xen/arch/riscv/aplic.c
>>> @@ -325,9 +325,40 @@ static const hw_irq_controller aplic_xen_irq_type = {
>>>       .set_affinity = aplic_set_irq_affinity,
>>>   };
>>>   
>>> +static unsigned int cf_check aplic_guest_irq_startup(struct irq_desc *desc)
>>> +{
>>> +    BUG_ON("unimplemented");
>>> +}
>>> +
>>> +/*
>>> + * Shared by ->shutdown(), ->enable(), ->disable() and ->end(), which have
>>> + * no state.
>>> + */
>>> +static void cf_check aplic_guest_irq_stub(struct irq_desc *desc)
>>> +{
>>> +    BUG_ON("unimplemented");
>>> +}
>>> +
>>> +static void cf_check aplic_guest_set_irq_affinity(struct irq_desc *desc,
>>> +                                                  const cpumask_t *mask)
>>> +{
>>> +    BUG_ON("unimplemented");
>>> +}
>>> +
>>> +static const hw_irq_controller aplic_guest = {
>>> +    .typename     = "aplic",
>>
>> This is indistinguishable from aplic_xen_irq_type. Naming of both variables
>> also isn't consistent
> 
> I will align names properly:
> 
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -316,8 +316,8 @@ static void cf_check aplic_set_irq_type(struct 
> irq_desc *desc,
>       spin_unlock(&aplic.lock);
>   }
> 
> -static const hw_irq_controller aplic_xen_irq_type = {
> -    .typename     = "aplic",
> +static const hw_irq_controller aplic_host_irq_type = {
> +    .typename     = "aplic-host",
>       .startup      = aplic_irq_startup,
>       .shutdown     = aplic_irq_disable,
>       .enable       = aplic_irq_enable,

I'm not convinced this one is needed.

> @@ -345,8 +345,8 @@ static void cf_check 
> aplic_guest_set_irq_affinity(struct irq_desc *desc,
>       BUG_ON("unimplemented");
>   }
> 
> -static const hw_irq_controller aplic_guest = {
> -    .typename     = "aplic",
> +static const hw_irq_controller aplic_guest_irq_type = {
> +    .typename     = "aplic-guest",
>       .startup      = aplic_guest_irq_startup,
>       .shutdown     = aplic_guest_irq_stub,
>       .enable       = aplic_guest_irq_stub,

This one certainly is.

Jan