* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-05-29 22:03 ` [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART Julia Suvorova
@ 2018-05-31 9:42 ` Stefan Hajnoczi
2018-06-01 15:21 ` Julia Suvorova
2018-05-31 12:28 ` Peter Maydell
2018-05-31 13:58 ` sundeep subbaraya
2 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2018-05-31 9:42 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Peter Maydell, Joel Stanley, Jim Mussared,
Steffen Görtz
[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]
On Wed, May 30, 2018 at 01:03:37AM +0300, Julia Suvorova wrote:
> The following features are not yet implemented:
> Control with SUSPEND/START*/STOP*
This is probably worth implementing for completeness. Just rx_enabled
and tx_enabled boolean states will be sufficient. SUSPEND flushes tx
and stops rx, it doesn't require any extra state.
> CTS/NCTS flow control
CTS flow control is probably not necessary since we don't have bitwise
I/O and guest applications probably don't care either.
> Mapping registers to pins
This is probably not necessary since the micro:bit only uses the UART in
one pin configuration (back to the USB interface chip).
Please make sure that every register is explicitly handle in the code
and falls into these categories:
1. Implemented.
2. Unimplemented - can be observed via a trace event. This will make
debugging easier in the future when an application doesn't work
with the UART.
3. Stubbed out - explicitly marked as ignored in the code.
This way we can be confident about the completeness of this device
model.
> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> new file mode 100644
> index 0000000000..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * nRF51 SoC UART emulation
Please add a reference to the hardware specfication here:
See nRF51 Series Reference Manual, "29 Universal Asynchronous
Receiver/Transmitter" for hardware specifications:
http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> + int r;
> +
> + s->watch_tag = 0;
> +
> + r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
This does not work on big-endian hosts:
s->reg[A_TXD] = 'A';
little-endian memory layout: 41 00 00 00
big-endian memory layout: 00 00 00 41
(uint8_t *) &s->reg[A_TXD] --^
Instead please use a uint8_t local:
uint8_t ch = s->reg[A_TXD];
...
r = qemu_chr_fe_write(&s->chr, &ch, 1);
> + if (r <= 0) {
> + s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> + uart_transmit, s);
> + if (!s->watch_tag) {
> + goto buffer_drained;
Please add a comment here:
/* The hardware has no transmit error reporting, so silently drop the byte */
> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
Indentation is off. Please use 4 spaces.
> + return;
> + }
> +
> + s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> + s->rx_fifo_len++;
> +
> + s->reg[A_RXDRDY] = 1;
> + nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
This is very subtle: this function returns the number of bytes that can
be read. This expression looks like a boolean return value but actually
relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
Please write it so it doesn't look like a boolean return value:
if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
return 0;
}
return 1 /* byte */;
Alternatively, you could handle more than 1 byte:
return sizeof(s->rx_fifo) - s->rx_fifo_len;
but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
data in a single call.
> +}
> +
> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> + Nrf51UART *s = NRF51_UART(dev);
> +
> + qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> + NULL, NULL, s, NULL, true);
> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> + Nrf51UART *s = NRF51_UART(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> + "nrf51_soc.uart", 0x1000);
s/0x1000/UART_SIZE/
> +typedef struct Nrf51UART {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> + CharBackend chr;
> + qemu_irq irq;
> + guint watch_tag;
> +
> + uint8_t rx_fifo[UART_FIFO_LENGTH];
> + unsigned int rx_fifo_pos;
> + unsigned int rx_fifo_len;
> +
> + uint32_t reg[0x1000];
Where does 0x1000 come from? I think the actual 0x1000-byte range would
be uint32_t reg[UART_SIZE / sizeof(reg[0])].
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-05-31 9:42 ` Stefan Hajnoczi
@ 2018-06-01 15:21 ` Julia Suvorova
2018-06-01 15:58 ` Peter Maydell
0 siblings, 1 reply; 17+ messages in thread
From: Julia Suvorova @ 2018-06-01 15:21 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Peter Maydell, Joel Stanley, Jim Mussared,
Steffen Görtz
On 31.05.2018 12:42, Stefan Hajnoczi wrote:
> On Wed, May 30, 2018 at 01:03:37AM +0300, Julia Suvorova wrote:
>> The following features are not yet implemented:
>> Control with SUSPEND/START*/STOP*
>
> This is probably worth implementing for completeness. Just rx_enabled
> and tx_enabled boolean states will be sufficient. SUSPEND flushes tx
> and stops rx, it doesn't require any extra state.
>
>> CTS/NCTS flow control
>
> CTS flow control is probably not necessary since we don't have bitwise
> I/O and guest applications probably don't care either.
>
>> Mapping registers to pins
>
> This is probably not necessary since the micro:bit only uses the UART in
> one pin configuration (back to the USB interface chip).
>
> Please make sure that every register is explicitly handle in the code
> and falls into these categories:
>
> 1. Implemented.
> 2. Unimplemented - can be observed via a trace event. This will make
> debugging easier in the future when an application doesn't work
> with the UART.
> 3. Stubbed out - explicitly marked as ignored in the code.
>
> This way we can be confident about the completeness of this device
> model.
I've definitely missed handling some registers, so marking it seems a
good idea.
>> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
>> new file mode 100644
>> index 0000000000..2da97aa0c4
>> --- /dev/null
>> +++ b/hw/char/nrf51_uart.c
>> @@ -0,0 +1,232 @@
>> +/*
>> + * nRF51 SoC UART emulation
>
> Please add a reference to the hardware specfication here:
>
> See nRF51 Series Reference Manual, "29 Universal Asynchronous
> Receiver/Transmitter" for hardware specifications:
> http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
Sure, I'll add it.
>> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>> +{
>> + Nrf51UART *s = NRF51_UART(opaque);
>> + int r;
>> +
>> + s->watch_tag = 0;
>> +
>> + r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
>
> This does not work on big-endian hosts:
>
> s->reg[A_TXD] = 'A';
>
> little-endian memory layout: 41 00 00 00
> big-endian memory layout: 00 00 00 41
> (uint8_t *) &s->reg[A_TXD] --^
>
> Instead please use a uint8_t local:
>
> uint8_t ch = s->reg[A_TXD];
>
> ...
>
> r = qemu_chr_fe_write(&s->chr, &ch, 1);
>
A real bug! I'll fix this.
>> + if (r <= 0) {
>> + s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
>> + uart_transmit, s);
>> + if (!s->watch_tag) {
>> + goto buffer_drained;
>
> Please add a comment here:
>
> /* The hardware has no transmit error reporting, so silently drop the byte */
>
The QEMU code is inconsistent with comments. Some files are full of
comments, some have only code. Is there any policy how to comment
the files?
>> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
>> +{
>> +
>> + Nrf51UART *s = NRF51_UART(opaque);
>> +
>> + if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
>
> Indentation is off. Please use 4 spaces.
Sorry, have no idea how it happened.
>> + return;
>> + }
>> +
>> + s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
>> + s->rx_fifo_len++;
>> +
>> + s->reg[A_RXDRDY] = 1;
>> + nrf51_uart_update_irq(s);
>> +}
>> +
>> +static int uart_can_receive(void *opaque)
>> +{
>> + Nrf51UART *s = NRF51_UART(opaque);
>> +
>> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
>
> This is very subtle: this function returns the number of bytes that can
> be read. This expression looks like a boolean return value but actually
> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>
> Please write it so it doesn't look like a boolean return value:
>
> if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
> return 0;
> }
>
> return 1 /* byte */;
>
> Alternatively, you could handle more than 1 byte:
>
> return sizeof(s->rx_fifo) - s->rx_fifo_len;
>
> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
> data in a single call.
I will rewrite uart_receive function to accept many bytes at once.
>> +}
>> +
>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>> +{
>> + Nrf51UART *s = NRF51_UART(dev);
>> +
>> + qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>> + NULL, NULL, s, NULL, true);
>> +}
>> +
>> +static void nrf51_uart_init(Object *obj)
>> +{
>> + Nrf51UART *s = NRF51_UART(obj);
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +
>> + memory_region_init_io(&s->mmio, obj, &uart_ops, s,
>> + "nrf51_soc.uart", 0x1000);
>
> s/0x1000/UART_SIZE/
Should I just add a new define or move the existing one from nrf51_soc.c to
some header (or any other way to pass the UART_SIZE)?
Best regards, Julia Suvorova.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-06-01 15:21 ` Julia Suvorova
@ 2018-06-01 15:58 ` Peter Maydell
2018-06-02 8:15 ` Stefan Hajnoczi
0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2018-06-01 15:58 UTC (permalink / raw)
To: Julia Suvorova
Cc: Stefan Hajnoczi, qemu-devel, Joel Stanley, Jim Mussared,
Steffen Görtz
On 1 June 2018 at 16:21, Julia Suvorova <jusual@mail.ru> wrote:
> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>> Please add a comment here:
>>
>> /* The hardware has no transmit error reporting, so silently drop the
>> byte */
>>
>
> The QEMU code is inconsistent with comments. Some files are full of
> comments, some have only code. Is there any policy how to comment
> the files?
The only policy we have written down is that you should use /* */
even for one-line comments.
/* one line comments like this */
We're inconsistent about how to format multiline comments, but
my preference for the arm parts of the codebase is:
/* multi line comments look
* like this
*/
For the actual content of comments:
* comments generally should describe the 'why' rather than the 'what'
(but not "why did we change this?", which goes in the commit message)
* comments are helpful where the code looks wrong or strange
(as an indication of "this is deliberate and this is why";
Stefan's suggested comment above is in this category). If
there's a way to write the code that doesn't look wrong then
that's obviously preferable :-)
* you don't need to comment things that are obvious from just
reading the code
* I prefer to err on the side of more comments rather than fewer
* you don't need to replicate lots of info that's in the data sheet,
but don't assume your readers know all the weird corners of the
hardware behaviour inside-out, either
* known missing or broken things can be commented with TODO or
FIXME comments (but if you have too many of these for easy things
we may ask you to just fix some of them :-))
Code reviewers will let you know if you've got the balance wrong.
As a special case:
* any new function with global scope should have a doc-comment
(starts with "/**") describing its purpose and API
Optionally:
* for a few recent devices I thought it was useful to document
in their header what the "QEMU interface" (what the various
QOM properties, named GPIO outputs, inputs, etc are). You can
see examples with 'grep "QEMU interface" include/'. If that seems
helpful, feel free to have a comment like that; if not, skip it.
Overall advice: QEMU is a large code base, and it has both new
and well maintained parts and also very old parts that are
unloved and often use no-longer-recommended style or APIs.
When you're looking around for examples to copy the style of,
it's better to try to find something that's been added or
overhauled recently.
>>> +static int uart_can_receive(void *opaque)
>>> +{
>>> + Nrf51UART *s = NRF51_UART(opaque);
>>> +
>>> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>
>> This is very subtle: this function returns the number of bytes that can
>> be read. This expression looks like a boolean return value but actually
>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>
>> Please write it so it doesn't look like a boolean return value:
>>
>> if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>> return 0;
>> }
>>
>> return 1 /* byte */;
>>
>> Alternatively, you could handle more than 1 byte:
>>
>> return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>
>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>> data in a single call.
>
> I will rewrite uart_receive function to accept many bytes at once.
Stefan, I was wondering about this when I read this patch.
What is the API for the can_receive and receive functions?
There's no documentation of the semantics either with the
IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
or in the doc comment for qemu_chr_fe_set_handlers.
I'm guessing that the answer is "your can_read handler should
return the number of bytes you can accept, and a subsequent call
to the read handler then must accept that many bytes, it can't
change its mind and only accept a smaller number" (with some sort
of guarantee by the caller that it won't let guest execution or
other simulation-changes things between the call to can_receive
and receive) ?
(Similarly we don't document the semantics for the NetClientInfo
can_receive and receive functions.)
>>> +}
>>> +
>>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + Nrf51UART *s = NRF51_UART(dev);
>>> +
>>> + qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>>> + NULL, NULL, s, NULL, true);
>>> +}
>>> +
>>> +static void nrf51_uart_init(Object *obj)
>>> +{
>>> + Nrf51UART *s = NRF51_UART(obj);
>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>> +
>>> + memory_region_init_io(&s->mmio, obj, &uart_ops, s,
>>> + "nrf51_soc.uart", 0x1000);
>>
>> s/0x1000/UART_SIZE/
>
> Should I just add a new define or move the existing one from nrf51_soc.c to
> some header (or any other way to pass the UART_SIZE)?
Usual thing here is that the uart's own header file defines this sort
of constant, and then the SoC's .c file includes that header file
and uses the constant.
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-06-01 15:58 ` Peter Maydell
@ 2018-06-02 8:15 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2018-06-02 8:15 UTC (permalink / raw)
To: Peter Maydell
Cc: Julia Suvorova, qemu-devel, Joel Stanley, Jim Mussared,
Steffen Görtz
On Fri, Jun 1, 2018 at 4:58 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2018 at 16:21, Julia Suvorova <jusual@mail.ru> wrote:
>> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>>>> +static int uart_can_receive(void *opaque)
>>>> +{
>>>> + Nrf51UART *s = NRF51_UART(opaque);
>>>> +
>>>> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>>
>>> This is very subtle: this function returns the number of bytes that can
>>> be read. This expression looks like a boolean return value but actually
>>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>>
>>> Please write it so it doesn't look like a boolean return value:
>>>
>>> if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>>> return 0;
>>> }
>>>
>>> return 1 /* byte */;
>>>
>>> Alternatively, you could handle more than 1 byte:
>>>
>>> return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>>
>>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>>> data in a single call.
>>
>> I will rewrite uart_receive function to accept many bytes at once.
>
> Stefan, I was wondering about this when I read this patch.
> What is the API for the can_receive and receive functions?
> There's no documentation of the semantics either with the
> IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
> or in the doc comment for qemu_chr_fe_set_handlers.
>
> I'm guessing that the answer is "your can_read handler should
> return the number of bytes you can accept, and a subsequent call
> to the read handler then must accept that many bytes, it can't
> change its mind and only accept a smaller number" (with some sort
> of guarantee by the caller that it won't let guest execution or
> other simulation-changes things between the call to can_receive
> and receive) ?
>
> (Similarly we don't document the semantics for the NetClientInfo
> can_receive and receive functions.)
I'll send documentation patches.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-05-29 22:03 ` [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART Julia Suvorova
2018-05-31 9:42 ` Stefan Hajnoczi
@ 2018-05-31 12:28 ` Peter Maydell
2018-05-31 13:58 ` sundeep subbaraya
2 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2018-05-31 12:28 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
On 29 May 2018 at 23:03, Julia Suvorova <jusual@mail.ru> wrote:
> Basic implementation of nRF51 SoC UART.
> Description could be found here:
> http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
>
> The following features are not yet implemented:
> Control with SUSPEND/START*/STOP*
> CTS/NCTS flow control
> Mapping registers to pins
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
Hi; thanks for this patch; the UART implementation looks generally
in good shape. I have some specific review comments below.
> ---
> hw/arm/nrf51_soc.c | 7 ++
> hw/char/Makefile.objs | 1 +
> hw/char/nrf51_uart.c | 232 +++++++++++++++++++++++++++++++++++
> include/hw/arm/nrf51_soc.h | 1 +
> include/hw/char/nrf51_uart.h | 54 ++++++++
I recommend having "implement the UART" in one patch, and
"add the new UART to this SoC" as a separate patch.
> 5 files changed, 295 insertions(+)
> create mode 100644 hw/char/nrf51_uart.c
> create mode 100644 include/hw/char/nrf51_uart.h
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 6fe06dcfd2..a2ee6f3f3b 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -21,6 +21,7 @@
> #include "cpu.h"
>
> #include "hw/arm/nrf51_soc.h"
> +#include "hw/char/nrf51_uart.h"
>
> #define IOMEM_BASE 0x40000000
> #define IOMEM_SIZE 0x20000000
> @@ -34,6 +35,9 @@
> #define SRAM_BASE 0x20000000
> #define SRAM_SIZE (16 * 1024)
>
> +#define UART_BASE 0x40002000
> +#define UART_SIZE 0x1000
> +
> static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> {
> NRF51State *s = NRF51_SOC(dev_soc);
> @@ -73,6 +77,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> /* TODO: implement a cortex m0 and update this */
> s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +
> + s->uart = nrf51_uart_create(UART_BASE, qdev_get_gpio_in(s->nvic, 2),
> + serial_hd(0));
I would recommend having the UART struct embedded in the SoC
struct and using in-place init/realize.
Devices in an SoC object should also be being mapped into a
container memory region, rather than mapping themselves
directly into system memory.
> }
>
> static Property nrf51_soc_properties[] = {
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 1b979100b7..1060c62a54 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -1,5 +1,6 @@
> common-obj-$(CONFIG_IPACK) += ipoctal232.o
> common-obj-$(CONFIG_ESCC) += escc.o
> +common-obj-$(CONFIG_NRF51_SOC) += nrf51_uart.o
> common-obj-$(CONFIG_PARALLEL) += parallel.o
> common-obj-$(CONFIG_PARALLEL) += parallel-isa.o
> common-obj-$(CONFIG_PL011) += pl011.o
> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> new file mode 100644
> index 0000000000..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * nRF51 SoC UART emulation
> + *
> + * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
I would suggest having a comment with the data sheet URL here.
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/registerfields.h"
> +#include "hw/char/nrf51_uart.h"
> +
> +REG32(STARTRX, 0x000)
> +REG32(STOPRX, 0x004)
> +REG32(STARTTX, 0x008)
> +REG32(STOPTX, 0x00C)
> +REG32(SUSPEND, 0x01C)
> +
> +REG32(CTS, 0x100)
> +REG32(NCTS, 0x104)
> +REG32(RXDRDY, 0x108)
> +REG32(TXDRDY, 0x11C)
> +REG32(ERROR, 0x124)
> +REG32(RXTO, 0x144)
> +
> +REG32(INTEN, 0x300)
> + FIELD(INTEN, CTS, 0, 1)
> + FIELD(INTEN, NCTS, 1, 1)
> + FIELD(INTEN, RXDRDY, 2, 1)
> + FIELD(INTEN, TXDRDY, 7, 1)
> + FIELD(INTEN, ERROR, 9, 1)
> + FIELD(INTEN, RXTO, 17, 1)
> +REG32(INTENSET, 0x304)
> +REG32(INTENCLR, 0x308)
> +REG32(ERRORSRC, 0x480)
> +REG32(ENABLE, 0x500)
> +REG32(PSELRTS, 0x508)
> +REG32(PSELTXD, 0x50C)
> +REG32(PSELCTS, 0x510)
> +REG32(PSELRXD, 0x514)
> +REG32(RXD, 0x518)
> +REG32(TXD, 0x51C)
> +REG32(BAUDRATE, 0x524)
> +REG32(CONFIG, 0x56C)
> +
> +static void nrf51_uart_update_irq(Nrf51UART *s)
> +{
> + unsigned int irq = 0;
Since this is just holding a boolean, you can make it 'bool' type,
and then you don't need the !! when you pass it to qemu_set_irq().
> +
> + irq = irq || (s->reg[A_RXDRDY] && (s->reg[A_INTEN] & R_INTEN_RXDRDY_MASK));
> + irq = irq || (s->reg[A_TXDRDY] && (s->reg[A_INTEN] & R_INTEN_TXDRDY_MASK));
> + irq = irq || (s->reg[A_ERROR] && (s->reg[A_INTEN] & R_INTEN_ERROR_MASK));
> + irq = irq || (s->reg[A_RXTO] && (s->reg[A_INTEN] & R_INTEN_RXTO_MASK));
> +
> + qemu_set_irq(s->irq, !!irq);
> +}
> +
> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> + uint64_t r;
> +
> + switch (addr) {
> + case A_RXD:
> + r = s->rx_fifo[s->rx_fifo_pos];
> + if (s->rx_fifo_len > 0) {
> + s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
> + s->rx_fifo_len--;
> + qemu_chr_fe_accept_input(&s->chr);
> + }
> + break;
> +
> + case A_INTENSET:
> + case A_INTENCLR:
> + case A_INTEN:
> + r = s->reg[A_INTEN];
> + break;
> + default:
> + r = s->reg[addr];
> + break;
> + }
> +
> + return r;
> +}
> +
> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> + int r;
> +
> + s->watch_tag = 0;
> +
> + r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
This won't work if the host is big-endian. You need to do something like
uint8_t c = s->reg[A_TXD];
r = qemu_chr_fe_write(&s->chr, &c, 1);
> + if (r <= 0) {
> + s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> + uart_transmit, s);
> + if (!s->watch_tag) {
> + goto buffer_drained;
> + }
> + return FALSE;
> + }
> +
> +buffer_drained:
> + s->reg[A_TXDRDY] = 1;
> + nrf51_uart_update_irq(s);
> + return FALSE;
> +}
> +
> +static void uart_cancel_transmit(Nrf51UART *s)
> +{
> + if (s->watch_tag) {
> + g_source_remove(s->watch_tag);
> + s->watch_tag = 0;
> + }
> +}
> +
> +static void uart_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned int size)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + switch (addr) {
> + case A_TXD:
> + s->reg[A_TXD] = value;
> + uart_transmit(NULL, G_IO_OUT, s);
If you just call uart_transmit() here and we were already
in the process of transmitting a character, we'll end up
registering a second 'watch' and will leak the old one.
Sadly there isn't any guest-visible state that tracks
"there is a byte in the TX register that has not yet been sent",
so we'll have to have state that's not guest visible. You
can do this by having a new s->pending_tx_byte bool, whose
semantics should look like the R_STATE_TXFULL_MASK bit in
the hw/char/cmsdk-apb-uart.c UART:
* uart_transmit() does nothing if pending_tx_byte is false
* in uart_transmit(), when we write TXDRDY to 1 we also
set pending_tx_byte to false
* in this "write to TXD" code, if pending_tx_byte is true
we do nothing (because this UART doesn't tell the guest
about tx overruns); otherwise we set pending_tx_byte
to true and call uart_transmit()
* in the migration state's post_load function, if
pending_tx_byte is true then we register a watch on the
char frontend
(In the CMSDK uart we use a bit in a state register because
that UART happens to have guest-visible state that tracks
this, so we can avoid having an extra bool.)
> + break;
> + case A_INTENSET:
> + s->reg[A_INTEN] |= value;
> + break;
> + case A_INTENCLR:
> + s->reg[A_INTEN] &= ~value;
> + break;
> + case A_CTS ... A_RXTO:
> + s->reg[addr] = value;
> + nrf51_uart_update_irq(s);
ERRORSRC is write-1-to-clear, so needs to be specially handled.
RXD is read-only, so also needs handling.
> + default:
> + s->reg[addr] = value;
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps uart_ops = {
> + .read = uart_read,
> + .write = uart_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void nrf51_uart_reset(DeviceState *dev)
> +{
> + Nrf51UART *s = NRF51_UART(dev);
> +
> + uart_cancel_transmit(s);
> +
> + memset(s->reg, 0, sizeof(s->reg));
> +
> + s->rx_fifo_len = 0;
> + s->rx_fifo_pos = 0;
The PSEL* registers are documented as having an 0xffffffff reset value.
BAUDRATE also has a non-zero reset value.
> +}
> +
> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
> + return;
> + }
> +
> + s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> + s->rx_fifo_len++;
The 'size' argument to this function is a byte count of how
many bytes are in the buffer, so you should take as many
of them as will fill the fifo, not just one.
> +
> + s->reg[A_RXDRDY] = 1;
> + nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
Could you be consistent about whether you use sizeof(s->rx_fifo) or
UART_FIFO_LENGTH in checks on the fifo length, please?
> +}
> +
> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> + Nrf51UART *s = NRF51_UART(dev);
> +
> + qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> + NULL, NULL, s, NULL, true);
This UART supports detection of the 'break' condition. You
can implement that with a uart_event function (4th argument
here), which does the right thing when it gets a CHR_EVENT_BREAK.
(The data sheet says that a break means "set the FRAMING and
BREAK bits in the ERRORSRC register and raise ERROR.")
> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> + Nrf51UART *s = NRF51_UART(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> + "nrf51_soc.uart", 0x1000);
> + sysbus_init_mmio(sbd, &s->mmio);
> + sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static Property nrf51_uart_properties[] = {
> + DEFINE_PROP_CHR("chardev", Nrf51UART, chr),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void nrf51_uart_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = nrf51_uart_reset;
> + dc->realize = nrf51_uart_realize;
> + dc->props = nrf51_uart_properties;
All new devices should have a VMState description struct
and set dc->vmsd, so that they support state save/restore.
> +}
> +
> +static const TypeInfo nrf51_uart_info = {
> + .name = TYPE_NRF51_UART,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(Nrf51UART),
> + .instance_init = nrf51_uart_init,
> + .class_init = nrf51_uart_class_init
> +};
> +
> +static void nrf51_uart_register_types(void)
> +{
> + type_register_static(&nrf51_uart_info);
> +}
> +
> +type_init(nrf51_uart_register_types)
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index a6bbe9f108..a38b984675 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -24,6 +24,7 @@ typedef struct NRF51State {
> /*< public >*/
> char *kernel_filename;
> DeviceState *nvic;
> + DeviceState *uart;
>
> MemoryRegion iomem;
> } NRF51State;
> diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
> new file mode 100644
> index 0000000000..758203f1c3
> --- /dev/null
> +++ b/include/hw/char/nrf51_uart.h
> @@ -0,0 +1,54 @@
> +/*
> + * nRF51 SoC UART emulation
> + *
> + * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#ifndef NRF51_UART_H
> +#define NRF51_UART_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define UART_FIFO_LENGTH 6
> +
> +#define TYPE_NRF51_UART "nrf51_soc.uart"
> +#define NRF51_UART(obj) OBJECT_CHECK(Nrf51UART, (obj), TYPE_NRF51_UART)
> +
> +typedef struct Nrf51UART {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> + CharBackend chr;
> + qemu_irq irq;
> + guint watch_tag;
> +
> + uint8_t rx_fifo[UART_FIFO_LENGTH];
> + unsigned int rx_fifo_pos;
> + unsigned int rx_fifo_len;
> +
> + uint32_t reg[0x1000];
> +} Nrf51UART;
> +
> +static inline DeviceState *nrf51_uart_create(hwaddr addr,
> + qemu_irq irq,
> + Chardev *chr)
> +{
> + DeviceState *dev;
> + SysBusDevice *s;
> +
> + dev = qdev_create(NULL, "nrf51_soc.uart");
> + s = SYS_BUS_DEVICE(dev);
> + qdev_prop_set_chr(dev, "chardev", chr);
> + qdev_init_nofail(dev);
> + sysbus_mmio_map(s, 0, addr);
> + sysbus_connect_irq(s, 0, irq);
> +
> + return dev;
> +}
> +
> +#endif
> --
> 2.17.0
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-05-29 22:03 ` [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART Julia Suvorova
2018-05-31 9:42 ` Stefan Hajnoczi
2018-05-31 12:28 ` Peter Maydell
@ 2018-05-31 13:58 ` sundeep subbaraya
2018-06-01 10:41 ` Stefan Hajnoczi
2 siblings, 1 reply; 17+ messages in thread
From: sundeep subbaraya @ 2018-05-31 13:58 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Peter Maydell, Jim Mussared, Steffen Görtz,
Stefan Hajnoczi, Joel Stanley
Hi,
On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
<qemu-devel@nongnu.org> wrote:
> Basic implementation of nRF51 SoC UART.
> Description could be found here:
> http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
>
> The following features are not yet implemented:
> Control with SUSPEND/START*/STOP*
> CTS/NCTS flow control
> Mapping registers to pins
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
> hw/arm/nrf51_soc.c | 7 ++
> hw/char/Makefile.objs | 1 +
> hw/char/nrf51_uart.c | 232 +++++++++++++++++++++++++++++++++++
> include/hw/arm/nrf51_soc.h | 1 +
> include/hw/char/nrf51_uart.h | 54 ++++++++
> 5 files changed, 295 insertions(+)
> create mode 100644 hw/char/nrf51_uart.c
> create mode 100644 include/hw/char/nrf51_uart.h
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 6fe06dcfd2..a2ee6f3f3b 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -21,6 +21,7 @@
> #include "cpu.h"
>
> #include "hw/arm/nrf51_soc.h"
> +#include "hw/char/nrf51_uart.h"
>
> #define IOMEM_BASE 0x40000000
> #define IOMEM_SIZE 0x20000000
> @@ -34,6 +35,9 @@
> #define SRAM_BASE 0x20000000
> #define SRAM_SIZE (16 * 1024)
>
> +#define UART_BASE 0x40002000
> +#define UART_SIZE 0x1000
> +
> static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> {
> NRF51State *s = NRF51_SOC(dev_soc);
> @@ -73,6 +77,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> /* TODO: implement a cortex m0 and update this */
> s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +
> + s->uart = nrf51_uart_create(UART_BASE, qdev_get_gpio_in(s->nvic, 2),
> + serial_hd(0));
> }
>
> static Property nrf51_soc_properties[] = {
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 1b979100b7..1060c62a54 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -1,5 +1,6 @@
> common-obj-$(CONFIG_IPACK) += ipoctal232.o
> common-obj-$(CONFIG_ESCC) += escc.o
> +common-obj-$(CONFIG_NRF51_SOC) += nrf51_uart.o
> common-obj-$(CONFIG_PARALLEL) += parallel.o
> common-obj-$(CONFIG_PARALLEL) += parallel-isa.o
> common-obj-$(CONFIG_PL011) += pl011.o
> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> new file mode 100644
> index 0000000000..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * nRF51 SoC UART emulation
> + *
> + * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/registerfields.h"
> +#include "hw/char/nrf51_uart.h"
> +
> +REG32(STARTRX, 0x000)
> +REG32(STOPRX, 0x004)
> +REG32(STARTTX, 0x008)
> +REG32(STOPTX, 0x00C)
> +REG32(SUSPEND, 0x01C)
> +
> +REG32(CTS, 0x100)
> +REG32(NCTS, 0x104)
> +REG32(RXDRDY, 0x108)
> +REG32(TXDRDY, 0x11C)
> +REG32(ERROR, 0x124)
> +REG32(RXTO, 0x144)
> +
> +REG32(INTEN, 0x300)
> + FIELD(INTEN, CTS, 0, 1)
> + FIELD(INTEN, NCTS, 1, 1)
> + FIELD(INTEN, RXDRDY, 2, 1)
> + FIELD(INTEN, TXDRDY, 7, 1)
> + FIELD(INTEN, ERROR, 9, 1)
> + FIELD(INTEN, RXTO, 17, 1)
> +REG32(INTENSET, 0x304)
> +REG32(INTENCLR, 0x308)
> +REG32(ERRORSRC, 0x480)
> +REG32(ENABLE, 0x500)
> +REG32(PSELRTS, 0x508)
> +REG32(PSELTXD, 0x50C)
> +REG32(PSELCTS, 0x510)
> +REG32(PSELRXD, 0x514)
> +REG32(RXD, 0x518)
> +REG32(TXD, 0x51C)
> +REG32(BAUDRATE, 0x524)
> +REG32(CONFIG, 0x56C)
> +
> +static void nrf51_uart_update_irq(Nrf51UART *s)
> +{
> + unsigned int irq = 0;
> +
> + irq = irq || (s->reg[A_RXDRDY] && (s->reg[A_INTEN] & R_INTEN_RXDRDY_MASK));
> + irq = irq || (s->reg[A_TXDRDY] && (s->reg[A_INTEN] & R_INTEN_TXDRDY_MASK));
> + irq = irq || (s->reg[A_ERROR] && (s->reg[A_INTEN] & R_INTEN_ERROR_MASK));
> + irq = irq || (s->reg[A_RXTO] && (s->reg[A_INTEN] & R_INTEN_RXTO_MASK));
> +
> + qemu_set_irq(s->irq, !!irq);
> +}
> +
> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> + uint64_t r;
> +
> + switch (addr) {
> + case A_RXD:
> + r = s->rx_fifo[s->rx_fifo_pos];
> + if (s->rx_fifo_len > 0) {
> + s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
> + s->rx_fifo_len--;
> + qemu_chr_fe_accept_input(&s->chr);
> + }
> + break;
> +
> + case A_INTENSET:
> + case A_INTENCLR:
> + case A_INTEN:
> + r = s->reg[A_INTEN];
> + break;
> + default:
> + r = s->reg[addr];
You can use R_* macros for registers and access regs[ ] with addr/4 as index.
It is better than using big regs[ ] array out of which most of
locations go unused.
Say, for 0x4 register you are using regs[4] and regs[1], regs[2],
regs[3] remain unused.
Thanks,
Sundeep
> + break;
> + }
> +
> + return r;
> +}
> +
> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> + int r;
> +
> + s->watch_tag = 0;
> +
> + r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
> + if (r <= 0) {
> + s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> + uart_transmit, s);
> + if (!s->watch_tag) {
> + goto buffer_drained;
> + }
> + return FALSE;
> + }
> +
> +buffer_drained:
> + s->reg[A_TXDRDY] = 1;
> + nrf51_uart_update_irq(s);
> + return FALSE;
> +}
> +
> +static void uart_cancel_transmit(Nrf51UART *s)
> +{
> + if (s->watch_tag) {
> + g_source_remove(s->watch_tag);
> + s->watch_tag = 0;
> + }
> +}
> +
> +static void uart_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned int size)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + switch (addr) {
> + case A_TXD:
> + s->reg[A_TXD] = value;
> + uart_transmit(NULL, G_IO_OUT, s);
> + break;
> + case A_INTENSET:
> + s->reg[A_INTEN] |= value;
> + break;
> + case A_INTENCLR:
> + s->reg[A_INTEN] &= ~value;
> + break;
> + case A_CTS ... A_RXTO:
> + s->reg[addr] = value;
> + nrf51_uart_update_irq(s);
> + default:
> + s->reg[addr] = value;
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps uart_ops = {
> + .read = uart_read,
> + .write = uart_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void nrf51_uart_reset(DeviceState *dev)
> +{
> + Nrf51UART *s = NRF51_UART(dev);
> +
> + uart_cancel_transmit(s);
> +
> + memset(s->reg, 0, sizeof(s->reg));
> +
> + s->rx_fifo_len = 0;
> + s->rx_fifo_pos = 0;
> +}
> +
> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
> + return;
> + }
> +
> + s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> + s->rx_fifo_len++;
> +
> + s->reg[A_RXDRDY] = 1;
> + nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> + Nrf51UART *s = NRF51_UART(opaque);
> +
> + return (s->rx_fifo_len < sizeof(s->rx_fifo));
> +}
> +
> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> + Nrf51UART *s = NRF51_UART(dev);
> +
> + qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> + NULL, NULL, s, NULL, true);
> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> + Nrf51UART *s = NRF51_UART(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> + "nrf51_soc.uart", 0x1000);
> + sysbus_init_mmio(sbd, &s->mmio);
> + sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static Property nrf51_uart_properties[] = {
> + DEFINE_PROP_CHR("chardev", Nrf51UART, chr),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void nrf51_uart_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = nrf51_uart_reset;
> + dc->realize = nrf51_uart_realize;
> + dc->props = nrf51_uart_properties;
> +}
> +
> +static const TypeInfo nrf51_uart_info = {
> + .name = TYPE_NRF51_UART,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(Nrf51UART),
> + .instance_init = nrf51_uart_init,
> + .class_init = nrf51_uart_class_init
> +};
> +
> +static void nrf51_uart_register_types(void)
> +{
> + type_register_static(&nrf51_uart_info);
> +}
> +
> +type_init(nrf51_uart_register_types)
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index a6bbe9f108..a38b984675 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -24,6 +24,7 @@ typedef struct NRF51State {
> /*< public >*/
> char *kernel_filename;
> DeviceState *nvic;
> + DeviceState *uart;
>
> MemoryRegion iomem;
> } NRF51State;
> diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
> new file mode 100644
> index 0000000000..758203f1c3
> --- /dev/null
> +++ b/include/hw/char/nrf51_uart.h
> @@ -0,0 +1,54 @@
> +/*
> + * nRF51 SoC UART emulation
> + *
> + * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#ifndef NRF51_UART_H
> +#define NRF51_UART_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define UART_FIFO_LENGTH 6
> +
> +#define TYPE_NRF51_UART "nrf51_soc.uart"
> +#define NRF51_UART(obj) OBJECT_CHECK(Nrf51UART, (obj), TYPE_NRF51_UART)
> +
> +typedef struct Nrf51UART {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> + CharBackend chr;
> + qemu_irq irq;
> + guint watch_tag;
> +
> + uint8_t rx_fifo[UART_FIFO_LENGTH];
> + unsigned int rx_fifo_pos;
> + unsigned int rx_fifo_len;
> +
> + uint32_t reg[0x1000];
> +} Nrf51UART;
> +
> +static inline DeviceState *nrf51_uart_create(hwaddr addr,
> + qemu_irq irq,
> + Chardev *chr)
> +{
> + DeviceState *dev;
> + SysBusDevice *s;
> +
> + dev = qdev_create(NULL, "nrf51_soc.uart");
> + s = SYS_BUS_DEVICE(dev);
> + qdev_prop_set_chr(dev, "chardev", chr);
> + qdev_init_nofail(dev);
> + sysbus_mmio_map(s, 0, addr);
> + sysbus_connect_irq(s, 0, irq);
> +
> + return dev;
> +}
> +
> +#endif
> --
> 2.17.0
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-05-31 13:58 ` sundeep subbaraya
@ 2018-06-01 10:41 ` Stefan Hajnoczi
2018-06-01 10:44 ` Stefan Hajnoczi
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2018-06-01 10:41 UTC (permalink / raw)
To: sundeep subbaraya
Cc: Julia Suvorova, qemu-devel, Peter Maydell, Jim Mussared,
Steffen Görtz, Joel Stanley
On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
<sundeep.lkml@gmail.com> wrote:
> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
> <qemu-devel@nongnu.org> wrote:
>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> + Nrf51UART *s = NRF51_UART(opaque);
>> + uint64_t r;
>> +
>> + switch (addr) {
>> + case A_RXD:
>> + r = s->rx_fifo[s->rx_fifo_pos];
>> + if (s->rx_fifo_len > 0) {
>> + s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>> + s->rx_fifo_len--;
>> + qemu_chr_fe_accept_input(&s->chr);
>> + }
>> + break;
>> +
>> + case A_INTENSET:
>> + case A_INTENCLR:
>> + case A_INTEN:
>> + r = s->reg[A_INTEN];
>> + break;
>> + default:
>> + r = s->reg[addr];
>
> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
> It is better than using big regs[ ] array out of which most of
> locations go unused.
Good point. The bug is more severe than an inefficiency.
s->reg[addr] allows out-of-bounds accesses. This is a security bug.
The memory region is 0x1000 *bytes* long, but the array has 0x1000
32-bit *elements*. A read from address 0xfffc results in a memory
load from s->reg + 0xfffc * sizeof(s->reg[0]). That's beyond the end
of the array!
s->reg[A_*] should be changed to s->reg[R_*]. s->reg[addr] needs to
be s->reg[addr / sizeof(s->reg[0])].
It may be worth adding a warning to scripts/checkpatch.pl for
array[A_*] so this bug is reported automatically in the future.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-06-01 10:41 ` Stefan Hajnoczi
@ 2018-06-01 10:44 ` Stefan Hajnoczi
2018-06-01 15:36 ` Julia Suvorova
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2018-06-01 10:44 UTC (permalink / raw)
To: sundeep subbaraya
Cc: Julia Suvorova, qemu-devel, Peter Maydell, Jim Mussared,
Steffen Görtz, Joel Stanley
On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
> <sundeep.lkml@gmail.com> wrote:
>> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
>> <qemu-devel@nongnu.org> wrote:
>>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> + Nrf51UART *s = NRF51_UART(opaque);
>>> + uint64_t r;
>>> +
>>> + switch (addr) {
>>> + case A_RXD:
>>> + r = s->rx_fifo[s->rx_fifo_pos];
>>> + if (s->rx_fifo_len > 0) {
>>> + s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>>> + s->rx_fifo_len--;
>>> + qemu_chr_fe_accept_input(&s->chr);
>>> + }
>>> + break;
>>> +
>>> + case A_INTENSET:
>>> + case A_INTENCLR:
>>> + case A_INTEN:
>>> + r = s->reg[A_INTEN];
>>> + break;
>>> + default:
>>> + r = s->reg[addr];
>>
>> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
>> It is better than using big regs[ ] array out of which most of
>> locations go unused.
>
> Good point. The bug is more severe than an inefficiency.
> s->reg[addr] allows out-of-bounds accesses. This is a security bug.
>
> The memory region is 0x1000 *bytes* long, but the array has 0x1000
> 32-bit *elements*. A read from address 0xfffc results in a memory
> load from s->reg + 0xfffc * sizeof(s->reg[0]). That's beyond the end
> of the array!
Sorry, I was wrong. The array is large enough after all. It's just
an inefficiency, but still worth fixing. Similar issues could lead to
out-of-bound accesses.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART
2018-06-01 10:44 ` Stefan Hajnoczi
@ 2018-06-01 15:36 ` Julia Suvorova
0 siblings, 0 replies; 17+ messages in thread
From: Julia Suvorova @ 2018-06-01 15:36 UTC (permalink / raw)
To: Stefan Hajnoczi, sundeep subbaraya
Cc: qemu-devel, Peter Maydell, Jim Mussared, Steffen Görtz,
Joel Stanley
On 01.06.2018 13:44, Stefan Hajnoczi wrote:
> On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
>> <sundeep.lkml@gmail.com> wrote:
>>> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
>>> <qemu-devel@nongnu.org> wrote:
>>>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>>>> +{
>>>> + Nrf51UART *s = NRF51_UART(opaque);
>>>> + uint64_t r;
>>>> +
>>>> + switch (addr) {
>>>> + case A_RXD:
>>>> + r = s->rx_fifo[s->rx_fifo_pos];
>>>> + if (s->rx_fifo_len > 0) {
>>>> + s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>>>> + s->rx_fifo_len--;
>>>> + qemu_chr_fe_accept_input(&s->chr);
>>>> + }
>>>> + break;
>>>> +
>>>> + case A_INTENSET:
>>>> + case A_INTENCLR:
>>>> + case A_INTEN:
>>>> + r = s->reg[A_INTEN];
>>>> + break;
>>>> + default:
>>>> + r = s->reg[addr];
>>>
>>> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
>>> It is better than using big regs[ ] array out of which most of
>>> locations go unused.
>>
>> Good point. The bug is more severe than an inefficiency.
>> s->reg[addr] allows out-of-bounds accesses. This is a security bug.
>>
>> The memory region is 0x1000 *bytes* long, but the array has 0x1000
>> 32-bit *elements*. A read from address 0xfffc results in a memory
>> load from s->reg + 0xfffc * sizeof(s->reg[0]). That's beyond the end
>> of the array!
>
> Sorry, I was wrong. The array is large enough after all. It's just
> an inefficiency, but still worth fixing. Similar issues could lead to
> out-of-bound accesses.
Even if I change the reg array to work with the R_* macros, it will
still be inefficient, since registers are not sequentially located.
I can define the registers in the Nrf51UART structure separately, but
this will increase the code size. Is there a proper way?
Best regards, Julia Suvorova.
^ permalink raw reply [flat|nested] 17+ messages in thread