Currently, Xen will return IO unhandled when guests write ICPENDR*
virtual registers, which will raise a data abort inside the guest.
For Linux guest, these virtual registers will not be accessed. But
for Zephyr, these virtual registers will be accessed during the
initialization. Zephyr guest will get an IO data abort and crash.
Emulating ICPENDR is not easy with the existing vGIC, this patch
reworks the emulation to ignore write access to ICPENDR* virtual
registers and print a message about whether they are already pending
instead of returning unhandled.
More details can be found at [1].

[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274

Signed-off-by: Hongda Deng <hongda.deng@arm.com>
---
Changes since v3:
 *  Commit message modification
 *  Change "goto write_ignore_32" to "goto write_ignore" to avoid double
    checking dabt.size
 *  Delete data.size check in vgic_v3_rdistr_sgi_mmio_write to avoid
    double check in __vgic_v3_distr_common_mmio_write for SGI
 *  Declare flags, p, v_target within the loop to reduce their scope
 *  Use the same vgic_get_target_vcpu(v, irq) to get v_target for SPI,
    PPI and SGI
 *  Code principle modification
Changes since v2:
 *  Avoid to print messages when there is no pending interrupt
 *  Add helper vgic_check_inflight_irqs_pending to check pending status
 *  Print a message for each interrupt separately
Changes since v1:
 *  Check pending states by going through vcpu->arch.vgic.inflight_irqs
    instead of checking hardware registers
---
 xen/arch/arm/vgic-v2.c     | 10 ++++++----
 xen/arch/arm/vgic-v3.c     | 17 ++++++++---------
 xen/arch/arm/vgic.c        | 28 ++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h |  2 ++
 4 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..589c033eda 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
 
     case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk(XENLOG_G_ERR
-               "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d\n",
-               v, r, gicd_reg - GICD_ICPENDR);
-        return 0;
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+        if ( rank == NULL ) goto write_ignore;
+
+        vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+        goto write_ignore;
 
     case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
         if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..65bb7991a6 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,12 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
 
     case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk(XENLOG_G_ERR
-               "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
-               v, name, r, reg - GICD_ICPENDR);
-        return 0;
+        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+        if ( rank == NULL ) goto write_ignore;
+
+        vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+        goto write_ignore;
 
     case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -986,11 +988,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
                                                  info, gicr_reg, r);
 
     case VREG32(GICR_ICPENDR0):
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk(XENLOG_G_ERR
-               "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to ICPENDR0\n",
-               v, r);
-        return 0;
+        return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+                                                 info, gicr_reg, r);
 
     case VREG32(GICR_IGRPMODR0):
         /* We do not implement security extensions for guests, write ignore */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f9400a519..83386cf3d5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -726,6 +726,34 @@ unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
     }
 }
 
+void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
+                                      unsigned int rank, uint32_t r)
+{
+    const unsigned long mask = r;
+    unsigned int i;
+
+    for_each_set_bit( i, &mask, 32 )
+    {
+        struct pending_irq *p;
+        struct vcpu *v_target;
+        unsigned long flags;
+        unsigned int irq = i + 32 * rank;
+
+        v_target = vgic_get_target_vcpu(v, irq);
+
+        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+
+        p = irq_to_pending(v_target, irq);
+
+        if ( p && !list_empty(&p->inflight) )
+            printk(XENLOG_G_WARNING
+                   "%pv trying to clear pending interrupt %u.\n",
+                   v, irq);
+
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 62c2ae538d..e69a59063a 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -298,6 +298,8 @@ extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
                         enum gic_sgi_mode irqmode, int virq,
                         const struct sgi_target *target);
 extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
+extern void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
+                                             unsigned int rank, uint32_t r);
 
 #endif /* !CONFIG_NEW_VGIC */
 
-- 
2.17.1

Hello Hongda,

On the previous version, we discussed to include the patch for 4.16. So 
please tag it with for-4.16 and CC the Release Manager (Ian). This will 
help him to track what's outstanding for the release.

