[PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[Jean-Philippe Brucker]

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

Re: [PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[Philippe Mathieu-Daudé]

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);
> 

Re: [PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[Peter Maydell]

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

Re: [PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[Jean-Philippe Brucker]

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);
> > 
> 

Re: [PATCH] hw/intc/arm_gicv3_cpuif: Tolerate spurious EOIR writes

[Shashi Mallela]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

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
>


[-- Attachment #2: Type: text/html, Size: 2444 bytes --]