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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 21/05/2024 05:35, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
>
> Currently, routing/removing physical interrupts are only allowed at
> the domain creation/destroy time. For use cases such as dynamic device
> tree overlay adding/removing, the routing/removing of physical IRQ to
> running domains should be allowed.
>
> Removing the above-mentioned domain creation/dying check. Since this
> will introduce interrupt state unsync issues for cases when the
> interrupt is active or pending in the guest, therefore for these cases
> we simply reject the operation. Do it for both new and old vGIC
> implementations.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> ---
> v3:
> - Update in-code comments.
> - Correct the if conditions.
> - Add taking/releasing the vgic lock of the vcpu.
> v2:
> - Reject the case where the IRQ is active or pending in guest.
> ---
>   xen/arch/arm/gic-vgic.c  | 15 ++++++++++++---
>   xen/arch/arm/gic.c       | 15 ---------------
>   xen/arch/arm/vgic/vgic.c | 10 +++++++---
>   3 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 56490dbc43..956c11ba13 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, 
> unsigned int virq,
>   
>       /* We are taking to rank lock to prevent parallel connections. */
>       vgic_lock_rank(v_target, rank, flags);
> +    spin_lock(&v_target->arch.vgic.lock);
I know this is what Stefano suggested, but v_target would point to the 
current affinity whereas the interrupt may be pending/active on the 
"previous" vCPU. So it is a little unclear whether v_target is the 
correct lock. Do you have more pointer to show this is correct?
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?

What about the other flags? Is this going to be a concern if we don't 
reset them?
Cheers,

--
Julien Grall