| Message ID | 20210603110012.1182530-1-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes | expand |
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); >
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
On Thu, Jun 03, 2021 at 02:39:30PM +0200, Philippe Mathieu-Daudé wrote: > > 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? No problem. I had it at first, but then wondered if that meant I should update similar cases in the GIC device that silently ignore guest errors at the moment (e.g. the guest writes a number > 1023 to EOIR) and decided against it. I'll resend with only this error report if there is no objection. Thanks, Jean > > + return; > > } > > > > icc_drop_prio(cs, grp); > > >
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 >
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);
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(-)