* [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
@ 2021-09-23 6:14 Hongda Deng
2021-09-23 9:36 ` Julien Grall
0 siblings, 1 reply; 4+ messages in thread
From: Hongda Deng @ 2021-09-23 6:14 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Wei.Chen, Hongda.Deng
Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initilization code, these virtual registers will be accessed. And
zephyr guest will get an IO dataabort in initilization stage and enter
fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].
[1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
msg00744.html
Signed-off-by: Hongda Deng <hongda.deng@arm.com>
---
xen/arch/arm/vgic-v2.c | 10 +++++++---
xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++------------
xen/arch/arm/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/vgic.h | 2 ++
4 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..644c62757c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,14 @@ 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;
+ rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+ if ( rank == NULL ) goto write_ignore;
+
printk(XENLOG_G_ERR
- "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, r, gicd_reg - GICD_ICPENDR);
- return 0;
+ "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d, and current pending state is: %s\n",
+ v, r, gicd_reg - GICD_ICPENDR,
+ vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
+ goto write_ignore_32;
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..c94e33ff4f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,14 @@ 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;
+ rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+ if ( rank == NULL ) goto write_ignore;
+
printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, name, r, reg - GICD_ICPENDR);
- return 0;
+ "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d, and current pending state is: %s\n",
+ v, name, r, reg - GICD_ICPENDR,
+ vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
+ goto write_ignore_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
case VREG32(GICR_ICFGR1):
case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
case VREG32(GICR_ISPENDR0):
- /*
- * Above registers offset are common with GICD.
- * So handle common with GICD handling
- */
+ /*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
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;
+ /*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
+ 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..29a1aa5056 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
}
}
+bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
+{
+ const unsigned long mask = r;
+ unsigned int i;
+ /* The first rank is always per-vCPU */
+ bool private = rank == 0;
+ bool is_pending = false;
+
+ /* LPIs status will never be retrieved via this function */
+ ASSERT(!is_lpi(32 * rank + 31));
+
+ for_each_set_bit( i, &mask, 32 )
+ {
+ unsigned int irq = i + 32 * rank;
+
+ if ( !private )
+ {
+ struct pending_irq *p = spi_to_pending(v->domain, irq);
+
+ if ( p->desc != NULL )
+ {
+ unsigned long flags;
+
+ spin_lock_irqsave(&p->desc->lock, flags);
+ is_pending = gic_read_pending_state(p->desc);
+ spin_unlock_irqrestore(&p->desc->lock, flags);
+ }
+ }
+
+ /* When get pending state, break the loop in time */
+ if ( is_pending )
+ break;
+ }
+
+ return is_pending;
+}
+
bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
int virq, const struct sgi_target *target)
{
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 62c2ae538d..ffc841e5f5 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -290,6 +290,8 @@ extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
extern void vgic_set_irqs_pending(struct vcpu *v, uint32_t r,
unsigned int rank);
+extern bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r,
+ unsigned int rank);
extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
int vgic_v2_init(struct domain *d, int *mmio_count);
int vgic_v3_init(struct domain *d, int *mmio_count);
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
2021-09-23 6:14 [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access Hongda Deng
@ 2021-09-23 9:36 ` Julien Grall
2021-09-23 20:54 ` Stefano Stabellini
0 siblings, 1 reply; 4+ messages in thread
From: Julien Grall @ 2021-09-23 9:36 UTC (permalink / raw)
To: Hongda Deng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 11:14, Hongda Deng wrote:
> Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> registers. This will raise a data abort inside guest. For Linux Guest,
> these virtual registers will not be accessed. But for Zephyr, in its
> GIC initilization code, these virtual registers will be accessed. And
> zephyr guest will get an IO dataabort in initilization stage and enter
s/dataabort/data abort/
s/initilization/initialization/
> fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> we currently ignore these virtual registers access and print a message
> about whether they are already pending instead of returning unhandled.
> More details can be found at [1].
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> msg00744.html
>
> Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> ---
> xen/arch/arm/vgic-v2.c | 10 +++++++---
> xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++------------
> xen/arch/arm/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/vgic.h | 2 ++
> 4 files changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b2da886adc..644c62757c 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -481,10 +481,14 @@ 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;
> + rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
> + if ( rank == NULL ) goto write_ignore;
> +
> printk(XENLOG_G_ERR
> - "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d\n",
> - v, r, gicd_reg - GICD_ICPENDR);
> - return 0;
> + "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d, and current pending state is: %s\n",
> + v, r, gicd_reg - GICD_ICPENDR,
> + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
Each register contain the information for multiple pending interrupts.
So it is a bit confusing to say whether the state is on/off. Instead, it
would be better to state which interrupt is pending.
Also, I would rather avoid printing a message if there are no interrupts
pending because there are no issues if this is happening.
> + goto write_ignore_32;
>
> 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..c94e33ff4f 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -817,10 +817,14 @@ 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;
> + rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> + if ( rank == NULL ) goto write_ignore;
> +
> printk(XENLOG_G_ERR
> - "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
> - v, name, r, reg - GICD_ICPENDR);
> - return 0;
> + "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d, and current pending state is: %s\n",
> + v, name, r, reg - GICD_ICPENDR,
> + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> + goto write_ignore_32;
>
> case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
> case VREG32(GICR_ICFGR1):
> case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
> case VREG32(GICR_ISPENDR0):
> - /*
> - * Above registers offset are common with GICD.
> - * So handle common with GICD handling
> - */
> + /*
> + * Above registers offset are common with GICD.
> + * So handle common with GICD handling
> + */
> return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> 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;
> + /*
> + * Above registers offset are common with GICD.
> + * So handle common with GICD handling
> + */
> + 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..29a1aa5056 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
> }
> }
>
> +bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
> +{
> + const unsigned long mask = r;
> + unsigned int i;
> + /* The first rank is always per-vCPU */
> + bool private = rank == 0;
> + bool is_pending = false;
> +
> + /* LPIs status will never be retrieved via this function */
> + ASSERT(!is_lpi(32 * rank + 31));
> +
> + for_each_set_bit( i, &mask, 32 )
> + {
> + unsigned int irq = i + 32 * rank;
> +
> + if ( !private )
It is not clear to me why you not handling PPIs/SGIs and ...
> + {
> + struct pending_irq *p = spi_to_pending(v->domain, irq);
> +
> + if ( p->desc != NULL )
... emulated SPIs (e.g. PL011).
> + {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&p->desc->lock, flags);
> + is_pending = gic_read_pending_state(p->desc);
> + spin_unlock_irqrestore(&p->desc->lock, flags);
What you are reading here is the pending state from the HW. This is not
the same as the pending state from the VM PoV. In fact, in the most
common case, the interrupt will be pending from the VM PoV, but simply
active from the HW PoV (it is deactivated once the interrupt has been
handled by the guest).
I think what you want to check is whether the flag GIC_IRQ_GUEST_QUEUED
is set in p->status (Stefano ?).
This is technically still a bit racy as Xen may still think the
interrupt is pending while the it may be actually active in the guest.
AFAIK, the other way around (i.e. not pending in Xen but pending in the
guest) cannot happen.
Anyway, this is just a message, so it is still better than crashing :).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
2021-09-23 9:36 ` Julien Grall
@ 2021-09-23 20:54 ` Stefano Stabellini
2021-10-12 6:00 ` Hongda Deng
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Stabellini @ 2021-09-23 20:54 UTC (permalink / raw)
To: Julien Grall
Cc: Hongda Deng, xen-devel, sstabellini, Bertrand.Marquis, Wei.Chen
On Thu, 23 Sep 2021, Julien Grall wrote:
> Hi,
>
> On 23/09/2021 11:14, Hongda Deng wrote:
> > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > registers. This will raise a data abort inside guest. For Linux Guest,
> > these virtual registers will not be accessed. But for Zephyr, in its
> > GIC initilization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO dataabort in initilization stage and enter
>
> s/dataabort/data abort/
> s/initilization/initialization/
>
> > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > we currently ignore these virtual registers access and print a message
> > about whether they are already pending instead of returning unhandled.
> > More details can be found at [1].
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > msg00744.html
> >
> > Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> > ---
> > xen/arch/arm/vgic-v2.c | 10 +++++++---
> > xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++------------
> > xen/arch/arm/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
> > xen/include/asm-arm/vgic.h | 2 ++
> > 4 files changed, 63 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index b2da886adc..644c62757c 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -481,10 +481,14 @@ 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;
> > + rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
> > + if ( rank == NULL ) goto write_ignore;
>
>
>
> > +
> > printk(XENLOG_G_ERR
> > - "%pv: vGICD: unhandled word write %#"PRIregister" to
> > ICPENDR%d\n",
> > - v, r, gicd_reg - GICD_ICPENDR);
> > - return 0;
> > + "%pv: vGICD: unhandled word write %#"PRIregister" to
> > ICPENDR%d, and current pending state is: %s\n",
> > + v, r, gicd_reg - GICD_ICPENDR,
> > + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
>
> Each register contain the information for multiple pending interrupts. So it
> is a bit confusing to say whether the state is on/off. Instead, it would be
> better to state which interrupt is pending.
>
> Also, I would rather avoid printing a message if there are no interrupts
> pending because there are no issues if this is happening.
>
> > + goto write_ignore_32;
> > 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..c94e33ff4f 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -817,10 +817,14 @@ 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;
> > + rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> > + if ( rank == NULL ) goto write_ignore;
> > +
> > printk(XENLOG_G_ERR
> > - "%pv: %s: unhandled word write %#"PRIregister" to
> > ICPENDR%d\n",
> > - v, name, r, reg - GICD_ICPENDR);
> > - return 0;
> > + "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d,
> > and current pending state is: %s\n",
> > + v, name, r, reg - GICD_ICPENDR,
> > + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> > + goto write_ignore_32;
> > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > if ( dabt.size != DABT_WORD ) goto bad_width;
> > @@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu
> > *v, mmio_info_t *info,
> > case VREG32(GICR_ICFGR1):
> > case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
> > case VREG32(GICR_ISPENDR0):
> > - /*
> > - * Above registers offset are common with GICD.
> > - * So handle common with GICD handling
> > - */
> > + /*
> > + * Above registers offset are common with GICD.
> > + * So handle common with GICD handling
> > + */
> > return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> > 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;
> > + /*
> > + * Above registers offset are common with GICD.
> > + * So handle common with GICD handling
> > + */
> > + 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..29a1aa5056 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t r,
> > unsigned int rank)
> > }
> > }
> > +bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
> > +{
> > + const unsigned long mask = r;
> > + unsigned int i;
> > + /* The first rank is always per-vCPU */
> > + bool private = rank == 0;
> > + bool is_pending = false;
> > +
> > + /* LPIs status will never be retrieved via this function */
> > + ASSERT(!is_lpi(32 * rank + 31));
> > +
> > + for_each_set_bit( i, &mask, 32 )
> > + {
> > + unsigned int irq = i + 32 * rank;
> > +
> > + if ( !private )
>
> It is not clear to me why you not handling PPIs/SGIs and ...
>
> > + {
> > + struct pending_irq *p = spi_to_pending(v->domain, irq);
> > +
> > + if ( p->desc != NULL )
>
> ... emulated SPIs (e.g. PL011).
>
> > + {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&p->desc->lock, flags);
> > + is_pending = gic_read_pending_state(p->desc);
> > + spin_unlock_irqrestore(&p->desc->lock, flags);
>
> What you are reading here is the pending state from the HW. This is not the
> same as the pending state from the VM PoV. In fact, in the most common case,
> the interrupt will be pending from the VM PoV, but simply active from the HW
> PoV (it is deactivated once the interrupt has been handled by the guest).
>
> I think what you want to check is whether the flag GIC_IRQ_GUEST_QUEUED is set
> in p->status (Stefano ?).
Yeah, that's right. In fact, there is no need for checking the hardware
registers. You can just go through the inflight_irqs list and print all
of them (the list is sync on hyp entry on the cpu you are running on,
not the others of course).
> This is technically still a bit racy as Xen may still think the interrupt is
> pending while the it may be actually active in the guest. AFAIK, the other way
> around (i.e. not pending in Xen but pending in the guest) cannot happen.
>
> Anyway, this is just a message, so it is still better than crashing :).
+1
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
2021-09-23 20:54 ` Stefano Stabellini
@ 2021-10-12 6:00 ` Hongda Deng
0 siblings, 0 replies; 4+ messages in thread
From: Hongda Deng @ 2021-10-12 6:00 UTC (permalink / raw)
To: Stefano Stabellini, Julien Grall; +Cc: xen-devel, Bertrand Marquis, Wei Chen
Hi,
Thanks for your great and detailed advice, I did some investigations about vgic especially inflight_irqs in the last few days.
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: 2021年9月24日 4:54
> To: Julien Grall <julien@xen.org>
> Cc: Hongda Deng <Hongda.Deng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei
> Chen <Wei.Chen@arm.com>
> Subject: Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
>
> On Thu, 23 Sep 2021, Julien Grall wrote:
> > Hi,
> >
> > On 23/09/2021 11:14, Hongda Deng wrote:
> > > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > > registers. This will raise a data abort inside guest. For Linux Guest,
> > > these virtual registers will not be accessed. But for Zephyr, in its
> > > GIC initilization code, these virtual registers will be accessed. And
> > > zephyr guest will get an IO dataabort in initilization stage and enter
> >
> > s/dataabort/data abort/
> > s/initilization/initialization/
> >
Ack.
> > > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > > we currently ignore these virtual registers access and print a message
> > > about whether they are already pending instead of returning unhandled.
> > > More details can be found at [1].
> > >
> > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > > msg00744.html
> > >
> > > Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> > > ---
> > > xen/arch/arm/vgic-v2.c | 10 +++++++---
> > > xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++------------
> > > xen/arch/arm/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
> > > xen/include/asm-arm/vgic.h | 2 ++
> > > 4 files changed, 63 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > index b2da886adc..644c62757c 100644
> > > --- a/xen/arch/arm/vgic-v2.c
> > > +++ b/xen/arch/arm/vgic-v2.c
> > > @@ -481,10 +481,14 @@ 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;
> > > + rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
> > > + if ( rank == NULL ) goto write_ignore;
> >
> >
> >
> > > +
> > > printk(XENLOG_G_ERR
> > > - "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > - v, r, gicd_reg - GICD_ICPENDR);
> > > - return 0;
> > > + "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d, and current pending state is: %s\n",
> > > + v, r, gicd_reg - GICD_ICPENDR,
> > > + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> >
> > Each register contain the information for multiple pending interrupts. So it
> > is a bit confusing to say whether the state is on/off. Instead, it would be
> > better to state which interrupt is pending.
> >
> > Also, I would rather avoid printing a message if there are no interrupts
> > pending because there are no issues if this is happening.
I will fix it in the next version patch.
> >
> > > + goto write_ignore_32;
> > > 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..c94e33ff4f 100644
> > > --- a/xen/arch/arm/vgic-v3.c
> > > +++ b/xen/arch/arm/vgic-v3.c
> > > @@ -817,10 +817,14 @@ 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;
> > > + rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> > > + if ( rank == NULL ) goto write_ignore;
> > > +
> > > printk(XENLOG_G_ERR
> > > - "%pv: %s: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > - v, name, r, reg - GICD_ICPENDR);
> > > - return 0;
> > > + "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d,
> > > and current pending state is: %s\n",
> > > + v, name, r, reg - GICD_ICPENDR,
> > > + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> > > + goto write_ignore_32;
> > > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > > if ( dabt.size != DABT_WORD ) goto bad_width;
> > > @@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct
> vcpu
> > > *v, mmio_info_t *info,
> > > case VREG32(GICR_ICFGR1):
> > > case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
> > > case VREG32(GICR_ISPENDR0):
> > > - /*
> > > - * Above registers offset are common with GICD.
> > > - * So handle common with GICD handling
> > > - */
> > > + /*
> > > + * Above registers offset are common with GICD.
> > > + * So handle common with GICD handling
> > > + */
> > > return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> > > 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;
> > > + /*
> > > + * Above registers offset are common with GICD.
> > > + * So handle common with GICD handling
> > > + */
> > > + 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..29a1aa5056 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t
> r,
> > > unsigned int rank)
> > > }
> > > }
> > > +bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
> > > +{
> > > + const unsigned long mask = r;
> > > + unsigned int i;
> > > + /* The first rank is always per-vCPU */
> > > + bool private = rank == 0;
> > > + bool is_pending = false;
> > > +
> > > + /* LPIs status will never be retrieved via this function */
> > > + ASSERT(!is_lpi(32 * rank + 31));
> > > +
> > > + for_each_set_bit( i, &mask, 32 )
> > > + {
> > > + unsigned int irq = i + 32 * rank;
> > > +
> > > + if ( !private )
> >
> > It is not clear to me why you not handling PPIs/SGIs and ...
> >
> > > + {
> > > + struct pending_irq *p = spi_to_pending(v->domain, irq);
> > > +
> > > + if ( p->desc != NULL )
> >
> > ... emulated SPIs (e.g. PL011).
> >
> > > + {
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&p->desc->lock, flags);
> > > + is_pending = gic_read_pending_state(p->desc);
> > > + spin_unlock_irqrestore(&p->desc->lock, flags);
> >
> > What you are reading here is the pending state from the HW. This is not the
> > same as the pending state from the VM PoV. In fact, in the most common case,
> > the interrupt will be pending from the VM PoV, but simply active from the HW
> > PoV (it is deactivated once the interrupt has been handled by the guest).
> >
> > I think what you want to check is whether the flag GIC_IRQ_GUEST_QUEUED
> is set
> > in p->status (Stefano ?).
>
> Yeah, that's right. In fact, there is no need for checking the hardware
> registers. You can just go through the inflight_irqs list and print all
> of them (the list is sync on hyp entry on the cpu you are running on,
> not the others of course).
>
>
> > This is technically still a bit racy as Xen may still think the interrupt is
> > pending while the it may be actually active in the guest. AFAIK, the other way
> > around (i.e. not pending in Xen but pending in the guest) cannot happen.
> >
> > Anyway, this is just a message, so it is still better than crashing :).
>
> +1
Thanks again for your advice.
Based on that, I wrote a new patch to go through vcpu->arch.vgic.inflight_irqs to check the pending
states and print them if there are in the next version patch. I will send it for review later.
Cheers,
Hongda
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-12 6:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23 6:14 [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access Hongda Deng
2021-09-23 9:36 ` Julien Grall
2021-09-23 20:54 ` Stefano Stabellini
2021-10-12 6:00 ` Hongda Deng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.