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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 5/10/2024 4:54 PM, Julien Grall wrote:
> Hi,
>
> On 09/05/2024 16:31, Henry Wang wrote:
> > On 5/9/2024 4:46 AM, Julien Grall wrote:
> > > Hi Henry,
> > > [...]
> > > ```
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index a775f886ed..d3f9cd2299 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -135,16 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, 
> > > unsigned int virq,
> > >       ASSERT(virq < vgic_num_irqs(d));
> > >       ASSERT(!is_lpi(virq));
> > >
> > > -    /*
> > > -     * When routing an IRQ to guest, the virtual state is not synced
> > > -     * back to the physical IRQ. To prevent get unsync, restrict the
> > > -     * routing to when the Domain is been created.
> > > -     */
> > > -#ifndef CONFIG_OVERLAY_DTB
> > > -    if ( d->creation_finished )
> > > -        return -EBUSY;
> > > -#endif
> > > -
> > >       ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
> This is checking if the interrupt is already enabled. Do we also need 
> to check for active/pending?
Thank you for raising this! I assume you meant this?
@@ -444,7 +444,9 @@ int vgic_connect_hw_irq(struct domain *d, struct 
vcpu *v, unsigned int virq,
     {
         /* The VIRQ should not be already enabled by the guest */
         if ( !p->desc &&
-             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
             p->desc = desc;
         else
             ret = -EBUSY;

I think adding the check for active/pending check at the time of routing 
the IRQ makes sense, so I will add them (both for old and new vGIC 
implementation).
> >       if ( ret )
> >           return ret;
> > @@ -169,20 +159,40 @@ int gic_remove_irq_from_guest(struct domain *d, 
> > unsigned int virq,
> >       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> >       ASSERT(!is_lpi(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
> > -
> >       desc->handler->shutdown(desc);
> >
> >       /* EOI the IRQ if it has not been done by the guest */
> >       if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> > +    {
> I assume this is just a PoC state, but I just want to point out that 
> this will not work with the new vGIC (some of the functions doesn't 
> exist there).
Thank you. Yes currently we can discuss for the old vGIC implementation. 
After we reach the final conclusion I will do the changes for both old 
and new vGIC.
> > +        /*
> > +         * Handle the LR where the physical interrupt is de-assigned 
> > from the
> > +         * guest before it was EOIed
> > +         */
> > +        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> This will return a vCPU from the current affinity. This may not be 
> where the interrupt was injected. From a brief look, I can't tell 
> whether we have an easy way to know where the interrupt was injected 
> (other than the pending_irq is in the list lr_queue/inflight)
I doubt if we need to handle more than this - I think if the pending_irq 
is not in the lr_queue/inflight list, it would not belong to the corner 
case we are talking about (?).
> > +        }
> > + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> > +
> > +        vgic_lock_rank(v_target, rank, flags);
> > +        vgic_disable_irqs(v_target, (~rank->ienable) & 
> > rank->ienable, rank->index);
> > +        vgic_unlock_rank(v_target, rank, flags);
> Why do you need to call vgic_disable_irqs()?
I will drop this part.

Kind regards,
Henry