On 13 June 2016 at 07:27, Shannon Zhao <[email protected]> wrote: > > > On 2016/5/26 22:55, Peter Maydell wrote: >> +static uint8_t gicd_read_ipriorityr(GICv3State *s, MemTxAttrs attrs, int >> irq) >> +{ >> + /* Read the value of GICD_IPRIORITYR<n> for the specified interrupt, >> + * honouring security state (these are RAZ/WI for Group 0 or Secure >> + * Group 1 interrupts). >> + */ >> + uint32_t prio; >> + >> + if (irq < GIC_INTERNAL || irq >= s->num_irq) { >> + return 0; >> + } >> + >> + prio = s->gicd_ipriority[irq]; >> + >> + if (!attrs.secure && !(s->gicd_ctlr & GICD_CTLR_DS)) { >> + if (!gicv3_gicd_group_test(s, irq)) { >> + /* Fields for Group 0 or Secure Group 1 interrupts are RAZ/WI */ > Here this check assure this interrupt belongs to Group 0 and NS access > is not permitted, so it should return 0. But it doesn't say anything > about Secure Group 1.
We're testing the GICD_IGROUPR bit here. If DS is zero (security enabled), then IGROUPR == 0 means "Group 0 or Secure Group 1", which is what the comment says we're testing. (If you care which of 0 and S1 it is then you look at IGRPMODR, but for security checks like these we don't need to.) >> + return 0; >> + } >> + /* NS view of the interrupt priority */ >> + prio = (prio << 1) & 0xff; >> + } > So maybe here it should check if attrs.secure is true and the Group is > 1, then return 0. I'm confused. If attrs.secure is true this is a secure access which should be allowed to see anything, shouldn't it? >> + return prio; >> +} thanks -- PMM
