Re: [PATCH] arm_gic: Implement GICC_AIAR, GICC_AEOIR and GICC_AHPPIR

[Peter Maydell]

On Wed, 20 Dec 2023 at 06:47, Andrei Homescu <ah@immunant.com> wrote:
>
> From: Arve Hjønnevåg <arve@android.com>
>
> Implement aliased registers so group 1 interrupts can be used in secure
> mode.

Hi; thanks for this patch.

> GICC_AEOIR is only implemented as a direct alias to GICC_EOIR for now as
> gic_complete_irq does not currently check if the cpu is in secure mode.

I'm not really sure what this comment has in mind, but I think
perhaps it is alluding to the issue that https://r.android.com/859189
fixes ? In any case, we should either expand it to be clearer about
what problem it's referring to or else delete it as no longer
relevant.

> Upstreamed from https://r.android.com/705890 and
> https://r.android.com/859189.
>
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>

Hmm, I'm not entirely sure if we should be creating signed-off-by
lines if they've not been provided by the original authors.
(We can take the code because it's from a GPL2'd fork, and we
should credit the original authors, but a signed-off-by tag
says a bit more than that.)

> Signed-off-by: Andrei Homescu <ah@immunant.com>
> ---
>  hw/intc/arm_gic.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 074cf50af2..d0e267a4b2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -854,7 +854,8 @@ static void gic_deactivate_irq(GICState *s, int cpu, int 
> irq, MemTxAttrs attrs)
>      gic_clear_active(s, irq, cpu);
>  }
>
> -static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
> +static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs,
> +                             bool ns_irq)

I think this argument should be called group1_only (see later).

>  {
>      int cm = 1 << cpu;
>      int group;

There's a codepath here for "gic_is_vcpu())"; we should
add a comment somewhere there that says something like:

 /*
  * Acknowledging a group 0 interrupt via GICV_AEOIR is
  * UNPREDICTABLE; we choose to treat it as if the guest
  * had acknowledged it via GICV_EOIR, i.e. we ignore
  * the group1_only flag.
  */

> @@ -922,7 +923,7 @@ static void gic_complete_irq(GICState *s, int cpu, int 
> irq, MemTxAttrs attrs)
>
>      group = gic_has_groups(s) && gic_test_group(s, irq, cpu);
>
> -    if (gic_cpu_ns_access(s, cpu, attrs) && !group) {
> +    if (ns_irq && !group) {
>          DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
>          return;
>      }

> @@ -1647,6 +1648,22 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, 
> int offset,
>              *data = s->abpr[cpu];
>          }
>          break;
> +    case 0x20: /* Aliased Interrupt Acknowledge */
> +        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {

This doesn't look like the right condition -- the spec
says GICC_AIAR is only present in GICv2. gic_has_groups()
will return true for GICv2 or for a GICv1 with the security
extensions. (That's the right check for GICC_ABPR, but not
for GICC_AIAR or GICC_AHPPIR or GIC_AEOIR.)

It will also mean we incorrectly return 0 for the case
of the GICV_AIAR vcpu interface register. (This is why the
existing code for the GICC_ABPR uses gic_cpu_ns_access() in
this part of its check.)

> +            *data = 0;
> +        } else {
> +            attrs.secure = false;
> +            *data = gic_acknowledge_irq(s, cpu, attrs);

This doesn't do the right thing I think for the GICV_AIAR
vcpu interface, or for a GICv2 which doesn't have the security
extensions, because gic_cpu_ns_access() will return false
for both of those cases even if attrs.secure is false, and
so when gic_acknowledge_irq() calls gic_get_current_pending_irq()
it won't get the group 1 IRQ it's supposed to.

> +        }
> +        break;
> +    case 0x28: /* Aliased Highest Priority Pending Interrupt */
> +        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
> +            *data = 0;
> +        } else {
> +            attrs.secure = false;
> +            *data = gic_get_current_pending_irq(s, cpu, attrs);
> +        }

Similarly this isn't correct for the GICv2-no-security-interface
and the GICV-vcpu-interface cases.

I think we probably need to plumb through more of the "what
behaviour do we need" rather than trying to do it by setting
attrs.secure. My first thought is a 'bool group1_only' argument
to gic_get_current_pending_irq() and gic_acknowledge_irq(),
so that those functions can give the behaviour we want for
the alias registers.

> +        break;
>      case 0xd0: case 0xd4: case 0xd8: case 0xdc:
>      {
>          int regno = (offset - 0xd0) / 4;
> @@ -1724,7 +1741,15 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, 
> int offset,
>          }
>          break;
>      case 0x10: /* End Of Interrupt */
> -        gic_complete_irq(s, cpu, value & 0x3ff, attrs);
> +        gic_complete_irq(s, cpu, value & 0x3ff, attrs,
> +                         gic_cpu_ns_access(s, cpu, attrs));
> +        return MEMTX_OK;
> +    case 0x24: /* Aliased End Of Interrupt */
> +        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
> +            /* unimplemented, or NS access: RAZ/WI */

Same remark applies here about the condition to use.

> +        } else {
> +            gic_complete_irq(s, cpu, value & 0x3ff, attrs, true);

Here we are passing through the equivalent of a
"group1_only" argument, so my suggestion above pretty
much is to bring the AHPPIR and AIAR into line with this.

> +        }

>          return MEMTX_OK;
>      case 0x1c: /* Aliased Binary Point */
>          if (!gic_has_groups(s) || (gic_cpu_ns_access(s, cpu, attrs))) {
> --
> 2.42.1

thanks
-- PMM