xen/arch/arm/vgic-v2.c | 10 ++++++---- xen/arch/arm/vgic-v3.c | 16 ++++++++-------- xen/arch/arm/vgic.c | 36 ++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/vgic.h | 3 ++- 4 files changed, 52 insertions(+), 13 deletions(-)
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 initialization code, these virtual registers will be accessed. And
zephyr guest will get an IO data abort 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://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 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 | 16 ++++++++--------
xen/arch/arm/vgic.c | 36 ++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/vgic.h | 3 ++-
4 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..7c30da327c 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_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..4913301d22 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_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
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..0565557814 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -726,6 +726,42 @@ 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;
+ unsigned long flags;
+ struct pending_irq *p;
+ bool private = rank == 0;
+ struct vcpu *v_target;
+
+ for_each_set_bit( i, &mask, 32 )
+ {
+ unsigned int irq = i + 32 * rank;
+
+ if ( private )
+ v_target = vgic_get_target_vcpu(v, irq);
+ else
+ v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
+
+ spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+
+ p = irq_to_pending(v_target, irq);
+
+ if ( unlikely(!p) )
+ {
+ spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+ continue;
+ }
+
+ if ( !list_empty(&p->inflight) )
+ printk("%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..abcaae2969 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -298,7 +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 */
/*** Common VGIC functions used by Xen arch code ****/
--
2.17.1
Hi Hongda,
[+Ian]
> On 20 Oct 2021, at 11:10, Hongda Deng <Hongda.Deng@arm.com> 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 initialization code, these virtual registers will be accessed. And
> zephyr guest will get an IO data abort 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://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
>
> Signed-off-by: Hongda Deng <hongda.deng@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Ian this is fixing a bug in the vgic implementation which is preventing to run
Zephyr as a guest on top of Xen. Xen support has now been merged in Zephyr
so this is required to use it.
Could we consider adding this patch into the 4.16 release ?
Thanks
Bertrand
> ---
> 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 | 16 ++++++++--------
> xen/arch/arm/vgic.c | 36 ++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/vgic.h | 3 ++-
> 4 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b2da886adc..7c30da327c 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_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..4913301d22 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_32;
>
> case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
>
> 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..0565557814 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -726,6 +726,42 @@ 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;
> + unsigned long flags;
> + struct pending_irq *p;
> + bool private = rank == 0;
> + struct vcpu *v_target;
> +
> + for_each_set_bit( i, &mask, 32 )
> + {
> + unsigned int irq = i + 32 * rank;
> +
> + if ( private )
> + v_target = vgic_get_target_vcpu(v, irq);
> + else
> + v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
> +
> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> +
> + p = irq_to_pending(v_target, irq);
> +
> + if ( unlikely(!p) )
> + {
> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> + continue;
> + }
> +
> + if ( !list_empty(&p->inflight) )
> + printk("%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..abcaae2969 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -298,7 +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 */
>
> /*** Common VGIC functions used by Xen arch code ****/
> --
> 2.17.1
>
~Bertrand Marquis writes ("Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access"):
> [+Ian]
> > On 20 Oct 2021, at 11:10, Hongda Deng <Hongda.Deng@arm.com> 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 initialization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO data abort 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://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
> > cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
> >
> > Signed-off-by: Hongda Deng <hongda.deng@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Ian this is fixing a bug in the vgic implementation which is preventing to run
> Zephyr as a guest on top of Xen. Xen support has now been merged in Zephyr
> so this is required to use it.
>
> Could we consider adding this patch into the 4.16 release ?
Hi. I'm definitely open to the idea, especially if it goes in soon.
I think this needs an ARM maintainer review, still ?
It doesn't seem entirely straightforward. I'd like to hear from the
maintainer, to confirm that they agree it's a bugfix, and to get an
idea of the risks vs benefits of this patch.
Thanks,
Ian.
Hi Ian,
On 20/10/2021 14:30, Ian Jackson wrote:
> ~Bertrand Marquis writes ("Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access"):
>> [+Ian]
>>> On 20 Oct 2021, at 11:10, Hongda Deng <Hongda.Deng@arm.com> 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 initialization code, these virtual registers will be accessed. And
>>> zephyr guest will get an IO data abort 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://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
>>> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
>>>
>>> Signed-off-by: Hongda Deng <hongda.deng@arm.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Ian this is fixing a bug in the vgic implementation which is preventing to run
>> Zephyr as a guest on top of Xen. Xen support has now been merged in Zephyr
>> so this is required to use it.
>>
>> Could we consider adding this patch into the 4.16 release ?
>
> Hi. I'm definitely open to the idea, especially if it goes in soon.
>
> I think this needs an ARM maintainer review, still ?
Yes. I am planning to review it later today.
>
> It doesn't seem entirely straightforward. I'd like to hear from the
> maintainer, to confirm that they agree it's a bugfix, and to get an
> idea of the risks vs benefits of this patch.
TL;DR: This is a bug fix and I agree that this should be included in 4.16.
ICPENDRn are a mandatory registers of the GIC implementation. But it is
not trivial to emulate properly with our existing vGIC. So for the past
years, we chose the lazy approach and inject a data abort when the vCPU
access it.
IOW, this is not a new bug fix. We haven't seen any problem before
because most of our users were using Linux based guests. Now this is
starting to change and therefore we are exercising paths that Linux
never used it. In this case, we would not be able to boot Zephyr on Xen.
During boot, Zephyr will write to ICPENDR to clear all the pending
interrupts. I am not entirely convinced that from Zephyr PoV this is a
useful things to do because, unless you quiesced the devices, interrupts
can become pending again right away after clearing.
I would suggest to chat with the Zephyr folks to understand why they
need to write to ICPENDR during boot.
In any case, I am assuming there are already Zephyr OS out there. So we
need to solve the issue in Xen.
This patch doesn't fully emulate ICPENDR. The new appropach will ignore
access and print a message when the OS is trying to clear a pending
interrupt.
The code itself is only walking the internal structure. So as long as
the correct locks are held, there is no change in the emulated state.
The only difference will happen at the domain level. Now, the domain
will be able to continue booting. We will not clear pending interrupts
but I think this is an acceptable approach as the worst that can happen
is the guest may receive a "spurious" interrupt.
In both cases, I think the risks are limited and I would support the
inclusion of this patch (pending appropriate acks) in 4.16.
Cheers,
--
Julien Grall
Julien Grall writes ("Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access"):
> TL;DR: This is a bug fix and I agree that this should be included in 4.16.
Thank you very much for the detailed and helpful reply.
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Hi Hongda,
Title: I would suggest the following title:
xen/arm: vgic: Ignore write access to ICPENDR*
On 20/10/2021 11:10, 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 initialization code, these virtual registers will be accessed. And
> zephyr guest will get an IO data abort in initilization stage and enter
Typo: s/initilization/initialization/
I would also s/in/during the/
> fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
How about s/enter fatal error/crash/?
> we currently ignore these virtual registers access and print a message
To me 'currently' refers to the existing code base (i.e. without your
patch). In fact, this seems to be how you use 'currently' in the first
paragraph. So how about replace "so we currently" with "rework the
emulation to ignore...".
This seems to suggest the patch will modify both read and write access.
However, AFAICT, only the write emulation is modified. Can this be
clarified in the commit 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 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 | 16 ++++++++--------
> xen/arch/arm/vgic.c | 36 ++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/vgic.h | 3 ++-
> 4 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b2da886adc..7c30da327c 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_32;
NIT: We already check the access above. So I would simply use
"write_ignore" here.
>
> 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..4913301d22 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_32;
NIT: Same remark as the previous write_ignore_32.
>
> case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
>
> 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..0565557814 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -726,6 +726,42 @@ 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;
> + unsigned long flags;
> + struct pending_irq *p;
> + bool private = rank == 0;
> + struct vcpu *v_target;
AFAIC, flags, p, v_target are only used within the loop. So please
reduce the scope and only declare them in for_each_set_bit().
> +
> + for_each_set_bit( i, &mask, 32 )
> + {
> + unsigned int irq = i + 32 * rank;
> +
> + if ( private )
> + v_target = vgic_get_target_vcpu(v, irq);
> + else
> + v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
Shared interrupts can be accessed from any vCPU. So you can replace the
4 lines with:
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 ( unlikely(!p) )
> + {
> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> + continue;
> + }
irq_to_pending() cannot return NULL for non-LPI interrupts. But if you
still want to check it, then the two if can be combined to something like:
if ( p && !list_empty(&p->inflight) )
printk(...)
spin_unlock_irqrestore(...);
> +
> + if ( !list_empty(&p->inflight) )
> + printk("%pv trying to clear pending interrupt %u.\n", v, irq);
This wants to be a printk(XENLOG_G_WARNING ...) so the message will be
appropriately rate-limited.
> +
> + 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..abcaae2969 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -298,7 +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);
Please keep the newline before the #endif.
> #endif /* !CONFIG_NEW_VGIC */
>
> /*** Common VGIC functions used by Xen arch code ****/
>
Cheers,
--
Julien Grall
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年10月21日 1:45
> 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>
> Subject: Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers
> access
>
> Hi Hongda,
>
> Title: I would suggest the following title:
>
> xen/arm: vgic: Ignore write access to ICPENDR*
>
Agreed.
> On 20/10/2021 11:10, 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 initialization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO data abort in initilization stage and enter
> Typo: s/initilization/initialization/
>
> I would also s/in/during the/
>
> > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
>
> How about s/enter fatal error/crash/?
>
Agreed.
> > we currently ignore these virtual registers access and print a message
>
> To me 'currently' refers to the existing code base (i.e. without your
> patch). In fact, this seems to be how you use 'currently' in the first
> paragraph. So how about replace "so we currently" with "rework the
> emulation to ignore...".
>
> This seems to suggest the patch will modify both read and write access.
> However, AFAICT, only the write emulation is modified. Can this be
> clarified in the commit message?
>
Ack.
> > 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 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 | 16 ++++++++--------
> > xen/arch/arm/vgic.c | 36
> ++++++++++++++++++++++++++++++++++++
> > xen/include/asm-arm/vgic.h | 3 ++-
> > 4 files changed, 52 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index b2da886adc..7c30da327c 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_32;
>
> NIT: We already check the access above. So I would simply use
> "write_ignore" here.
>
Ack.
> >
> > 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..4913301d22 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_32;
>
> NIT: Same remark as the previous write_ignore_32.
>
Ack.
> >
> > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > if ( dabt.size != DABT_WORD ) goto bad_width;
> > @@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct
> vcpu *v, mmio_info_t *info,
> >
> > 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..0565557814 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -726,6 +726,42 @@ 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;
> > + unsigned long flags;
> > + struct pending_irq *p;
> > + bool private = rank == 0;
> > + struct vcpu *v_target;
>
> AFAIC, flags, p, v_target are only used within the loop. So please
> reduce the scope and only declare them in for_each_set_bit().
>
Ack.
> > +
> > + for_each_set_bit( i, &mask, 32 )
> > + {
> > + unsigned int irq = i + 32 * rank;
> > +
> > + if ( private )
> > + v_target = vgic_get_target_vcpu(v, irq);
> > + else
> > + v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
>
> Shared interrupts can be accessed from any vCPU. So you can replace the
> 4 lines with:
> v_target = vgic_get_target_vcpu(v, irq);
>
Ack.
I thought that v may be NULL, obviously I was overthinking about it.
> > +
> > + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> > +
> > + p = irq_to_pending(v_target, irq);
> > +
> > + if ( unlikely(!p) )
> > + {
> > + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> > + continue;
> > + }
>
> irq_to_pending() cannot return NULL for non-LPI interrupts. But if you
> still want to check it, then the two if can be combined to something like:
>
> if ( p && !list_empty(&p->inflight) )
> printk(...)
>
> spin_unlock_irqrestore(...);
>
Ack.
> > +
> > + if ( !list_empty(&p->inflight) )
> > + printk("%pv trying to clear pending interrupt %u.\n", v, irq);
>
> This wants to be a printk(XENLOG_G_WARNING ...) so the message will be
> appropriately rate-limited.
>
Ack.
> > +
> > + 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..abcaae2969 100644
> > --- a/xen/include/asm-arm/vgic.h
> > +++ b/xen/include/asm-arm/vgic.h
> > @@ -298,7 +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);
>
> Please keep the newline before the #endif.
>
Ack.
> > #endif /* !CONFIG_NEW_VGIC */
> >
> > /*** Common VGIC functions used by Xen arch code ****/
> >
>
> Cheers,
>
> --
> Julien Grall
Thanks for your detailed suggestion ! I will keep these code principles in mind.
I will send patch version4 ASAP.
Cheers,
---
Hongda
© 2016 - 2024 Red Hat, Inc.