Re: [Qemu-devel] [RFC v2] arm: Add NRF51 SOC non-volatile memory controller

[Peter Maydell]

On 27 June 2018 at 10:44, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
>> Changes since V1:
>> - Code style changes
>
> Please put the changelog below '---'.
>
>> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
>> new file mode 100644
>> index 0000000000..5dde3088a8
>> --- /dev/null
>> +++ b/hw/nvram/nrf51_nvmc.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * nrf51_nvmc.c
>> + *
>> + * Copyright 2018 Steffen Görtz <address@hidden>
>> + *
>> + * This code is licensed under the GPL version 2 or later.  See
>> + * the COPYING file in the top-level directory.
>> + */
>
> Please mention the full name of the peripheral so its purpose is clear
> and include a link to the datasheet.
>
> It's convenient to have this information in the .c file, since that's
> where the majority of code changes happen.
>
>> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
>> new file mode 100644
>> index 0000000000..3a63b7e5ad
>> --- /dev/null
>> +++ b/include/hw/nvram/nrf51_nvmc.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * nrf51_nvmc.h
>> + *
>> + * Copyright 2018 Steffen Görtz <address@hidden>
>> + *
>> + * This code is licensed under the GPL version 2 or later.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
>> + * See Nrf51 product sheet 8.22 NVMC specifications
>> + *
>> + * QEMU interface:
>> + * + sysbus MMIO regions 0: Memory Region with registers
>> + *   to be mapped to the peripherals instance address by the SOC.
>> + * + page_size property to set the page size in bytes.
>> + * + code_size property to set the code size in number of pages.
>> + *
>> + * Accuracy of the peripheral model:
>> + * + The NVMC is always ready, all requested erase operations succeed
>> + *   immediately.
>> + * + CONFIG.WEN and CONFIG.EEN flags can be written and read back
>> + *   but are not evaluated to check whether a requested write/erase 
>> operation
>> + *   is legal.
>
> Checking CONFIG.EEN would be easy, please do it.
>
> Peter: CONFIG.WEN determines whether stores to the flash memory region
> are allowed.  What is the best way to implement this protection?

What accesses do they gate? If this is just accesses that go
via the io_read() and io_write() functions then this is easy:
just put a check in those functions in the appropriate place.
The harder case is if they affect accesses to a region that
is implemented as an MMIO ram region which can be executed from;
that can be done, but gets annoyingly complicated:
 * unmap the RAM and map an MMIO region which handles the errors
   (only possible if the config bit gates the reads as well)
 * use a ROM device memory region (only possible if reads are
   always ok but writes might not be
 * use the IOMMU APIs (maximum flexibility, maximum pain)

A quick scan of the code suggests you're in the easy case
where you can just have io_read() and io_write() handle
the config switch checks, though.

>> +    MemoryRegion *mr;
>> +    AddressSpace as;
>
> I remember you said you tried
> address_space_read/write(get_system_memory(), ...) but it didn't work.
> Did you figure out why?

Devices directly talking to get_system_memory() isn't a great
design anyway.

It's not clear to me what this device is doing with its
AddressSpace, though; comments that went into more detail
about what the device is and what the "memory" property is
for might help.

>> +    struct {
>> +        uint32_t config:2;
>> +    } state;
>
> I noticed you used bit-fields in recent patches.  They are rarely-used
> in QEMU.

Agreed that avoiding bitfields is preferable.

thanks
-- PMM