Re: [Qemu-devel] [PATCH v3 09/20] intc/arm_gic: Add virtualization enabled IRQ helper functions

[Peter Maydell <peter.maydell@linaro.org>]

On 29 June 2018 at 14:29, Luc Michel <luc.michel@greensocs.com> wrote:
> Add some helper functions to gic_internal.h to get or change the state
> of an IRQ. When the current CPU is not a vCPU, the call is forwarded to
> the GIC distributor. Otherwise, it acts on the list register matching
> the IRQ in the current CPU virtual interface.
>
> gic_clear_active can have a side effect on the distributor, even in the
> vCPU case, when the correponding LR has the HW field set.
>
> Use those functions in the CPU interface code path to prepare for the
> vCPU interface implementation.
>
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>

My review remarks on patch 7 will affect this patch a bit but
generally this looks good.

> +static inline void gic_clear_active(GICState *s, int irq, int cpu)
> +{
> +    if (gic_is_vcpu(cpu)) {
> +        uint32_t *entry = gic_get_lr_entry_nofail(s, irq, cpu);
> +        GICH_LR_CLEAR_ACTIVE(*entry);
> +
> +        if (GICH_LR_HW(*entry)) {
> +            /* Hardware interrupt. We must forward the deactivation request to
> +             * the distributor.
> +             */
> +            int phys_irq = GICH_LR_PHYS_ID(*entry);
> +            int rcpu = gic_get_vcpu_real_id(cpu);

You should check here that phys_irq is not one of the reserved
values >= GIC_MAXIRQ (ie 1020-1023). Otherwise the GIC_DIST_CLEAR_ACTIVE()
below will index off the end of the irq_state[] array.

(The current code for the physical GIC doesn't make this check, which
is a bit lax of it, but we should treat that as a separate bug rather
than trying to fix it here. I'll send a patch that fixes that for 3.0.)

> +
> +            /* This is equivalent to a NS write to DIR on the physical CPU
> +             * interface. Hence group0 interrupt deactivation is ignored if
> +             * the GIC is secure.
> +             */
> +            if (!s->security_extn || GIC_DIST_TEST_GROUP(phys_irq, 1 << rcpu)) {
> +                GIC_DIST_CLEAR_ACTIVE(phys_irq, 1 << rcpu);
> +            }
> +        }
> +    } else {
> +        GIC_DIST_CLEAR_ACTIVE(irq, 1 << cpu);
> +    }
> +}

>  #endif /* QEMU_ARM_GIC_INTERNAL_H */

thanks
-- PMM