Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
On Mon, 20 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 20/05/2024 02:01, Henry Wang wrote:
> > Hi Julien,
> >
> > On 5/19/2024 7:08 PM, Julien Grall wrote:
> > > Hi,
> > >
> > > On 17/05/2024 07:03, Henry Wang wrote:
> > > > > @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct
> > > > > vcpu *v, unsigned int virq,
> > > > > {
> > > > > /* The VIRQ should not be already enabled by the guest */
> > >
> > > This comment needs to be updated.
> >
> > Yes, sorry. I will update this and the one in the new vGIC in v3.
> >
> > > > > if ( !p->desc &&
> > > > > - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> > > > > + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > > > + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
> > > > > + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > > > p->desc = desc;
> > > > > else
> > > > > ret = -EBUSY;
> > > > > }
> > > > > else
> > > > > {
> > > > > - if ( desc && p->desc != desc )
> > > > > + if ( desc && p->desc != desc &&
> > > > > + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> > > > > + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
> > > >
> > > > This should be
> > > >
> > > > + if ( (desc && p->desc != desc) ||
> > > > + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> > > > + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > Looking at gic_set_lr(), we first check p->desc, before setting
> > > IRQ_GUEST_VISIBLE.
> > >
> > > I can't find a common lock, so what would guarantee that p->desc is not
> > > going to be used or IRQ_GUEST_VISIBLE set afterwards?
> >
> > I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
> > taken, at least the current two callers (gic_raise_guest_irq() and
> > gic_restore_pending_irqs()) are doing it this way. Would this address your
> > concern? Thanks.
>
> I don't think it would address my concern. AFAICT, the lock is not taken by
> vgic_connect_hw_irq().
>
> I also haven't touched the vGIC for quite a while and didn't have much time to
> dig into the code. Hence why I didn't propose a fix.
>
> The vGIC code was mainly written by Stefano, so maybe he will have an idea how
> this could be fixed.
I think we need to take the v->arch.vgic.lock just after the rank lock
in vgic_connect_hw_irq():
vgic_lock_rank(v_target, rank, flags);
spin_lock(&v_target->arch.vgic.lock);
Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi Henry,
On 20/05/2024 02:01, Henry Wang wrote:
Hi Julien,
On 5/19/2024 7:08 PM, Julien Grall wrote:
Hi,
On 17/05/2024 07:03, Henry Wang wrote:
@@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d,
struct vcpu *v, unsigned int virq,
{
/* The VIRQ should not be already enabled by the guest */
This comment needs to be updated.
Yes, sorry. I will update this and the one in the new vGIC in v3.
if ( !p->desc &&
- !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
- if ( desc && p->desc != desc )
+ if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
This should be
+ if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting
IRQ_GUEST_VISIBLE.
I can't find a common lock, so what would guarantee that p->desc is
not going to be used or IRQ_GUEST_VISIBLE set afterwards?
I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
taken, at least the current two callers (gic_raise_guest_irq() and
gic_restore_pending_irqs()) are doing it this way. Would this address
your concern? Thanks.
I don't think it would address my concern. AFAICT, the lock is not taken
by vgic_connect_hw_irq().
I also haven't touched the vGIC for quite a while and didn't have much
time to dig into the code. Hence why I didn't propose a fix.
The vGIC code was mainly written by Stefano, so maybe he will have an
idea how this could be fixed.
Cheers,
--
Julien Grall
Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi Julien,
On 5/19/2024 7:08 PM, Julien Grall wrote:
Hi,
On 17/05/2024 07:03, Henry Wang wrote:
@@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d,
struct vcpu *v, unsigned int virq,
{
/* The VIRQ should not be already enabled by the guest */
This comment needs to be updated.
Yes, sorry. I will update this and the one in the new vGIC in v3.
if ( !p->desc &&
- !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
- if ( desc && p->desc != desc )
+ if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
This should be
+ if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting
IRQ_GUEST_VISIBLE.
I can't find a common lock, so what would guarantee that p->desc is
not going to be used or IRQ_GUEST_VISIBLE set afterwards?
I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
taken, at least the current two callers (gic_raise_guest_irq() and
gic_restore_pending_irqs()) are doing it this way. Would this address
your concern? Thanks.
Kind regards,
Henry
Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi,
On 17/05/2024 07:03, Henry Wang wrote:
On 5/16/2024 6:03 PM, Henry Wang wrote:
From: Vikram Garhwal
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
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
v2:
- Reject the case where the IRQ is active or pending in guest.
---
xen/arch/arm/gic-vgic.c | 8 ++--
xen/arch/arm/gic.c | 15 ---
xen/arch/arm/vgic/vgic.c | 5 +++--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..d1608415f8 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct
vcpu *v, unsigned int virq,
{
/* The VIRQ should not be already enabled by the guest */
This comment needs to be updated.
if ( !p->desc &&
- !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
- if ( desc && p->desc != desc )
+ if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
This should be
+ if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting
IRQ_GUEST_VISIBLE.
I can't find a common lock, so what would guarantee that p->desc is not
going to be used or IRQ_GUEST_VISIBLE set afterwards?
@@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct
vcpu *vcpu,
}
else /* remove a mapped IRQ */
{
- if ( desc && irq->hwintid != desc->irq )
+ if ( desc && irq->hwintid != desc->irq &&
+ (irq->active || irq->pending_latch) )
Same here, this should be
+ if ( (desc && irq->hwintid != desc->irq) ||
+ irq->active || irq->pending_latch )
I think the new vGIC doesn't have the same problem because it looks like
the IRQ lock will be taken for any access to 'irq'.
Cheers,
--
Julien Grall
Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
On 5/16/2024 6:03 PM, Henry Wang wrote:
From: Vikram Garhwal
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
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
v2:
- Reject the case where the IRQ is active or pending in guest.
---
xen/arch/arm/gic-vgic.c | 8 ++--
xen/arch/arm/gic.c | 15 ---
xen/arch/arm/vgic/vgic.c | 5 +++--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..d1608415f8 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -444,14 +444,18 @@ 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_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
-if ( desc && p->desc != desc )
+if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
This should be
+if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
@@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
}
else/* remove a mapped IRQ */
{
-if ( desc && irq->hwintid != desc->irq )
+if ( desc && irq->hwintid != desc->irq &&
+ (irq->active || irq->pending_latch) )
Same here, this should be
+if ( (desc && irq->hwintid != desc->irq) ||
+ irq->active || irq->pending_latch )
Kind regards,
Henry
[PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
From: Vikram Garhwal
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
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
v2:
- Reject the case where the IRQ is active or pending in guest.
---
xen/arch/arm/gic-vgic.c | 8 ++--
xen/arch/arm/gic.c | 15 ---
xen/arch/arm/vgic/vgic.c | 5 +++--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..d1608415f8 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -444,14 +444,18 @@ 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_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
-if ( desc && p->desc != desc )
+if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
ret = -EINVAL;
else
p->desc = NULL;
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..3ebd89940a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -135,14 +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.
- */
-if ( d->creation_finished )
-return -EBUSY;
-
ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
if ( ret )
return ret;
@@ -167,13 +159,6 @@ 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.
- */
-if ( !d->is_dying )
-return -EBUSY;
-
desc->handler->shutdown(desc);
/* EOI the IRQ if it has not been done by the guest */
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index b9463a5f27..785ef2b192 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -877,7 +877,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
if ( connect ) /* assign a mapped IRQ */
{
/* The VIRQ should not be already enabled by the guest */
-if ( !irq->hw && !irq->enabled )
+if ( !irq->hw && !irq->enabled && !irq->active && !irq->pending_latch )
{
irq->hw = true;
irq->hwintid = desc->irq;
@@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
}
else/* remove a mapped IRQ */
{
-if ( desc && irq->hwintid != desc->irq )
+if ( desc && irq->hwintid != desc->irq &&
+ (irq->active || irq->pending_latch) )
{
ret = -EINVAL;
}
--
2.34.1
