[Qemu-devel] [RFC 0/3] MultiCPU changes

[Qemu-devel] [RFC 0/3] MultiCPU changes

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Those are changes that I made to create SoC model with 2xCortexA9
and Cortex-M3. Generally all are done to allow move work further,
not to upstreaming (especially patch 3). I would like to start
discossion if those modification could be upstreamed.

Marcin Krzeminski (3):
  nvic: Add CPU numebr property
  ARM: boot: Add option to skip FDT memory node update
  exec: Allow to change Address Space from board level

 exec.c                |  8 ++++++++
 hw/arm/boot.c         | 29 ++++++++++++++++-------------
 hw/intc/armv7m_nvic.c | 24 ++++++++++++++++++------
 include/hw/arm/arm.h  |  5 +++++
 include/qom/cpu.h     |  1 +
 5 files changed, 48 insertions(+), 19 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC 1/3] nvic: Add CPU numebr property

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

In case of MultiCPU SoC M3 is not always CPU0.
This commit add cpu_id property to allow set CPU
number for NVIC model. Also address space that this used
by NVIC is updated to mach CPU's one.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/intc/armv7m_nvic.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 06d8db6..66f0f70 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -34,6 +34,7 @@ typedef struct {
     MemoryRegion container;
     uint32_t num_irq;
     qemu_irq sysresetreq;
+    uint32_t cpu_id;
 } nvic_state;
 
 #define TYPE_NVIC "armv7m_nvic"
@@ -187,11 +188,11 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
     case 0x1c: /* SysTick Calibration Value.  */
         return 10000;
     case 0xd00: /* CPUID Base.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
+        cpu = ARM_CPU(qemu_get_cpu(s->cpu_id));
         return cpu->midr;
     case 0xd04: /* Interrupt Control State.  */
         /* VECTACTIVE */
-        cpu = ARM_CPU(qemu_get_cpu(0));
+        cpu = ARM_CPU(qemu_get_cpu(s->cpu_id));
         val = cpu->env.v7m.exception;
         if (val == 1023) {
             val = 0;
@@ -222,7 +223,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
             val |= (1 << 31);
         return val;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
+        cpu = ARM_CPU(qemu_get_cpu(s->cpu_id));
         return cpu->env.v7m.vecbase;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
         return 0xfa050000;
@@ -349,7 +350,7 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         }
         break;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
+        cpu = ARM_CPU(qemu_get_cpu(s->cpu_id));
         cpu->env.v7m.vecbase = value & 0xffffff80;
         break;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
@@ -453,6 +454,11 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
                   "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
 }
 
+static Property nvic_properties[] = {
+    DEFINE_PROP_UINT32("cpu-id", nvic_state, cpu_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static const MemoryRegionOps nvic_sysreg_ops = {
     .read = nvic_sysreg_read,
     .write = nvic_sysreg_write,
@@ -461,13 +467,14 @@ static const MemoryRegionOps nvic_sysreg_ops = {
 
 static const VMStateDescription vmstate_nvic = {
     .name = "armv7m_nvic",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(systick.control, nvic_state),
         VMSTATE_UINT32(systick.reload, nvic_state),
         VMSTATE_INT64(systick.tick, nvic_state),
         VMSTATE_TIMER_PTR(systick.timer, nvic_state),
+	VMSTATE_UINT32(cpu_id, nvic_state),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -494,6 +501,7 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     nvic_state *s = NVIC(dev);
     NVICClass *nc = NVIC_GET_CLASS(s);
     Error *local_err = NULL;
+    CPUState *cpustate;
 
     /* The NVIC always has only one CPU */
     s->gic.num_cpu = 1;
@@ -531,7 +539,10 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     /* Map the whole thing into system memory at the location required
      * by the v7M architecture.
      */
-    memory_region_add_subregion(get_system_memory(), 0xe000e000, &s->container);
+
+    cpustate = qemu_get_cpu(s->cpu_id);
+    memory_region_add_subregion(cpustate->as->root, 0xe000e000, &s->container);
+
     s->systick.timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, systick_timer_tick, s);
 }
 
@@ -564,6 +575,7 @@ static void armv7m_nvic_class_init(ObjectClass *klass, void *data)
     dc->vmsd  = &vmstate_nvic;
     dc->reset = armv7m_nvic_reset;
     dc->realize = armv7m_nvic_realize;
+    dc->props = nvic_properties;
 }
 
 static const TypeInfo armv7m_nvic_info = {
-- 
2.7.4

[Qemu-devel] [RFC 2/3] ARM: boot: Add option to skip FDT memory node update

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

When Qemu boots directly ARM kernel the memory node in device
tree is automatically updated to mach guest RAM size assigned
to Qemu. This commit allow use case when user do not
want to pass all guest RAM to linux kernel by skipping
device tree mmory node update.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/arm/boot.c        | 29 ++++++++++++++++-------------
 include/hw/arm/arm.h |  5 +++++
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ff621e4..0af72bd 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -489,21 +489,24 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     } else {
         Error *err = NULL;
 
-        rc = fdt_path_offset(fdt, "/memory");
-        if (rc < 0) {
-            qemu_fdt_add_subnode(fdt, "/memory");
-        }
+        if (!binfo->skip_fdt_mem_node) {
 
-        if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) {
-            qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
-        }
+            rc = fdt_path_offset(fdt, "/memory");
+            if (rc < 0) {
+                qemu_fdt_add_subnode(fdt, "/memory");
+            }
 
-        rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
-                                          acells, binfo->loader_start,
-                                          scells, binfo->ram_size);
-        if (rc < 0) {
-            fprintf(stderr, "couldn't set /memory/reg\n");
-            goto fail;
+            if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) {
+                qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
+            }
+
+            rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
+                                              acells, binfo->loader_start,
+                                              scells, binfo->ram_size);
+            if (rc < 0) {
+                fprintf(stderr, "couldn't set /memory/reg\n");
+                goto fail;
+            }
         }
     }
 
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index aeeebfe..e237930 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -112,6 +112,11 @@ struct arm_boot_info {
     bool secure_board_setup;
 
     arm_endianness endianness;
+
+    /* Do not override memory node in device tree - useful when
+     * not whole guest memory is under Linux control.
+     */
+    bool skip_fdt_mem_node;
 };
 
 /**
-- 
2.7.4

[Qemu-devel] [RFC 3/3] exec: Allow to change Address Space from board level

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

By introduction of CPUAddressSpace table it is not possible
to change address space for CPU from board code. In my case
Cortex-M3 core has it own address space that is created at board
level, then updated by changing cpu->as. For current code cpu->as
is used only at init, so changing it does not change actual
address space.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 exec.c            | 8 ++++++++
 include/qom/cpu.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/exec.c b/exec.c
index 08c558e..9c6e302 100644
--- a/exec.c
+++ b/exec.c
@@ -628,6 +628,14 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
     /* Return the AddressSpace corresponding to the specified index */
     return cpu->cpu_ases[asidx].as;
 }
