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
