| Message ID | 20190212181917.8322-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/armv7m_nvic: Allow byte accesses to SHPR1 | expand |
Hi Peter, On 2/12/19 7:19 PM, Peter Maydell wrote: > The code for handling the NVIC SHPR1 register intends to permit > byte and halfword accesses (as the architecture requires). However > the 'case' line for it only lists the base address of the > register, so attempts to access bytes other than the first one > end up in the "bad write" default logic. This bug was added > accidentally when we split out the SHPR1 logic from SHPR2 and > SHPR3 to support v6M. > > Fixes: 7c9140afd594 ("nvic: Handle ARMv6-M SCS reserved registers") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > The Zephyr RTOS happens to access SHPR1 byte at a time, > which is how I spotted this. > --- > hw/intc/armv7m_nvic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > index 790a3d95849..2fd40f9dc4c 100644 > --- a/hw/intc/armv7m_nvic.c > +++ b/hw/intc/armv7m_nvic.c > @@ -1956,7 +1956,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr, > } > nvic_irq_update(s); > return MEMTX_OK; > - case 0xd18: /* System Handler Priority (SHPR1) */ > + case 0xd18 ... 0xd1b: /* System Handler Priority (SHPR1) */ Can you fix the nvic_sysreg_read() case too? With that: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) { > return MEMTX_OK; > } >
On Tue, 12 Feb 2019 at 18:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Peter, > > On 2/12/19 7:19 PM, Peter Maydell wrote: > > The code for handling the NVIC SHPR1 register intends to permit > > byte and halfword accesses (as the architecture requires). However > > the 'case' line for it only lists the base address of the > > register, so attempts to access bytes other than the first one > > end up in the "bad write" default logic. This bug was added > > accidentally when we split out the SHPR1 logic from SHPR2 and > > SHPR3 to support v6M. > > > > Fixes: 7c9140afd594 ("nvic: Handle ARMv6-M SCS reserved registers") > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > The Zephyr RTOS happens to access SHPR1 byte at a time, > > which is how I spotted this. > > --- > > hw/intc/armv7m_nvic.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > > index 790a3d95849..2fd40f9dc4c 100644 > > --- a/hw/intc/armv7m_nvic.c > > +++ b/hw/intc/armv7m_nvic.c > > @@ -1956,7 +1956,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr, > > } > > nvic_irq_update(s); > > return MEMTX_OK; > > - case 0xd18: /* System Handler Priority (SHPR1) */ > > + case 0xd18 ... 0xd1b: /* System Handler Priority (SHPR1) */ > > Can you fix the nvic_sysreg_read() case too? > > With that: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks for the catch -- careless of me not to check the read code too. -- PMM
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 790a3d95849..2fd40f9dc4c 100644 --- a/hw/intc/armv7m_nvic.c +++ b/hw/intc/armv7m_nvic.c @@ -1956,7 +1956,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr, } nvic_irq_update(s); return MEMTX_OK; - case 0xd18: /* System Handler Priority (SHPR1) */ + case 0xd18 ... 0xd1b: /* System Handler Priority (SHPR1) */ if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) { return MEMTX_OK; }
The code for handling the NVIC SHPR1 register intends to permit byte and halfword accesses (as the architecture requires). However the 'case' line for it only lists the base address of the register, so attempts to access bytes other than the first one end up in the "bad write" default logic. This bug was added accidentally when we split out the SHPR1 logic from SHPR2 and SHPR3 to support v6M. Fixes: 7c9140afd594 ("nvic: Handle ARMv6-M SCS reserved registers") Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- The Zephyr RTOS happens to access SHPR1 byte at a time, which is how I spotted this. --- hw/intc/armv7m_nvic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)