On 5 September 2016 at 16:45, Alex Bennée <[email protected]> wrote: > > Peter Maydell <[email protected]> writes: > >> On 5 September 2016 at 15:09, Alex Bennée <[email protected]> wrote: >>> } else if (offset < 0x400) { >>> - /* Interrupt Active. */ >>> - goto bad_reg; >>> + /* Interrupt Clear-Active */ >>> + irq = (offset - 0x380) * 8 + GIC_BASE_IRQ; >>> + if (irq >= s->num_irq) >>> + goto bad_reg; >> >> This is missing the check against s->revision. > > This was deliberate as the GICv1 reference was only mentioned in the > GICD_ISACTIVERn description. Unfortunately the only documentation for > GICs on silver.arm.com are for v2 and v3 so I had to guess what v1 had :-/
The GICv2 spec does actually say these registers don't exist in v1: in the Table 4-1 Distributor register map, the entry for GICD_ICACTIVERn is marked as having footnote 'e' applying, which says "GICv2 only". In the GICv1 spec range 0x380-0x3FC is marked Reserved (ie there is no clear-active register set at all.) Ditto in the 11MPCore TRM and the v7M ARM ARM. (The GICv1 register summary also appears in the A9 MPCore TRM, though that's a bit of an obscure place to know to look for it.) As a rule of thumb, if unsure whether something exists in all configurations, default to leaving it locked down/unimplemented. We can always widen it later if guest software turns out to use it, but it's much harder to later know we can remove something safely once we've let a QEMU into the wild that implemented it. thanks -- PMM
