[PATCH v1] nrf51: Fix last GPIO CNF address

[PATCH v1] nrf51: Fix last GPIO CNF address

[Cameron Esfahani via]

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.

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.

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

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
-- 
2.24.0

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

[Cédric Le Goater]

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.
> 
> 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.
> 
> Now, qtests work with or without Cedric's patch.
> 
> Signed-off-by: Cameron Esfahani <dirty@apple.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  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
> 

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

[Peter Maydell]

On Mon, 6 Apr 2020 at 23:55, Cameron Esfahani <dirty@apple.com> 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.

Do you have a link to this patch, please? I had a quick search through
my mailing list articles but couldn't see anything obviously relevant.

thanks
-- PMM

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

[Joel Stanley]

On Tue, 7 Apr 2020 at 08:41, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 6 Apr 2020 at 23:55, Cameron Esfahani <dirty@apple.com> 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.
>
> Do you have a link to this patch, please? I had a quick search through
> my mailing list articles but couldn't see anything obviously relevant.

There is a reference in this thread:

https://lore.kernel.org/qemu-devel/dd8fc1f7-56d9-4d9f-96a4-0fdcafdc8f55@www.fastmail.com/

The patch is here:

https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/

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

[Peter Maydell]

On Tue, 7 Apr 2020 at 09:45, Joel Stanley <joel@jms.id.au> wrote:
> On Tue, 7 Apr 2020 at 08:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Do you have a link to this patch, please? I had a quick search through
> > my mailing list articles but couldn't see anything obviously relevant.
>
> There is a reference in this thread:
>
> https://lore.kernel.org/qemu-devel/dd8fc1f7-56d9-4d9f-96a4-0fdcafdc8f55@www.fastmail.com/
>
> The patch is here:
>
> https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/

Oh, that's from 2017, no wonder I couldn't find it!

Does somebody who's already reviewed the patch want to summarize
what the effects on devices are -- i.e. what calls the device's read/write
methods used to get if the guest did an unaligned access, including an
unaligned access half off-the-end of the memory region, and what
calls the read/write methods get after the patch ? The patch's commit
message doesn't really describe what it's doing...

thanks
-- PMM

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

[Andrew Jeffery]

On Tue, 7 Apr 2020, at 18:20, Peter Maydell wrote:
> On Tue, 7 Apr 2020 at 09:45, Joel Stanley <joel@jms.id.au> wrote:
> > On Tue, 7 Apr 2020 at 08:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > Do you have a link to this patch, please? I had a quick search through
> > > my mailing list articles but couldn't see anything obviously relevant.
> >
> > There is a reference in this thread:
> >
> > https://lore.kernel.org/qemu-devel/dd8fc1f7-56d9-4d9f-96a4-0fdcafdc8f55@www.fastmail.com/
> >
> > The patch is here:
> >
> > https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/
> 
> Oh, that's from 2017, no wonder I couldn't find it!

Yeah, I never quite got back to finishing it :(

It's development was driven by development of the ASPEED ADC model,
which I hacked up in the interest of getting the ASPEED SDK booting
under qemu (the SDK kernel had an infinite spin waiting for the ADC-ready
bit).

IIRC Phil wanted to enable sub-word accesses to the sample value
registers but I didn't want to spread logic dealing with different access
widths through the model. We already had a mechanism to describe the
model's  supported access sizes (as opposed to the valid access sizes
allowed by hardware) in the `impl` member of the MemoryRegionOps, so
I was trying to use that, but it didn't work as I needed.

The accesses generated at the point of the guest MMIO need to be
expanded to the access width supported by the model and then the
resulting data trimmed again upon returning the data (in the case of a
read) via the MMIO operation.

So the intent was less about unaligned accesses than enabling models
implementations that only have to handle certain-sized access widths.
It happens to help the unaligned access case as well.

> 
> Does somebody who's already reviewed the patch want to summarize
> what the effects on devices are -- i.e. what calls the device's read/write
> methods used to get if the guest did an unaligned access, including an
> unaligned access half off-the-end of the memory region, and what
> calls the read/write methods get after the patch ? The patch's commit
> message doesn't really describe what it's doing...

Honestly any of that information has well left my memory at this point, I'd
have to analyse the patch to recover it.

I was hoping that my turn-around time would be shorter than 3 years but
there hasn't been a shortage of fires to put out in the mean time.

Andrew

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

[Peter Maydell]

On Fri, 10 Apr 2020 at 04:42, Andrew Jeffery <andrew@aj.id.au> wrote:
> IIRC Phil wanted to enable sub-word accesses to the sample value
> registers but I didn't want to spread logic dealing with different access
> widths through the model. We already had a mechanism to describe the
> model's  supported access sizes (as opposed to the valid access sizes
> allowed by hardware) in the `impl` member of the MemoryRegionOps, so
> I was trying to use that, but it didn't work as I needed.
>
> The accesses generated at the point of the guest MMIO need to be
> expanded to the access width supported by the model and then the
> resulting data trimmed again upon returning the data (in the case of a
> read) via the MMIO operation.
>
> So the intent was less about unaligned accesses than enabling models
> implementations that only have to handle certain-sized access widths.
> It happens to help the unaligned access case as well.

Yeah, we definitely could do with improving things here, it is annoying
to have to write code for handling some of the oddball cases when you
have just one register that allows byte accesses or whatever.
The thing I have in the back of my mind as a concern is that we have
had several "is this a buffer overrun" questions where the answer has
been "it can't be, because the core code doesn't allow a call to the
read/write function for a 4 byte access where the address is not 4-aligned,
so my_byte_array[addr] is always in-bounds".
So if we change the core code we need to make sure we don't
inadvertently remove a restriction that was protecting us from a guest
escape...

