Re: [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories

[Stefan Hajnoczi]

On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<address@hidden> wrote:
> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> +    offset >>= 2;
> +
> +    return s->uicr_content[offset];
> +}
> +
> +static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
> +        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;
> +    }

There is asymmetry here: uicr_read() doesn't check offset before
indexing the array but uicr_write() does.  Either the check is
necessary or it's not.

I think this check isn't necessary since the memory region is sized
appropriately:

  memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
          sizeof(s->uicr_content));

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

Can this be done in ->realize()?  Nothing changes the contents of
empty_page, so a ->reset() function seems unnecessary.