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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.
> > --- 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,
@@ -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,
@@ -357,8 +357,8 @@ static const hw_irq_controller aplic_guest = {

 static const struct intc_hw_operations aplic_ops = {
     .info                = &aplic_info,
-    .host_irq_type       = &aplic_xen_irq_type,
-    .guest_irq_type      = &aplic_guest,
+    .host_irq_type       = &aplic_host_irq_type,
+    .guest_irq_type      = &aplic_guest_irq_type,
     .handle_interrupt    = aplic_handle_interrupt,
     .set_irq_type        = aplic_set_irq_type,
 };

> > --- a/xen/arch/riscv/include/asm/intc.h
> > +++ b/xen/arch/riscv/include/asm/intc.h
> > @@ -15,6 +15,7 @@ enum intc_variant {
> >   };
> >   
> >   struct cpu_user_regs;
> > +struct domain;
> >   struct irq_desc;
> >   struct kernel_info;
> >   struct vcpu;
> Why is this suddenly necessary? There are ...
>
> > @@ -76,10 +82,13 @@ void register_intc_ops(const struct intc_hw_init_ops 
> > *init_ops);
> >   void intc_init(void);
> >   
> >   void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
> > +int intc_route_irq_to_guest(struct irq_desc *desc, unsigned int priority);
> >   
> >   void intc_handle_external_irqs(struct cpu_user_regs *regs);
> >   
> >   int domain_vintc_init(struct domain *d);
> >   void domain_vintc_deinit(struct domain *d);
> >   
> > +int vintc_reserve_virq(const struct domain *d, unsigned int virq);
> ... existing uses of struct domain already, immediately ahead of this added
> line.
Forward declaration of 'sturct domain' was missed before so it was added 
with this change. I can drop forward declaration as you noticed that it 
hasn't been needed before this change but it still make sense to add IMO.
> > @@ -78,6 +82,22 @@ void intc_route_irq_to_xen(struct irq_desc *desc, unsigned 
> > int priority)
> >       intc_set_irq_priority(desc, priority);
> >   }
> >   
> > +int intc_route_irq_to_guest(struct irq_desc *desc,
> > +                            unsigned int priority)
> > +{
> > +    ASSERT(spin_is_locked(&desc->lock));
> > +
> > +    ASSERT(intc_hw_ops->guest_irq_type);
> > +
> > +    desc->handler = intc_hw_ops->guest_irq_type;
> > +    __set_bit(_IRQ_GUEST, &desc->status);
> The revlog says you replaced all of these bitops.
>
> > @@ -227,3 +251,240 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
> >       spin_unlock(&desc->lock);
> >       irq_exit();
> >   }
> > +
> > +static struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
> > +{
> > +    ASSERT(spin_is_locked(&desc->lock));
> > +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> Same here. And more elsewhere.
>
> > +/* Route an IRQ to a specific guest */
> > +int route_irq_to_guest(struct domain *d, unsigned int virq,
> > +                       unsigned int irq, const char *devname)
> > +{
> > +    struct irq_guest *info;
> > +    struct irq_desc *desc = irq_to_desc(irq);
> > +    unsigned long flags;
> > +    int retval = 0;
> > +
> > +    if ( d->is_dying )
> > +        return -EINVAL;
> > +
> > +    info = xvzalloc(struct irq_guest);
> > +    if ( !info )
> > +        return -ENOMEM;
> > +
> > +    info->d = d;
> > +    info->virq = virq;
> > +
> > +    info->action.dev_id = info;
> > +    info->action.name = devname;
> > +    /* The action is part of 'info', thus it is freed together with it. */
> > +    info->action.free_on_release = false;
> The revlog says this line doesn't exist anymore.
I will double check all the places before sending v10.

Thanks.