Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien, Stefano,

On 5/22/2024 9:03 PM, Julien Grall wrote:
> Hi Henry,
>
> On 22/05/2024 02:22, Henry Wang wrote:
> > > > > Also, while looking at the locking, I noticed that we are not 
> > > > > doing anything
> > > > > with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem to 
> > > > > assume that
> > > > > if the flag is set, then p->desc cannot be NULL.
> > > > >
> > > > > Can we reach vgic_connect_hw_irq() with the flag set?
> > > > I think even from the perspective of making the code extra safe, we 
> > > > should
> > > > also check GIC_IRQ_GUEST_MIGRATING as the LR is allocated for this 
> > > > case. I
> > > > will also add the check of GIC_IRQ_GUEST_MIGRATING here.
> > > Yes. I think it might be easier to check for GIC_IRQ_GUEST_MIGRATING
> > > early and return error immediately in that case. Otherwise, we can
> > > continue and take spin_lock(&v_target->arch.vgic.lock) because no
> > > migration is in progress
> > Ok, this makes sense to me, I will add
> >
> >      if( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> >      {
> >          vgic_unlock_rank(v_target, rank, flags);
> >          return -EBUSY;
> >      }
> >
> > right after taking the vgic rank lock.
Summary of our yesterday's discussion on Matrix:
For the split of patch mentioned in...

> I think that would be ok. I have to admit, I am still a bit wary about 
> allowing to remove interrupts when the domain is running.
> I am less concerned about the add part. Do you need the remove part 
> now? If not, I would suggest to split in two so we can get the most of 
> this series merged for 4.19 and continue to deal with the remove path 
> in the background.
...here, I will do that in the next version.

> I will answer here to the other reply:
>
> > I don't think so, if I am not mistaken, no LR will be allocated with 
> other flags set.
> I wasn't necessarily thinking about the LR allocation. I was more 
> thinking whether there are any flags that could still be set.
> IOW, will the vIRQ like new once vgic_connect_hw_irq() is succesful?
>
> Also, while looking at the flags, I noticed we clear _IRQ_INPROGRESS 
> before vgic_connect_hw_irq(). Shouldn't we only clear *after*?
This is a good catch, with the logic of vgic_connect_hw_irq() extended 
to reject the invalid cases, it is indeed safer to clear the 
_IRQ_INPROGRESS  after the successful vgic_connect_hw_irq(). I will move 
it after.
> This brings to another question. You don't special case a dying 
> domain. If the domain is crashing, wouldn't this mean it wouldn't be 
> possible to destroy it?
Another good point, thanks. I will try to make a special case of the 
dying domain.
Kind regards,
Henry


> Cheers,