+
+void cpu_set_address_space(CPUState *cpu, AddressSpace *as, int asidx)
+{
+    /* Return the AddressSpace corresponding to the specified index */
+    if (asidx < cpu->num_ases) {
+        cpu->cpu_ases[asidx].as = as;
+    }
+}
 #endif
 
 void cpu_exec_unrealizefn(CPUState *cpu)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3f79a8e..7e2ead8 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -964,6 +964,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
  * specifies which address space to read.
  */
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
+void cpu_set_address_space(CPUState *cpu, AddressSpace *as, int asidx);
 
 void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
-- 
2.7.4

Re: [Qemu-devel] [RFC 0/3] MultiCPU changes

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [RFC 0/3] MultiCPU changes
Message-id: 1481894559-13974-1-git-send-email-marcin.krzeminski@nokia.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1481894559-13974-1-git-send-email-marcin.krzeminski@nokia.com -> patchew/1481894559-13974-1-git-send-email-marcin.krzeminski@nokia.com
Switched to a new branch 'test'
a483da8 exec: Allow to change Address Space from board level
2889528 ARM: boot: Add option to skip FDT memory node update
7a4aa5a nvic: Add CPU numebr property

=== OUTPUT BEGIN ===
Checking PATCH 1/3: nvic: Add CPU numebr property...
ERROR: code indent should never use tabs
#82: FILE: hw/intc/armv7m_nvic.c:477:
+^IVMSTATE_UINT32(cpu_id, nvic_state),$

total: 1 errors, 0 warnings, 87 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/3: ARM: boot: Add option to skip FDT memory node update...
WARNING: line over 80 characters
#44: FILE: hw/arm/boot.c:500:
+                qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");

total: 0 errors, 1 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/3: exec: Allow to change Address Space from board level...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC 1/3] nvic: Add CPU numebr property

[Peter Maydell]

On 16 December 2016 at 13:22,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> In case of MultiCPU SoC M3 is not always CPU0.
> This commit add cpu_id property to allow set CPU
> number for NVIC model. Also address space that this used
> by NVIC is updated to mach CPU's one.
>
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>

The right fix for this I think is probably for the NVIC to
have an actual pointer to its CPU object, which the armv7m_init
code sets up (perhaps with QOM links?) when it creates the CPU
and the NVIC. The two really do need to be closely coupled,
and the CPU already has a pointer to its NVIC. There's
no need for the NVIC to get the CPU pointer by looking it up
by CPU ID.

Once I've got the GICv3 virtualization out of the way, the
next thing on my todo list is to pick up the NVIC refactoring
that Michael Davidsaver posted last year:
 https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00504.html
which makes some moves in that direction (though it doesn't
init the NVIC's CPU pointer with a link, it just assumes
first_cpu).

thanks
-- PMM

[Qemu-devel] Odp.: [RFC 1/3] nvic: Add CPU numebr property

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Hi Peter,

sorry for late response - I missed your email.

> On 16 December 2016 at 13:22,  <marcin.krzeminski@nokia.com> wrote:
>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>
>> In case of MultiCPU SoC M3 is not always CPU0.
>> This commit add cpu_id property to allow set CPU
>> number for NVIC model. Also address space that this used
>> by NVIC is updated to mach CPU's one.
>>
>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> The right fix for this I think is probably for the NVIC to
> have an actual pointer to its CPU object, which the armv7m_init
> code sets up (perhaps with QOM links?) when it creates the CPU
> and the NVIC. The two really do need to be closely coupled,
> and the CPU already has a pointer to its NVIC. There's
> no need for the NVIC to get the CPU pointer by looking it up
> by CPU ID.

I was thinking about similar solution (maybe the same as you did in GIC v3).
Generally this should be done automatically without any user interact.
>
> Once I've got the GICv3 virtualization out of the way, the
> next thing on my todo list is to pick up the NVIC refactoring
> that Michael Davidsaver posted last year:
>  https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00504.html
> which makes some moves in that direction (though it doesn't
> init the NVIC's CPU pointer with a link, it just assumes
> first_cpu).

That is good news ;]. I think we can postpone this work until NVIC
refactoring will be finished.

Thanks,
Marcin
>
> thanks
> -- PMM
>