On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote: > 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.
Thanks! > >> + 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. I understand this issue now. It's the same as qtest. This device is only visible from cpu->as, it's not visible from address_space_memory (system_memory). The NVMC needs access to the SoC's flash memory region, and that's what mr/as achieve here. I agree that comments explaining the purpose of mr/as would be useful.
signature.asc
Description: PGP signature
