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