Re: [PATCH v1] nrf51: Fix last GPIO CNF address

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Cameron,

On 4/7/20 12:55 AM, Cameron Esfahani wrote:
> NRF51_GPIO_REG_CNF_END doesn't actually refer to the start of the last
> valid CNF register: it's referring to the last byte of the last valid
> CNF register.
> 
> This hasn't been a problem up to now, as current implementation in
> memory.c turns an unaligned 4-byte read from 0x77f to a single byte read
> and the qtest only looks at the least-significant byte of the register.
> 
> But, when running with Cedric Le Goater's <clg@kaod.org> pending fix for
> unaligned accesses in memory.c, the qtest breaks.

The 'fix' is from Andrew.

> 
> Considering NRF51 doesn't support unaligned accesses, the simplest fix
> is to actually set NRF51_GPIO_REG_CNF_END to the start of the last valid
> CNF register: 0x77c.

NAck. You are burying bugs deeper. This test has to work.

What would be helpful is qtests with unaligned accesses (expected to 
work) such your USB XHCI VERSION example.

> 
> Now, qtests work with or without Cedric's patch.

For the other reviewers, the cited patch is:
https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65.patch

If you look at it closer, it has:

          /* XXX: Big-endian path is untested...  */

              /* XXX: Can't do this hack for writes */

Also Paolo Bonzini made comments that are not addressed, about 3 years 
later:
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03684.html

> 
> Signed-off-by: Cameron Esfahani <dirty@apple.com>
> ---
>   include/hw/gpio/nrf51_gpio.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
> index 337ee534bb..1d62bbc928 100644
> --- a/include/hw/gpio/nrf51_gpio.h
> +++ b/include/hw/gpio/nrf51_gpio.h
> @@ -42,7 +42,7 @@
>   #define NRF51_GPIO_REG_DIRSET       0x518
>   #define NRF51_GPIO_REG_DIRCLR       0x51C
>   #define NRF51_GPIO_REG_CNF_START    0x700
> -#define NRF51_GPIO_REG_CNF_END      0x77F
> +#define NRF51_GPIO_REG_CNF_END      0x77C
>   
>   #define NRF51_GPIO_PULLDOWN 1
>   #define NRF51_GPIO_PULLUP 3
>