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,
