On 27 June 2018 at 10:44, Stefan Hajnoczi <stefa...@gmail.com> 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 <cont...@steffen-goertz.de>
>> + *
>> + * 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 <cont...@steffen-goertz.de>
>> + *
>> + * 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
