On 29 June 2018 at 14:29, Luc Michel <luc.mic...@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.mic...@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