-- PMM

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

[Andrew Jeffery]

On Fri, 10 Apr 2020, at 21:56, Peter Maydell wrote:
> On Fri, 10 Apr 2020 at 04:42, Andrew Jeffery <andrew@aj.id.au> wrote:
> > IIRC Phil wanted to enable sub-word accesses to the sample value
> > registers but I didn't want to spread logic dealing with different access
> > widths through the model. We already had a mechanism to describe the
> > model's  supported access sizes (as opposed to the valid access sizes
> > allowed by hardware) in the `impl` member of the MemoryRegionOps, so
> > I was trying to use that, but it didn't work as I needed.
> >
> > The accesses generated at the point of the guest MMIO need to be
> > expanded to the access width supported by the model and then the
> > resulting data trimmed again upon returning the data (in the case of a
> > read) via the MMIO operation.
> >
> > So the intent was less about unaligned accesses than enabling models
> > implementations that only have to handle certain-sized access widths.
> > It happens to help the unaligned access case as well.
> 
> Yeah, we definitely could do with improving things here, it is annoying
> to have to write code for handling some of the oddball cases when you
> have just one register that allows byte accesses or whatever.
> The thing I have in the back of my mind as a concern is that we have
> had several "is this a buffer overrun" questions where the answer has
> been "it can't be, because the core code doesn't allow a call to the
> read/write function for a 4 byte access where the address is not 4-aligned,
> so my_byte_array[addr] is always in-bounds".
> So if we change the core code we need to make sure we don't
> inadvertently remove a restriction that was protecting us from a guest
> escape...

Oh for sure. The patch was very RFC, as mentioned I just sent it to check
whether I was on the right track or off in the weeds, and from there it has
hung around in Cedric's tree without much progress.

Feels like the time is right to sort the problem out properly, which might
mean starting from scratch to make sure we don't miss any of the details.

Andrew

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

[Philippe Mathieu-Daudé]

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
> 

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

[Cameron Esfahani via]

I'm not burying anything.  This patch is stand alone and all the tests do work.  They work with or without Cedric's nee Andrew's patch.  But, if some derivative of that patch is eventually implemented, something needs to be done for this NRF51 gpio qtest to work.

There are two possibilities for the following qtest in microbit-test.c:

>     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
>     g_assert_cmpuint(actual, ==, 0x01);


1 - The code is purposefully reading 32-bits from an unaligned address which only partially refers to a documented register.  And the only reason this code works is because that 32-bit value is turned into a 8-bit read.  And if that's the case, then someone should update a comment in the test to indicate that this is being done purposefully.
2 - NRF51_GPIO_REG_CNF_END is incorrectly set to be 0x77F.  Looking at the documentation for this chip, the last defined CNF register starts at 0x77C.

The NRF51 doesn't support unaligned accesses, so I assume that possibility 1 isn't true.

So, is NRF51_GPIO_REG_CNF_END supposed to refer to a valid register, or the end of the implementation space?

If it's the former, then it should be adjusted to 0x77c and possibly update the below code in nrf51_gpio.c (line 156):

>     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END:


to become

>     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END + 3:

if unaligned access are expected to work.  But, considering the NRF51 doesn't support unaligned accesses, I don't think this appropriate.

If it's the latter, then the test cases in microbit-test.c should be updated to something like the following:

>     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END - 3) & 0x01;
>     g_assert_cmpuint(actual, ==, 0x01);


Cameron Esfahani
dirty@apple.com

"Americans are very skilled at creating a custom meaning from something that's mass-produced."

Ann Powers


> On Apr 7, 2020, at 1:44 AM, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> 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.

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

[Joel Stanley]

On Tue, 7 Apr 2020 at 10:09, Cameron Esfahani <dirty@apple.com> wrote:
>
> I'm not burying anything.  This patch is stand alone and all the tests do work.  They work with or without Cedric's nee Andrew's patch.  But, if some derivative of that patch is eventually implemented, something needs to be done for this NRF51 gpio qtest to work.
>
> There are two possibilities for the following qtest in microbit-test.c:
>
> >     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
> >     g_assert_cmpuint(actual, ==, 0x01);
>
>
> 1 - The code is purposefully reading 32-bits from an unaligned address which only partially refers to a documented register.  And the only reason this code works is because that 32-bit value is turned into a 8-bit read.  And if that's the case, then someone should update a comment in the test to indicate that this is being done purposefully.
> 2 - NRF51_GPIO_REG_CNF_END is incorrectly set to be 0x77F.  Looking at the documentation for this chip, the last defined CNF register starts at 0x77C.
>
> The NRF51 doesn't support unaligned accesses, so I assume that possibility 1 isn't true.
>
> So, is NRF51_GPIO_REG_CNF_END supposed to refer to a valid register, or the end of the implementation space?
>
> If it's the former, then it should be adjusted to 0x77c and possibly update the below code in nrf51_gpio.c (line 156):
>
> >     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END:
>
>
> to become
>
> >     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END + 3:
>
> if unaligned access are expected to work.  But, considering the NRF51 doesn't support unaligned accesses, I don't think this appropriate.
>
> If it's the latter, then the test cases in microbit-test.c should be updated to something like the following:
>
> >     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END - 3) & 0x01;
> >     g_assert_cmpuint(actual, ==, 0x01);


Your reasoning is sound, thanks for writing it out. I would be happy
to see your patch land.

Reviewed-by: Joel Stanley <joel@jms.id.au>


>
>
> Cameron Esfahani
> dirty@apple.com
>
> "Americans are very skilled at creating a custom meaning from something that's mass-produced."
>
> Ann Powers
>
>
> > On Apr 7, 2020, at 1:44 AM, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> >> 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.
>