Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 4/24/2024 8:58 PM, Julien Grall wrote:
> Hi Henry,
>
> On 24/04/2024 04:34, Henry Wang wrote:
> > From: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> >
> > Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
> >
> > Currently, irq_route and mapping is only allowed at the domain 
> > creation. Adding
> > exception for CONFIG_OVERLAY_DTB.
> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when 
> a physical IRQ can be routed/removed from/to a domain").
> > Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> > ---
> >   xen/arch/arm/gic.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 44c40e86de..a775f886ed 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, 
> > unsigned int virq,
> >        * back to the physical IRQ. To prevent get unsync, restrict the
> >        * routing to when the Domain is been created.
> >        */
> The above comment explains why the check was added. But the commit 
> message doesn't explain why this can be disregarded for your use-case.
> Looking at the history, I don't think you can simply remove the checks.
>
> Regardless that...
>
> > +#ifndef CONFIG_OVERLAY_DTB
> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB 
> enabled, yet the user will not use it.
> Instead, you want to remove the check once the code can properly 
> handle routing an IRQ the domain is created or ...
> >       if ( d->creation_finished )
> >           return -EBUSY;
> > +#endif
> >         ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
> >       if ( ret )
> > @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, 
> > unsigned int virq,
> >        * Removing an interrupt while the domain is running may have
> >        * undesirable effect on the vGIC emulation.
> >        */
> > +#ifndef CONFIG_OVERLAY_DTB
> >       if ( !d->is_dying )
> >           return -EBUSY;
> > +#endif
> ... removed before they domain is destroyed.
Thanks for your feeedback. After checking the b8577547236f commit 
message I think I now understand your point. Do you have any suggestion 
about how can I properly add the support to route/remove the IRQ to 
running domains? Thanks.
Kind regards,
Henry

> > desc->handler->shutdown(desc);
> Cheers,