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

[Peter Maydell]

On 5 February 2018 at 16:16, Philippe Mathieu-Daudé <address@hidden> wrote:
> 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;
> }

This function name suggests that it's for reads, which isn't
what's going on here. The 8 * version is for registers which
have 1 bit per interrupt. The version without 8* is for
registers which have 8 bits per interrupt. It just happens
that we got the reads wrong for a bunch of 1-bit-per-interrupt
registers and the writes wrong for an 8-bit-per-interrupt reg.
(I have a feeling this is something I noticed at some point in
code review of the original author's patches, and then didn't
realize had been not quite corrected in all the places it needed
to be, and overcorrected in one spot.)

Also, if you hide the "8 *" inside a function here, you lose
the fact that it parallels the multiplication in "end = size * 8"
in each of these loops. So I agree that it's confusing, but
overall I think it's better to have all the logic in one
place so you can read the whole loop easily.

thanks
-- PMM