| Message ID | 0b02fe788de99120894f87f6d5c60e15d6a75d85.1586213450.git.dirty@apple.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] nrf51: Fix last GPIO CNF address | expand |
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 >
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
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 >
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/
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
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.
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
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
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
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. >
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
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(-)