Re: [Qemu-devel] [RFC v3 1/2] arm: Add NRF51 SOC non-volatile memory controller

[Stefan Hajnoczi]

On Thu, Jul 12, 2018 at 12:12:18PM +0200, Steffen Görtz wrote:
> Changes in V3:
> - NVMs consolidated in one file
> - Removed bitfields
> - Add VMState
> - Add reset
> 
> Changes in V1/V2:
> - Code style changes
> 
> Signed-off-by: Steffen Görtz <address@hidden>
> ---

Everything above '---' goes into the commit description and is
permanently in the git log.  Changelogs are not useful in the git log
since their context (the earlier revisions and the discussion) is
missing.  Please put changelogs below '---' as anything there doesn't
get included in the commit description.

> +static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    if (offset > (ARRAY_SIZE(ficr_content) - size)) {

Bug alert, the semantic types look suspicious:

  bytes_t > num_elements_t - bytes_t

You cannot subtract a byte count from the number of elements in an
array.

Did you mean:

  if (offset >= sizeof(ficr_content)) {

MemoryRegion was already declared with FICR_SIZE length, so nothing
should ever call ficr_read() with an out-of-bounds offset.

Instead of defining FICR_SIZE, which could become out-of-sync with the
definition of ficr_content[], I would use sizeof(ficr_content) as the
MemoryRegion length.  This way the code is guaranteed to be safe even if
the size of ficr_content[] is changed in the future.  No if statement
would be necessary.

> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> +    offset >>= 2;
> +
> +    if (offset >= ARRAY_SIZE(s->uicr_content)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, 
> offset);
> +        return 0;
> +    }

The same applies to NRF51_UICR_SIZE.  It's simpler to use
sizeof(s->uicr_content) instead of defining a separate constant and then
using an if statement to check the offset.

> +static void nrf51_nvm_init(Object *obj)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
> +            NRF51_NVMC_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +
> +    memory_region_init_io(&s->ficr, NULL, &ficr_ops, s, "nrf51_soc.ficr",
> +            NRF51_FICR_SIZE);
> +    memory_region_set_readonly(&s->ficr, true);
> +    sysbus_init_mmio(sbd, &s->ficr);
> +
> +    memcpy(s->uicr_content, uicr_fixture, sizeof(s->uicr_content));

uicr_content[] persists across device reset?  Doing it that way seems
fine to me, but I wanted to check that it's what you intended.

> +static void nrf51_nvm_reset(DeviceState *dev)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(dev);
> +
> +    s->empty_page = g_malloc(s->page_size);
> +    memset(s->empty_page, 0xFF, s->page_size);
> +}

This function can be called multiple times and will leak the previous
s->empty_page.  It's easier to allocate s->empty_page in
nrf51_nvm_realize() instead.

> +
> +
> +static Property nrf51_nvm_properties[] = {
> +    DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
> +    DEFINE_PROP_UINT32("code_size", Nrf51NVMState, code_size, 0x100),

There is an intense battle between '-' and '_' for property names:

$ git grep 'DEFINE_PROP[^(]\+("[^"]\+_' | wc -l
234
$ git grep 'DEFINE_PROP[^(]\+("[^"]\+-' | wc -l
394

I like '-' is because -device foo,my-option=x is more consistent with
commonly-used properties than -device foo,my_option=x, but there are
plenty of properties with '_' lurking around too.

Up to you :).

> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 35dd71c3db..9c32d22abe 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -1,5 +1,5 @@
>  /*
> - * Nordic Semiconductor nRF51  SoC
> + * Nordic Semiconductor nRF51 SoC

Unrelated to this patch.  Please drop.

signature.asc
Description: PGP signature