On 21/10/2021 13:03, Hongda Deng wrote:
> Currently, Xen will return IO unhandled when guests write ICPENDR*
> virtual registers, which will raise a data abort inside the guest.
> For Linux guest, these virtual registers will not be accessed. But
> for Zephyr, these virtual registers will be accessed during the
> initialization. Zephyr guest will get an IO data abort and crash.
> Emulating ICPENDR is not easy with the existing vGIC, this patch
> reworks the emulation to ignore write access to ICPENDR* virtual
> registers and print a message about whether they are already pending
> instead of returning unhandled.
> More details can be found at [1].
> 
> [1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
> 
> Signed-off-by: Hongda Deng <hongda.deng@arm.com>

While I agree the Reviewed-by from Bertrand should be dropped, the 
Release-acked-by from Ian is simply a way to say he is happy to include 
the patch for 4.16. So this should have been retain.

The patch looks good to me, so I can add Ian's tag on commit:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

On 21/10/2021 16:14, Julien Grall wrote:
> On the previous version, we discussed to include the patch for 4.16. So 
> please tag it with for-4.16 and CC the Release Manager (Ian). This will 
> help him to track what's outstanding for the release.
> 
> On 21/10/2021 13:03, Hongda Deng wrote:
>> Currently, Xen will return IO unhandled when guests write ICPENDR*
>> virtual registers, which will raise a data abort inside the guest.
>> For Linux guest, these virtual registers will not be accessed. But
>> for Zephyr, these virtual registers will be accessed during the
>> initialization. Zephyr guest will get an IO data abort and crash.
>> Emulating ICPENDR is not easy with the existing vGIC, this patch
>> reworks the emulation to ignore write access to ICPENDR* virtual
>> registers and print a message about whether they are already pending
>> instead of returning unhandled.
>> More details can be found at [1].
>>
>> [1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
>> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
>>
>> Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> 
> While I agree the Reviewed-by from Bertrand should be dropped, the 
> Release-acked-by from Ian is simply a way to say he is happy to include 
> the patch for 4.16. So this should have been retain.
> 
> The patch looks good to me, so I can add Ian's tag on commit:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Committed.

Cheers,

-- 
Julien Grall

Hi,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年10月22日 1:16
> To: Hongda Deng <Hongda.Deng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; Ian Jackson <iwj@xenproject.org>
> Subject: Re: [for-4.16] Re: [PATCH v4] xen/arm: vgic: Ignore write access to
> ICPENDR*
> 
> 
> 
> On 21/10/2021 16:14, Julien Grall wrote:
> > On the previous version, we discussed to include the patch for 4.16. So
> > please tag it with for-4.16 and CC the Release Manager (Ian). This will
> > help him to track what's outstanding for the release.
> >
> > On 21/10/2021 13:03, Hongda Deng wrote:
> >> Currently, Xen will return IO unhandled when guests write ICPENDR*
> >> virtual registers, which will raise a data abort inside the guest.
> >> For Linux guest, these virtual registers will not be accessed. But
> >> for Zephyr, these virtual registers will be accessed during the
> >> initialization. Zephyr guest will get an IO data abort and crash.
> >> Emulating ICPENDR is not easy with the existing vGIC, this patch
> >> reworks the emulation to ignore write access to ICPENDR* virtual
> >> registers and print a message about whether they are already pending
> >> instead of returning unhandled.
> >> More details can be found at [1].
> >>
> >> [1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
> >> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
> >>
> >> Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> >
> > While I agree the Reviewed-by from Bertrand should be dropped, the
> > Release-acked-by from Ian is simply a way to say he is happy to include
> > the patch for 4.16. So this should have been retain.
> >
> > The patch looks good to me, so I can add Ian's tag on commit:
> >
> > Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> Committed.
> 
> Cheers,
> 
> --
> Julien Grall

Thank you !

I just learned that I should add "Reviewed-by" and " Release-acked-by" tags based on previous
patches, sorry for that, I will keep it in mind.

Cheers,

---
Hongda