| Message ID | 4EF0DDEB.40302@calxeda.com |
|---|---|
| State | New |
| Headers | show |
On 20 December 2011 19:11, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> > --- > hw/arm_gic.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/arm_gic.c b/hw/arm_gic.c > index 9b52119..5974c2f 100644 > --- a/hw/arm_gic.c > +++ b/hw/arm_gic.c > @@ -274,7 +274,7 @@ static uint32_t gic_dist_readb(void *opaque, > target_phys_addr_t offset) > > cpu = gic_get_current_cpu(); > cm = 1 << cpu; > - if (offset < 0x100) { > + if (offset < 0x80) { > #ifndef NVIC > if (offset == 0) > return s->enabled; > @@ -284,6 +284,9 @@ static uint32_t gic_dist_readb(void *opaque, > target_phys_addr_t offset) > return 0; > #endif > goto bad_reg; > + } else if (offset < 0x100) { > + /* Interrupt Security */ > + return 0; This won't actually break anything, but really the handling of 0x80..0xff would be inside the first if() clause, because of the way we piggyback the v7M NVIC off these functions. (We should clean that up, really). Anyway, the v7M NVIC doesn't own 0x0..0xff, which is what the first if () clause is marking off. Ditto in the write function. It would also be nice to have the comment explicitly say that these registers are defined to RAZ/WI in GIC implementations that don't implement the security extensions. (Strictly speaking, in the 11MPcore GIC these locations are reserved and probably don't RAZ/WI, but I don't think we need to worry about that wrinkle now.) > } else if (offset < 0x200) { > /* Interrupt Set/Clear Enable. */ > if (offset < 0x180) > @@ -404,7 +407,7 @@ static void gic_dist_writeb(void *opaque, > target_phys_addr_t offset, > int cpu; > > cpu = gic_get_current_cpu(); > - if (offset < 0x100) { > + if (offset < 0x80) { > #ifdef NVIC > goto bad_reg; > #else > @@ -417,6 +420,9 @@ static void gic_dist_writeb(void *opaque, > target_phys_addr_t offset, > goto bad_reg; > } > #endif > + } else if (offset < 0x100) { > + /* Interrupt Security Registers */ > + /* ignore */ > } else if (offset < 0x180) { > /* Interrupt Set Enable. */ > irq = (offset - 0x100) * 8 + GIC_BASE_IRQ; > -- > 1.7.5.4 > -- PMM
On 12/20/2011 01:58 PM, Peter Maydell wrote: > On 20 December 2011 19:11, Mark Langsdorf<mark.langsdorf@calxeda.com> wrote: >> From: Rob Herring<rob.herring@calxeda.com> >> >> Signed-off-by: Rob Herring<rob.herring@calxeda.com> >> Signed-off-by: Mark Langsdorf<mark.langsdorf@calxeda.com> >> --- >> hw/arm_gic.c | 10 ++++++++-- >> 1 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm_gic.c b/hw/arm_gic.c >> index 9b52119..5974c2f 100644 >> --- a/hw/arm_gic.c >> +++ b/hw/arm_gic.c >> @@ -274,7 +274,7 @@ static uint32_t gic_dist_readb(void *opaque, >> target_phys_addr_t offset) >> >> cpu = gic_get_current_cpu(); >> cm = 1<< cpu; >> - if (offset< 0x100) { >> + if (offset< 0x80) { >> #ifndef NVIC >> if (offset == 0) >> return s->enabled; >> @@ -284,6 +284,9 @@ static uint32_t gic_dist_readb(void *opaque, >> target_phys_addr_t offset) >> return 0; >> #endif >> goto bad_reg; >> + } else if (offset< 0x100) { >> + /* Interrupt Security */ >> + return 0; > > This won't actually break anything, but really the handling of 0x80..0xff > would be inside the first if() clause, because of the way we piggyback > the v7M NVIC off these functions. (We should clean that up, really). > Anyway, the v7M NVIC doesn't own 0x0..0xff, which is what the first > if () clause is marking off. Ditto in the write function. > > It would also be nice to have the comment explicitly say that these > registers are defined to RAZ/WI in GIC implementations that don't > implement the security extensions. > I'm not sure what changes you want made here. I can resubmit with the comment. But I don't know how qemu piggybacks the v7M NVIC so I don't understand what you want done. --Mark Langsdorf Calxeda, Inc.
On 20 December 2011 21:06, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > On 12/20/2011 01:58 PM, Peter Maydell wrote: >> On 20 December 2011 19:11, Mark Langsdorf<mark.langsdorf@calxeda.com> wrote: >>> From: Rob Herring<rob.herring@calxeda.com> >>> >>> Signed-off-by: Rob Herring<rob.herring@calxeda.com> >>> Signed-off-by: Mark Langsdorf<mark.langsdorf@calxeda.com> >>> --- >>> hw/arm_gic.c | 10 ++++++++-- >>> 1 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/arm_gic.c b/hw/arm_gic.c >>> index 9b52119..5974c2f 100644 >>> --- a/hw/arm_gic.c >>> +++ b/hw/arm_gic.c >>> @@ -274,7 +274,7 @@ static uint32_t gic_dist_readb(void *opaque, >>> target_phys_addr_t offset) >>> >>> cpu = gic_get_current_cpu(); >>> cm = 1<< cpu; >>> - if (offset< 0x100) { >>> + if (offset< 0x80) { >>> #ifndef NVIC >>> if (offset == 0) >>> return s->enabled; >>> @@ -284,6 +284,9 @@ static uint32_t gic_dist_readb(void *opaque, >>> target_phys_addr_t offset) >>> return 0; >>> #endif >>> goto bad_reg; >>> + } else if (offset< 0x100) { >>> + /* Interrupt Security */ >>> + return 0; >> >> This won't actually break anything, but really the handling of 0x80..0xff >> would be inside the first if() clause, because of the way we piggyback >> the v7M NVIC off these functions. (We should clean that up, really). >> Anyway, the v7M NVIC doesn't own 0x0..0xff, which is what the first >> if () clause is marking off. Ditto in the write function. >> >> It would also be nice to have the comment explicitly say that these >> registers are defined to RAZ/WI in GIC implementations that don't >> implement the security extensions. >> > I'm not sure what changes you want made here. I can resubmit with > the comment. But I don't know how qemu piggybacks the v7M NVIC so I > don't understand what you want done. I just mean that you want something like if (offset < 0x100) { #ifndef NVIC if (offset == 0) return s->enabled; if (offset == 4) return ((GIC_NIRQ / 32) - 1) | ((NUM_CPU(s) - 1) << 5); if (offset < 0x08) return 0; if (offset >= 0x80 && offset < 0x100) { /* Interrupt security, RAZ/WI */ return 0; } #endif goto bad_reg; } else if (offset < 0x200) { to keep this GIC-only register inside the not-NVIC ifdef. (I really want to clean this up but I can't do that until we can properly have memory regions which don't start at a page boundary and which pass the right offset through.) -- PMM
diff --git a/hw/arm_gic.c b/hw/arm_gic.c index 9b52119..5974c2f 100644 --- a/hw/arm_gic.c +++ b/hw/arm_gic.c @@ -274,7 +274,7 @@ static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset) cpu = gic_get_current_cpu(); cm = 1 << cpu; - if (offset < 0x100) { + if (offset < 0x80) { #ifndef NVIC if (offset == 0)