hw/intc/arm_gicv3_cpuif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Commit 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access
check logic") added an assert_not_reached() if the guest writes the EOIR
register while no interrupt is active.
It turns out some software does this: EDK2, in GicV3ExitBootServicesEvent(),
unconditionally write EOIR for all interrupts that it manages. This now
causes QEMU to abort when running UEFI on a VM with GICv3. Although it
is UNPREDICTABLE behavior and EDK2 does need fixing, the punishment
seems a little harsh, especially since icc_eoir_write() already
tolerates writes of nonexistent interrupt numbers. Simply ignore
spurious EOIR writes.
Fixes: 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
hw/intc/arm_gicv3_cpuif.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 81f94c7f4a..1d0964c9bf 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1357,7 +1357,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
}
break;
default:
- g_assert_not_reached();
+ /* No interrupt was active, this is UNPREDICTABLE. Ignore it. */
+ return;
}
icc_drop_prio(cs, grp);
--
2.31.1
On Thu, 3 Jun 2021 at 12:01, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > > Commit 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access > check logic") added an assert_not_reached() if the guest writes the EOIR > register while no interrupt is active. > > It turns out some software does this: EDK2, in GicV3ExitBootServicesEvent(), > unconditionally write EOIR for all interrupts that it manages. This now > causes QEMU to abort when running UEFI on a VM with GICv3. Although it > is UNPREDICTABLE behavior and EDK2 does need fixing, the punishment > seems a little harsh, especially since icc_eoir_write() already > tolerates writes of nonexistent interrupt numbers. Simply ignore > spurious EOIR writes. > > Fixes: 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic") > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > hw/intc/arm_gicv3_cpuif.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..1d0964c9bf 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -1357,7 +1357,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > } > break; > default: > - g_assert_not_reached(); > + /* No interrupt was active, this is UNPREDICTABLE. Ignore it. */ > + return; > Makes sense (and looking at the comment in icc_highest_active_group() that says "return -1 so the caller ignores the spurious EOI attempt" it is what the code originally intended). Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Shashi, I guess this also fixes the assert you were seeing here ? thanks -- PMM
Yes it does. Thanks Shashi On Jun 3 2021, at 8:56 am, Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 3 Jun 2021 at 12:01, Jean-Philippe Brucker > <jean-philippe@linaro.org> wrote: > > > > Commit 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access > > check logic") added an assert_not_reached() if the guest writes the EOIR > > register while no interrupt is active. > > > > It turns out some software does this: EDK2, in GicV3ExitBootServicesEvent(), > > unconditionally write EOIR for all interrupts that it manages. This now > > causes QEMU to abort when running UEFI on a VM with GICv3. Although it > > is UNPREDICTABLE behavior and EDK2 does need fixing, the punishment > > seems a little harsh, especially since icc_eoir_write() already > > tolerates writes of nonexistent interrupt numbers. Simply ignore > > spurious EOIR writes. > > > > Fixes: 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic") > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > --- > > hw/intc/arm_gicv3_cpuif.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > > index 81f94c7f4a..1d0964c9bf 100644 > > --- a/hw/intc/arm_gicv3_cpuif.c > > +++ b/hw/intc/arm_gicv3_cpuif.c > > @@ -1357,7 +1357,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > > } > > break; > > default: > > - g_assert_not_reached(); > > + /* No interrupt was active, this is UNPREDICTABLE. Ignore it. */ > > + return; > > > > Makes sense (and looking at the comment in icc_highest_active_group() > that says "return -1 so the caller ignores the spurious EOI attempt" > it is what the code originally intended). > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Shashi, I guess this also fixes the assert you were seeing here ? > thanks > -- PMM >
Hi Jean-Philippe, On 6/3/21 1:00 PM, Jean-Philippe Brucker wrote: > Commit 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access > check logic") added an assert_not_reached() if the guest writes the EOIR > register while no interrupt is active. > > It turns out some software does this: EDK2, in GicV3ExitBootServicesEvent(), > unconditionally write EOIR for all interrupts that it manages. This now > causes QEMU to abort when running UEFI on a VM with GICv3. Although it > is UNPREDICTABLE behavior and EDK2 does need fixing, the punishment > seems a little harsh, especially since icc_eoir_write() already > tolerates writes of nonexistent interrupt numbers. Simply ignore > spurious EOIR writes. > > Fixes: 382c7160d1cd ("hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic") > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > hw/intc/arm_gicv3_cpuif.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..1d0964c9bf 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -1357,7 +1357,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > } > break; > default: > - g_assert_not_reached(); > + /* No interrupt was active, this is UNPREDICTABLE. Ignore it. */ A qemu_log_mask(LOG_GUEST_ERROR, ...) call here could be useful, do you mind adding it? > + return; > } > > icc_drop_prio(cs, grp); >
© 2016 - 2024 Red Hat, Inc.