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

[Stefan Hajnoczi]

On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
> 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.

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