Re: [Qemu-arm] [PATCH 8/8] hw/intc/armv7m_nvic: Fix byte-to-interrupt number conversions

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/05/2018 07:57 AM, Peter Maydell wrote:
> In many of the NVIC registers relating to interrupts, we
> have to convert from a byte offset within a register set
> into the number of the first interrupt which is affected.
> We were getting this wrong for:
>  * reads of NVIC_ISPR<n>, NVIC_ISER<n>, NVIC_ICPR<n>, NVIC_ICER<n>,
>    NVIC_IABR<n> -- in all these cases we were missing the "* 8"
>    needed to convert from the byte offset to the interrupt number
>    (since all these registers use one bit per interrupt)
>  * writes of NVIC_IPR<n> had the opposite problem of a spurious
>    "* 8" (since these registers use one byte per interrupt)

What about using inline function (suggested) with those comments to ease
code review, since this code is kinda confusing at first.

> 
> Signed-off-by: Peter Maydell <address@hidden>

at any rate:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  hw/intc/armv7m_nvic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 8726be796e..9433efd1b8 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1721,7 +1721,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, 
> hwaddr addr,
>          /* fall through */
>      case 0x180 ... 0x1bf: /* NVIC Clear enable */
>          val = 0;
> -        startvec = offset - 0x180 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x180) + NVIC_FIRST_IRQ; /* vector # */

static uint32_t off2vec_rd(uint32_t offset, uint32_t base)
{
    return 8 * (offset - base) + NVIC_FIRST_IRQ;
}

>  
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; 
> i++) {
>              if (s->vectors[startvec + i].enabled &&
> @@ -1735,7 +1735,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, 
> hwaddr addr,
>          /* fall through */
>      case 0x280 ... 0x2bf: /* NVIC Clear pend */
>          val = 0;
> -        startvec = offset - 0x280 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x280) + NVIC_FIRST_IRQ; /* vector # */
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; 
> i++) {
>              if (s->vectors[startvec + i].pending &&
>                  (attrs.secure || s->itns[startvec + i])) {
> @@ -1745,7 +1745,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, 
> hwaddr addr,
>          break;
>      case 0x300 ... 0x33f: /* NVIC Active */
>          val = 0;
> -        startvec = offset - 0x300 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x300) + NVIC_FIRST_IRQ; /* vector # */
>  
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; 
> i++) {
>              if (s->vectors[startvec + i].active &&
> @@ -1860,7 +1860,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, 
> hwaddr addr,
>      case 0x300 ... 0x33f: /* NVIC Active */
>          return MEMTX_OK; /* R/O */
>      case 0x400 ... 0x5ef: /* NVIC Priority */
> -        startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */

static uint32_t off2vec_wr(uint32_t offset, uint32_t base)
{
    return (offset - base) + NVIC_FIRST_IRQ;
}

>  
>          for (i = 0; i < size && startvec + i < s->num_irq; i++) {
>              if (attrs.secure || s->itns[startvec + i]) {
>

signature.asc
Description: OpenPGP digital signature