Re: [Qemu-arm] [PATCH v4 12/20] intc/arm_gic: Implement virtualization extensions in gic_(deactivate|complete_irq)

[Peter Maydell]

On 14 July 2018 at 18:15, Luc Michel <address@hidden> wrote:
> Implement virtualization extensions in the gic_deactivate_irq() and
> gic_complete_irq() functions.  When a guest tries to deactivat or end an

"deactivate"

> IRQ that does not exist in the LRs, the EOICount field of the virtual
> interface HCR register is incremented by one, and the request is
> ignored.
>
> Signed-off-by: Luc Michel <address@hidden>
> ---
>  hw/intc/arm_gic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index be9e2594d9..50cbbfbe24 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -590,6 +590,15 @@ static void gic_deactivate_irq(GICState *s, int cpu, int 
> irq, MemTxAttrs attrs)
>          return;
>      }
>
> +    if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
> +        /* This vIRQ does not have an LR entry which is either active or
> +         * pending and active. Increment EOICount and ignore the write.
> +         */
> +        int rcpu = gic_get_vcpu_real_id(cpu);
> +        s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
> +        return;
> +    }

It's a bit hard to tell from the amount of context in the diff,
but I think this check is being done too late in this function.
It needs to happen before we do any operations on the irq we're
passed (eg checking which group it is).

> +
>      if (gic_cpu_ns_access(s, cpu, attrs) && !group) {
>          DPRINTF("Non-secure DI for Group0 interrupt %d ignored\n", irq);
>          return;
> @@ -604,6 +613,15 @@ static void gic_complete_irq(GICState *s, int cpu, int 
> irq, MemTxAttrs attrs)
>      int group;
>
>      DPRINTF("EOI %d\n", irq);
> +    if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
> +        /* This vIRQ does not have an LR entry which is either active or
> +         * pending and active. Increment EOICount and ignore the write.
> +         */
> +        int rcpu = gic_get_vcpu_real_id(cpu);
> +        s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
> +        return;
> +    }

This isn't consistent with the deactivate code about whether we
do this check before the "irq >= s->num_irq" check or after.

I think the correct answer is "before", because the number of
physical interrupts in the GIC shouldn't affect the valid
range of virtual interrupts.

There is also an edge case here: if the VIRQ written to the
EOI or DIR registers is a special interrupt number (1020..1023),
then should we increment the EOI count or not? The GICv2 spec
is not entirely clear on this point, but it does say in the
GICH_HCR.EOICount docs that "EOIs that do not clear a bit in
the Active Priorities register GICH_APR do not cause an increment",
and the GICv3 spec is clear so my recommendation is that for
1020..1023 we should ignore the write and not increment EOICount.

That bit about "EOIs that do not clear a bit in GICH_APR do
not increment EOICount" also suggests that our logic for DIR
and EOI needs to be something like:

  if (vcpu) {
      if (irq > 1020) {
          return;
      }
      clear GICH_HCR bit;
      if (no bit cleared) {
          return;
      }
      if (!gic_virq_is_valid()) {
          increment EOICount;
          return;
      }
      clear active bit in LR;
  }

?

> +
>      if (irq >= s->num_irq) {
>          /* This handles two cases:
>           * 1. If software writes the ID of a spurious interrupt [ie 1023]
> --
> 2.18.0
>

thanks
-- PMM