[Qemu-devel] [RFC] armv7m_nvic

[Qemu-devel] [RFC] armv7m_nvic

[Liviu Ionescu]

while working on the Cortex-M framework, I noticed that the current armv7m_nvic implementation is a bit old, and, to my opinion, not very fortunately structured.

first, it is named NVIC, but implements not only the NVIC registers, but some other register groups, like SysTick, system control, some IDs.

if the QEMU rule is to implement a physical contiguous memory space (0xE000E000 - 0xE000EFFF) in one file, then probably the file should be named armv7m_scb.c (System Control Block), and it must implement all registers.

the modular alternative (that I generally favour) would be to split the memory space into functional slices (SystemControl, SysTick, NVIC, CPUID, MPU, Debug, SW Triggers, MCUIDs) and implement them as separate objects, probably in separate files.


the changes that I might need in QEMU might also require some updates in the exception processing and might also affect NVIC, so if you have any comments on how the current structure should be improved, I'm open to suggestions.


regards,

Liviu

Re: [Qemu-devel] [RFC] armv7m_nvic

[Peter Maydell]

On 4 June 2015 at 13:45, Liviu Ionescu <ilg@livius.net> wrote:
> while working on the Cortex-M framework, I noticed that the current
> armv7m_nvic implementation is a bit old, and, to my opinion, not
> very fortunately structured.
>
> first, it is named NVIC, but implements not only the NVIC registers,
> but some other register groups, like SysTick, system control, some IDs.
>
> if the QEMU rule is to implement a physical contiguous memory space
> (0xE000E000 - 0xE000EFFF) in one file, then probably the file should
> be named armv7m_scb.c (System Control Block), and it must implement
> all registers.
>
> the modular alternative (that I generally favour) would be to split
> the memory space into functional slices (SystemControl, SysTick,
> NVIC, CPUID, MPU, Debug, SW Triggers, MCUIDs) and implement them as
> separate objects, probably in separate files.

Yes, the current structure is not the ideal one. It dates back to
before we had support for constructing memory regions by overlaying
multiple different devices, which is why it combines more things
than it ought to. I split some of the GIC memory regions out a
few years back, but there are more parts that could also be
separated. Splitting out Systick definitely makes sense.
Some of the rest should probably be part of the CPU object itself:
if the implementation is just reading and writing fields in
the ARM CPU state struct then probably the memory-mapped
register interface should be in the CPU object.
(We can now have the CPU object provide memory regions, because it's
a device object itself. This is another bit of API that wasn't
available to the original armv7m_nvic author.)

On the MPU, if you need an emulation of that you should look
at Peter Crosthwaite's Cortex-R5 patchset, which needs a v7PMSA
for the R profile core. The interface is obviously different
(cpregs rather than memory-mapped registers) but the underlying
code for doing address translation should be the same I think.

> the changes that I might need in QEMU might also require some
> updates in the exception processing and might also affect NVIC,
> so if you have any comments on how the current structure should
> be improved, I'm open to suggestions.

The current code borrows heavily from the ARM GIC implementation,
presumably because it was convenient at the time. I don't think
this is actually correct, because the M profile exception handling
is much more tightly coupled to the CPU and includes assigning
priorities to exceptions (BusFault, UsageFault and the like) and
dealing with those in the same way as external interrupts.
Depending on how accurate you need your emulation to be, it may
turn out to be necessary to bite the bullet and reimplement the
NVIC properly as an independent bit of code. (Of course if you
don't need to fix the problems our current implementation causes
you can leave it the way it is.)

thanks
-- PMM

Re: [Qemu-devel] [RFC] armv7m_nvic

[Liviu Ionescu]

> On 04 Jun 2015, at 16:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ... Splitting out Systick definitely makes sense.

as a first step, I would try to define a separate object (armv7m-systick) and copy/paste the access code.

then refer it inside the existing NVIC, to avoid affecting existing functionality.

it would be a real time saver if you could provide a hint on the best approach to map the SysTick memory range (E010-E0FF) over the existing NVIC, inside the current code.

> On the MPU...

I would use a similar approach, a separate object, at first mapped over the ED90-EDEF range inside the existing NVIC.

I also need to understand how to configure the memory range, to access the MPU registers, but since similar implementations are already in, it shouldn't be that hard.

> ... reimplement the
> NVIC properly as an independent bit of code.

if the current exception handling implementation is not accurate enough for my needs, I'll consider this.


regards,

Liviu

Re: [Qemu-devel] [RFC] armv7m_nvic

[Peter Maydell]

On 4 June 2015 at 19:17, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 04 Jun 2015, at 16:03, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> ... Splitting out Systick definitely makes sense.
>
> as a first step, I would try to define a separate object (armv7m-systick) and copy/paste the access code.
>
> then refer it inside the existing NVIC, to avoid affecting existing functionality.

Sounds good -- systick should be fairly small and self contained.

Eventually the container bit should probably move out to be
hw/arm/armv7m.c (which then instantiates separate system
regs, NVIC and systick and assembles them into the right
places in the memory map). That will require converting armv7m.c
into a proper QOM container device, though, so we can do the
systick bit first.

> it would be a real time saver if you could provide a hint on the best approach to map the SysTick memory range (E010-E0FF) over the existing NVIC, inside the current code.

The basic approach is to use memory_region_add_subregion()
to add the MemoryRegion for the systick object in the right place
in the container region. We do this already in armv7m_nvic_realize()
for the GIC region, though that's a bit funny because we have
direct access to its MemoryRegions. Look at hw/cpu/a9mpcore.c
for an example of building up a container using MemoryRegions
exposed by SysBus devices. (The sysbus device uses sysbus_init_mmio()
in its realize function to export memory regions. Then the
container device's realize function can create and realize the
smaller devices, and use sysbus_mmio_get_region() to get the
MemoryRegion* it needs to pass to memory_region_add_subregion().)

>> On the MPU...
>
> I would use a similar approach, a separate object, at first mapped
> over the ED90-EDEF range inside the existing NVIC.

My strong initial impression is that we should do this by having
the CPU object expose a MemoryRegion, which you can then map
over the right range in the NVIC. I don't think it makes sense
for the MPU to be a completely separate device object, because all
the actual implementation is (and must be) in the CPU.

-- PMM

Re: [Qemu-devel] [RFC] armv7m_nvic

[Liviu Ionescu]

> On 05 Jun 2015, at 00:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ... systick should be fairly small and self contained.
> 
> ... My strong initial impression is that we should do this by having
> the CPU object expose a MemoryRegion, which you can then map
> over the right range in the NVIC. I don't think it makes sense
> for the MPU to be a completely separate device object, because all
> the actual implementation is (and must be) in the CPU.

ok, I'll try to modularise the existing armv7m_nvic.c file and then we'll decide how to organise these pieces.

I'll add it to my TODO list, but with medium priority, I'm currently working on adding the necessary clock registers to make the CMSIS initialisations pass (on STM32 processors first).

 
regards,

Liviu