[Qemu-devel] [PATCH 0/5] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Qemu-devel] [PATCH 0/5] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Greg Bellows]

Added support for running an AArch32 guest on a AArch64 KVM host.  Support has
only been added to the QEMU machvirt machine.  The addition of CPU properties
specifiable from the command line were added to allow disablement of AArch64
execution state hereby forcing EL1 to be AArch32.  The new CPU command line
propoerty is "-aarch64" that is specified as follows:

    aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57,-aarch64 ...

Support also added to support uncompressed images (Image) for aarch32.

Greg Bellows (5):
  target-arm: Add ARM CPU feature parsing
  target-arm: Add feature parsing to virt
  target-arm: Add 32/64-bit register sync
  target-arm: Add AArch32 guest support to KVM64
  target-arm: Adjust kernel load address for Image

 hw/arm/boot.c           | 33 +++++++++++++++++-
 hw/arm/virt.c           | 21 ++++++++++--
 target-arm/cpu.c        | 45 ++++++++++++++++++++++++-
 target-arm/helper-a64.c |  5 +--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/kvm64.c      | 21 +++++++++---
 target-arm/op_helper.c  |  6 ++--
 7 files changed, 204 insertions(+), 16 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Greg Bellows]

Adds a CPU feature parsing function and assigns to the CPU class.  The only
feature added was "-aarch64" which disabled the AArch64 execution state on a
64-bit ARM CPU.

Also adds stripping of features from CPU model string in acquiring the ARM CPU
by name.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 285947f..f327dd7 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
     char *typename;
+    char *cpuname;
 
     if (!cpu_model) {
         return NULL;
     }
 
-    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
+    cpuname = g_strdup(cpu_model);
+    cpuname = strtok(cpuname, ",");
+    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
     oc = object_class_by_name(typename);
+    g_free(cpuname);
     g_free(typename);
     if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
         object_class_is_abstract(oc)) {
@@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
     DEFINE_PROP_END_OF_LIST()
 };
 
+static void arm_cpu_parse_features(CPUState *cs, char *features,
+                                   Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    char *featurestr;
+
+    featurestr = features ? strtok(features, ",") : NULL;
+    while (featurestr) {
+        if (featurestr[0] == '-') {
+            if (!strcmp(featurestr+1, "aarch64")) {
+                /* If AArch64 is disabled then we need to unset the feature */
+                unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
+            } else {
+                /* Everyting else is unsupported */
+                error_setg(errp, "unsupported CPU property '%s'",
+                           &featurestr[1]);
+                return;
+            }
+        } else if (featurestr[0] == '+') {
+            /* No '+' properties supported yet */
+            error_setg(errp, "unsupported CPU property '%s'",
+                       &featurestr[1]);
+            return;
+        } else if (g_strstr_len(featurestr, -1, "=")) {
+            /* No '=' properties supported yet */
+            char *prop = strtok(featurestr, "=");
+            error_setg(errp, "unsupported CPU property '%s'", prop);
+            return;
+        } else {
+            /* Everything else is a bad format */
+            error_setg(errp, "CPU property string '%s' not in format "
+                             "(+feature|-feature|feature=xyz)", featurestr);
+            return;
+        }
+        featurestr = strtok(NULL, ",");
+    }
+}
+
 static void arm_cpu_class_init(ObjectClass *oc, void *data)
 {
     ARMCPUClass *acc = ARM_CPU_CLASS(oc);
@@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->set_pc = arm_cpu_set_pc;
     cc->gdb_read_register = arm_cpu_gdb_read_register;
     cc->gdb_write_register = arm_cpu_gdb_write_register;
+    cc->parse_features = arm_cpu_parse_features;
 #ifdef CONFIG_USER_ONLY
     cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
 #else
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/5] target-arm: Add feature parsing to virt

[Greg Bellows]

Added machvirt parsing of feature keywords added to the -cpu command line
option.  Parsing occurs during machine initialization.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 hw/arm/virt.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2353440..cd192ae 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -573,12 +573,19 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     const char *cpu_model = machine->cpu_model;
     VirtBoardInfo *vbi;
+    char *cpuname, *features;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
     }
 
-    vbi = find_machine_info(cpu_model);
+    /* Separate the actual CPU model name from any appended features */
+    cpuname = g_strdup(cpu_model);
+    cpuname = strtok(cpuname, ",");
+    /* Keep track of the features for later parsing */
+    features = strtok(NULL, ",");
+
+    vbi = find_machine_info(cpuname);
 
     if (!vbi) {
         error_report("mach-virt: CPU %s not supported", cpu_model);
@@ -595,8 +602,10 @@ static void machvirt_init(MachineState *machine)
     create_fdt(vbi);
 
     for (n = 0; n < smp_cpus; n++) {
-        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
+        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpuname);
+        CPUClass *cc = CPU_CLASS(oc);
         Object *cpuobj;
+        Error *err = NULL;
 
         if (!oc) {
             fprintf(stderr, "Unable to find CPU definition\n");
@@ -604,6 +613,13 @@ static void machvirt_init(MachineState *machine)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        /* Handle any CPU options specfied by the user */
+        cc->parse_features(CPU(cpuobj), features, &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
+
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
         }
@@ -623,6 +639,7 @@ static void machvirt_init(MachineState *machine)
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
+    g_free(cpuname);
     fdt_add_timer_nodes(vbi);
     fdt_add_cpu_nodes(vbi);
     fdt_add_psci_node(vbi);
-- 
1.8.3.2

[Qemu-devel] [PATCH 3/5] target-arm: Add 32/64-bit register sync

[Greg Bellows]

Add AArch32 to AArch64 register sychronization functions.
Replace manual register synchronization with new functions in
aarch64_cpu_do_interrupt() and HELPER(exception_return)().

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/helper-a64.c |  5 +--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c  |  6 ++--
 3 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 81066ca..2b5a668 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
     target_ulong addr = env->cp15.vbar_el[new_el];
     unsigned int new_mode = aarch64_pstate_mode(new_el, true);
-    int i;
 
     if (arm_current_el(env) < new_el) {
         if (env->aarch64) {
@@ -512,9 +511,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         }
         env->elr_el[new_el] = env->regs[15];
 
-        for (i = 0; i < 15; i++) {
-            env->xregs[i] = env->regs[i];
-        }
+        aarch64_sync_32_to_64(env);
 
         env->condexec_bits = 0;
     }
diff --git a/target-arm/internals.h b/target-arm/internals.h
index bb171a7..626ea7d 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState *env, int el)
     }
 }
 
+static inline void aarch64_sync_32_to_64(CPUARMState *env)
+{
+    int i;
+
+    /* We can blanket copy R[0:7] to X[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->xregs[i] = env->regs[i];
+    }
+
+    /* If we are in USR mode then we just want to complete the above blanket
+     * copy so we get the accurate register values.  If not, then we have to go
+     * to the saved and banked user regs.
+     */
+    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
+        for (i = 8; i < 15; i++) {
+            env->xregs[i] = env->regs[i];
+        }
+    } else {
+        for (i = 8; i < 13; i++) {
+            env->xregs[i] = env->usr_regs[i-8];
+        }
+        env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)];
+        env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
+    }
+    env->pc = env->regs[15];
+
+    env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
+    env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
+    env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
+    env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
+    env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
+    env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
+    env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
+    env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
+    env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
+
+    for (i = 0; i < 5; i++) {
+        env->xregs[24+i] = env->fiq_regs[i];
+    }
+    env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
+    env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
+}
+
+static inline void aarch64_sync_64_to_32(CPUARMState *env)
+{
+    int i;
+
+    /* We can blanket copy R[0:7] to X[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->regs[i] = env->xregs[i];
+    }
+
+    /* If we are in USR mode then we want to complete the above blanket
+     * copy as the XREGs will contain the most recent value.
+     */
+    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
+        for (i = 8; i < 15; i++) {
+            env->regs[i] = env->xregs[i];
+        }
+    }
+
+    /* Update the user copies and banked registers so they are also up to
+     * date.
+     */
+    for (i = 8; i < 13; i++) {
+        env->usr_regs[i-8] = env->xregs[i];
+    }
+    env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13];
+    env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
+
+    env->regs[15] = env->pc;
+
+    env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
+    env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
+    env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
+    env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
+    env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
+    env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
+    env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
+    env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
+    env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
+
+    for (i = 0; i < 5; i++) {
+        env->fiq_regs[i] = env->xregs[24+i];
+    }
+    env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
+    env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
+}
+
 static inline void update_spsel(CPUARMState *env, uint32_t imm)
 {
     unsigned int cur_el = arm_current_el(env);
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 2bed914..7713022 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env)
     int cur_el = arm_current_el(env);
     unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
     uint32_t spsr = env->banked_spsr[spsr_idx];
-    int new_el, i;
+    int new_el;
 
     aarch64_save_sp(env, cur_el);
 
@@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env)
         if (!arm_singlestep_active(env)) {
             env->uncached_cpsr &= ~PSTATE_SS;
         }
-        for (i = 0; i < 15; i++) {
-            env->regs[i] = env->xregs[i];
-        }
+        aarch64_sync_64_to_32(env);
 
         env->regs[15] = env->elr_el[1] & ~0x1;
     } else {
-- 
1.8.3.2

[Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Greg Bellows]

Add 32-bit to/from 64-bit register synchronization on register gets and puts.
Set EL1_32BIT feature flag passed to KVM

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/kvm64.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index ba16821..0061099 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -81,8 +81,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     int ret;
     ARMCPU *cpu = ARM_CPU(cs);
 
-    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
-        !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
         fprintf(stderr, "KVM is not supported for this guest CPU type\n");
         return -EINVAL;
     }
@@ -96,6 +95,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
         cpu->psci_version = 2;
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
     }
+    if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
@@ -133,6 +135,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
+    aarch64_sync_32_to_64(env);
     for (i = 0; i < 31; i++) {
         reg.id = AARCH64_CORE_REG(regs.regs[i]);
         reg.addr = (uintptr_t) &env->xregs[i];
@@ -162,7 +165,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     }
 
     /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
-    val = pstate_read(env);
+    if (is_a64(env)) {
+        val = pstate_read(env);
+    } else {
+        val = cpsr_read(env);
+    }
     reg.id = AARCH64_CORE_REG(regs.pstate);
     reg.addr = (uintptr_t) &val;
     ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
@@ -218,6 +225,7 @@ int kvm_arch_get_registers(CPUState *cs)
             return ret;
         }
     }
+    aarch64_sync_64_to_32(env);
 
     reg.id = AARCH64_CORE_REG(regs.sp);
     reg.addr = (uintptr_t) &env->sp_el[0];
@@ -239,7 +247,12 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    pstate_write(env, val);
+    if (is_a64(env)) {
+        pstate_write(env, val);
+    } else {
+        env->uncached_cpsr = val & CPSR_M;
+        cpsr_write(env, val, 0xffffffff);
+    }
 
     /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
      * QEMU side we keep the current SP in xregs[31] as well.
-- 
1.8.3.2

[Qemu-devel] [PATCH 5/5] target-arm: Adjust kernel load address for Image

[Greg Bellows]

Adjust the kernel load offset by 0x8000 if not a zImage.

Signed-off-by: Pranavkumar Sawargaonkar <address@hidden>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 hw/arm/boot.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 52ebd8b..b581cea 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -537,6 +537,32 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
     fw_cfg_add_bytes(fw_cfg, data_key, data, size);
 }
 
+static int check_load_zimage(const char *filename)
+{
+    int fd;
+    uint8_t buf[40];
+    uint32_t *p = (uint32_t *) &buf[36];
+
+    fd = open(filename, O_RDONLY | O_BINARY);
+    if (fd < 0) {
+        perror(filename);
+        return -1;
+    }
+
+    memset(buf, 0, sizeof(buf));
+    if (read(fd, buf, sizeof(buf)) != sizeof(buf)) {
+        close(fd);
+        return -1;
+    }
+
+    /* Check for zImage magic number */
+    if (*p == 0x016F2818) {
+        return 1;
+    }
+
+   return 0;
+}
+
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
     CPUState *cs;
@@ -613,7 +639,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         primary_loader = bootloader;
         kernel_load_offset = KERNEL_LOAD_ADDR;
         elf_machine = EM_ARM;
-    }
+
+        if (!check_load_zimage(info->kernel_filename)) {
+            /* Assuming we are loading Image hence aligning it to 0x8000 */
+            kernel_load_offset -= 0x8000;
+        }
+   }
 
     info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 0/5] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Sergey Fedorov]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 20.01.2015 01:30, Greg Bellows wrote:
> Added support for running an AArch32 guest on a AArch64 KVM host.  Support has
> only been added to the QEMU machvirt machine.  The addition of CPU properties
> specifiable from the command line were added to allow disablement of AArch64
> execution state hereby forcing EL1 to be AArch32.  The new CPU command line
> propoerty is "-aarch64" that is specified as follows:
>
>     aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57,-aarch64 ...

Hi!

It seems a little confusing for me to specify '-aarch64' when forcing
AArch32 execution state. Why don't just specify 'aarch32' in command
line instead of '-aarch64' construction?

BTW, /propoerty/property/

Best regards,
Serge

>
> Support also added to support uncompressed images (Image) for aarch32.
>
> Greg Bellows (5):
>   target-arm: Add ARM CPU feature parsing
>   target-arm: Add feature parsing to virt
>   target-arm: Add 32/64-bit register sync
>   target-arm: Add AArch32 guest support to KVM64
>   target-arm: Adjust kernel load address for Image
>
>  hw/arm/boot.c           | 33 +++++++++++++++++-
>  hw/arm/virt.c           | 21 ++++++++++--
>  target-arm/cpu.c        | 45 ++++++++++++++++++++++++-
>  target-arm/helper-a64.c |  5 +--
>  target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/kvm64.c      | 21 +++++++++---
>  target-arm/op_helper.c  |  6 ++--
>  7 files changed, 204 insertions(+), 16 deletions(-)
>


[-- Attachment #2: Type: text/html, Size: 2025 bytes --]

Re: [Qemu-devel] [PATCH 0/5] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Peter Maydell]

On 20 January 2015 at 10:21, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>     aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57,-aarch64 ...

> It seems a little confusing for me to specify '-aarch64' when forcing
> AArch32 execution state. Why don't just specify 'aarch32' in command line
> instead of '-aarch64' construction?

This is one of the things we discussed during design of this patchset,
and I agree the command line semantics are a bit odd. Essentially, they
make sense from the PoV of the CPU object, because they're saying
"I want a 32-bit only CPU", ie "I do not want the 64 bit feature this
CPU defaults to". Specifying '+aarch32' wouldn't do anything, because
the default Cortex-A57 already has 32-bit support. But from the PoV of
the user, it's a bit odd. The other possible approach to this would be
to have a machine model parameter for "force 32 bit boot", and have this
turn off the CPU 64 bit feature.

Other proposals welcome; I don't like the UI we end up exposing to
the user here, it's just what falls out of the natural way to model
things at the CPU QOM property level.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Alex Bennée]

Greg Bellows <greg.bellows@linaro.org> writes:

> Adds a CPU feature parsing function and assigns to the CPU class.  The only
> feature added was "-aarch64" which disabled the AArch64 execution state on a
> 64-bit ARM CPU.
>
> Also adds stripping of features from CPU model string in acquiring the ARM CPU
> by name.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 285947f..f327dd7 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
>  {
>      ObjectClass *oc;
>      char *typename;
> +    char *cpuname;
>  
>      if (!cpu_model) {
>          return NULL;
>      }
>  
> -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> +    cpuname = g_strdup(cpu_model);
> +    cpuname = strtok(cpuname, ",");
> +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
>      oc = object_class_by_name(typename);
> +    g_free(cpuname);
>      g_free(typename);

Aren't we leaking here? strtok returns the next token (or NULL) so don't
we loose the original ptr?

Also while using glib you might want to consider using glib's own
tokenising functions (e.g. g_strsplit). This has the advantage of having
helper functions like g_strfreev() which can clean-up everything in one go.

>      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
>          object_class_is_abstract(oc)) {
> @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> +static void arm_cpu_parse_features(CPUState *cs, char *features,
> +                                   Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    char *featurestr;
> +
> +    featurestr = features ? strtok(features, ",") : NULL;
> +    while (featurestr) {
> +        if (featurestr[0] == '-') {
> +            if (!strcmp(featurestr+1, "aarch64")) {
> +                /* If AArch64 is disabled then we need to unset the feature */
> +                unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +            } else {
> +                /* Everyting else is unsupported */
> +                error_setg(errp, "unsupported CPU property '%s'",
> +                           &featurestr[1]);
> +                return;
> +            }
> +        } else if (featurestr[0] == '+') {
> +            /* No '+' properties supported yet */
> +            error_setg(errp, "unsupported CPU property '%s'",
> +                       &featurestr[1]);
> +            return;
> +        } else if (g_strstr_len(featurestr, -1, "=")) {
> +            /* No '=' properties supported yet */
> +            char *prop = strtok(featurestr, "=");
> +            error_setg(errp, "unsupported CPU property '%s'", prop);
> +            return;
> +        } else {
> +            /* Everything else is a bad format */
> +            error_setg(errp, "CPU property string '%s' not in format "
> +                             "(+feature|-feature|feature=xyz)", featurestr);
> +            return;
> +        }
> +        featurestr = strtok(NULL, ",");
> +    }
> +}

I only point to this for reference to a "gliby" approach to the parsing,
relative beauty being in the eye of the beholder ;-)

https://github.com/stsquad/qemu/commit/86bc88f661141b93cbe5b107c4d5b4322b563241#diff-286aa0f2c1f0d862c4197781280a92efR116

It does make me think boilerplate but I wonder if this is a generic
enough problem to be solved more generally in QEMU?

> +
>  static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = arm_cpu_set_pc;
>      cc->gdb_read_register = arm_cpu_gdb_read_register;
>      cc->gdb_write_register = arm_cpu_gdb_write_register;
> +    cc->parse_features = arm_cpu_parse_features;
>  #ifdef CONFIG_USER_ONLY
>      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
>  #else

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

Thanks Alex comments inline....

On Tue, Jan 20, 2015 at 8:19 AM, Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Greg Bellows <greg.bellows@linaro.org> writes:
>
> > Adds a CPU feature parsing function and assigns to the CPU class.  The
> only
> > feature added was "-aarch64" which disabled the AArch64 execution state
> on a
> > 64-bit ARM CPU.
> >
> > Also adds stripping of features from CPU model string in acquiring the
> ARM CPU
> > by name.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 285947f..f327dd7 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const
> char *cpu_model)
> >  {
> >      ObjectClass *oc;
> >      char *typename;
> > +    char *cpuname;
> >
> >      if (!cpu_model) {
> >          return NULL;
> >      }
> >
> > -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> > +    cpuname = g_strdup(cpu_model);
> > +    cpuname = strtok(cpuname, ",");
> > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
> >      oc = object_class_by_name(typename);
> > +    g_free(cpuname);
> >      g_free(typename);
>
> Aren't we leaking here? strtok returns the next token (or NULL) so don't
> we loose the original ptr?
>
>
​As I understand it, strtok uses static pointers to track the location
within an existing string rather than allocating storage that the user must
free.  This is apparently what makes the version I used non-reentrant.​  In
which case, there should not be an leak due to its use.



> Also while using glib you might want to consider using glib's own
> tokenising functions (e.g. g_strsplit). This has the advantage of having
> helper functions like g_strfreev() which can clean-up everything in one go.
>

​I certainly can use the glib version, but in this case I did not see the
advantage. In fact, it actually may be less performant​ to use the glib
version given it needs to allocate/free memory.  I am fine either way if
anyone feels strongly.


>
> >      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> >          object_class_is_abstract(oc)) {
> > @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > +static void arm_cpu_parse_features(CPUState *cs, char *features,
> > +                                   Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    char *featurestr;
> > +
> > +    featurestr = features ? strtok(features, ",") : NULL;
> > +    while (featurestr) {
> > +        if (featurestr[0] == '-') {
> > +            if (!strcmp(featurestr+1, "aarch64")) {
> > +                /* If AArch64 is disabled then we need to unset the
> feature */
> > +                unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +            } else {
> > +                /* Everyting else is unsupported */
> > +                error_setg(errp, "unsupported CPU property '%s'",
> > +                           &featurestr[1]);
> > +                return;
> > +            }
> > +        } else if (featurestr[0] == '+') {
> > +            /* No '+' properties supported yet */
> > +            error_setg(errp, "unsupported CPU property '%s'",
> > +                       &featurestr[1]);
> > +            return;
> > +        } else if (g_strstr_len(featurestr, -1, "=")) {
> > +            /* No '=' properties supported yet */
> > +            char *prop = strtok(featurestr, "=");
> > +            error_setg(errp, "unsupported CPU property '%s'", prop);
> > +            return;
> > +        } else {
> > +            /* Everything else is a bad format */
> > +            error_setg(errp, "CPU property string '%s' not in format "
> > +                             "(+feature|-feature|feature=xyz)",
> featurestr);
> > +            return;
> > +        }
> > +        featurestr = strtok(NULL, ",");
> > +    }
> > +}
>
> I only point to this for reference to a "gliby" approach to the parsing,
> relative beauty being in the eye of the beholder ;-)
>
>
> https://github.com/stsquad/qemu/commit/86bc88f661141b93cbe5b107c4d5b4322b563241#diff-286aa0f2c1f0d862c4197781280a92efR116
>
> It does make me think boilerplate but I wonder if this is a generic
> enough problem to be solved more generally in QEMU?
>

If we were to go with a boilerplate approach then it would make sense to
follow the mechanism used for machine properties.  However, this was how
other arch were doing the CPU props, so we stuck to this approach.  It does
look very similar to the code you pointed at, but I think that the
conditional piece looking for {+-=} would bt the only common piece once you
add unique options and handling.


>
> > +
> >  static void arm_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> >      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> > @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc,
> void *data)
> >      cc->set_pc = arm_cpu_set_pc;
> >      cc->gdb_read_register = arm_cpu_gdb_read_register;
> >      cc->gdb_write_register = arm_cpu_gdb_write_register;
> > +    cc->parse_features = arm_cpu_parse_features;
> >  #ifdef CONFIG_USER_ONLY
> >      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
> >  #else
>
> --
> Alex Bennée
>

[-- Attachment #2: Type: text/html, Size: 8079 bytes --]

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Mon, 19 Jan 2015 16:30:17 -0600
Greg Bellows <greg.bellows@linaro.org> wrote:

> Adds a CPU feature parsing function and assigns to the CPU class.  The only
> feature added was "-aarch64" which disabled the AArch64 execution state on a
> 64-bit ARM CPU.
> 
> Also adds stripping of features from CPU model string in acquiring the ARM CPU
> by name.
> 
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 285947f..f327dd7 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
>  {
>      ObjectClass *oc;
>      char *typename;
> +    char *cpuname;
>  
>      if (!cpu_model) {
>          return NULL;
>      }
>  
> -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> +    cpuname = g_strdup(cpu_model);
> +    cpuname = strtok(cpuname, ",");
> +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
>      oc = object_class_by_name(typename);
> +    g_free(cpuname);
>      g_free(typename);
>      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
>          object_class_is_abstract(oc)) {
> @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> +static void arm_cpu_parse_features(CPUState *cs, char *features,
> +                                   Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    char *featurestr;
> +
> +    featurestr = features ? strtok(features, ",") : NULL;
> +    while (featurestr) {
> +        if (featurestr[0] == '-') {
...
> +        } else if (featurestr[0] == '+') {
Please do not use legacy +-feature format and support only foo=val format.
Other targets have it only for to being able support legacy setups
which use +- format.

> +            /* Everything else is a bad format */
> +            error_setg(errp, "CPU property string '%s' not in format "
> +                             "(+feature|-feature|feature=xyz)", featurestr);


> +            return;
> +        }
> +        featurestr = strtok(NULL, ",");
> +    }
> +}
> +
>  static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = arm_cpu_set_pc;
>      cc->gdb_read_register = arm_cpu_gdb_read_register;
>      cc->gdb_write_register = arm_cpu_gdb_write_register;
> +    cc->parse_features = arm_cpu_parse_features;
>  #ifdef CONFIG_USER_ONLY
>      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
>  #else

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Peter Maydell]

On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
> Please do not use legacy +-feature format and support only foo=val format.
> Other targets have it only for to being able support legacy setups
> which use +- format.

I thought this was the standard format for CPU features. Do you
have an example of a CPU feature being set using foo=val format?

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

On Tue, Jan 20, 2015 at 9:22 AM, Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 19 Jan 2015 16:30:17 -0600
> Greg Bellows <greg.bellows@linaro.org> wrote:
>
> > Adds a CPU feature parsing function and assigns to the CPU class.  The
> only
> > feature added was "-aarch64" which disabled the AArch64 execution state
> on a
> > 64-bit ARM CPU.
> >
> > Also adds stripping of features from CPU model string in acquiring the
> ARM CPU
> > by name.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 285947f..f327dd7 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const
> char *cpu_model)
> >  {
> >      ObjectClass *oc;
> >      char *typename;
> > +    char *cpuname;
> >
> >      if (!cpu_model) {
> >          return NULL;
> >      }
> >
> > -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> > +    cpuname = g_strdup(cpu_model);
> > +    cpuname = strtok(cpuname, ",");
> > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
> >      oc = object_class_by_name(typename);
> > +    g_free(cpuname);
> >      g_free(typename);
> >      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> >          object_class_is_abstract(oc)) {
> > @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > +static void arm_cpu_parse_features(CPUState *cs, char *features,
> > +                                   Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    char *featurestr;
> > +
> > +    featurestr = features ? strtok(features, ",") : NULL;
> > +    while (featurestr) {
> > +        if (featurestr[0] == '-') {
> ...
> > +        } else if (featurestr[0] == '+') {
> Please do not use legacy +-feature format and support only foo=val format.
> Other targets have it only for to being able support legacy setups
> which use +- format.
>
>
​Thanks Igor. I was under the impression that the +/- notation was still
relevant. Perhaps it makes the most sense to convert to using object
properties similar to how machine options are specified? ​What do you think
Peter?


> > +            /* Everything else is a bad format */
> > +            error_setg(errp, "CPU property string '%s' not in format "
> > +                             "(+feature|-feature|feature=xyz)",
> featurestr);
>
>
> > +            return;
> > +        }
> > +        featurestr = strtok(NULL, ",");
> > +    }
> > +}
> > +
> >  static void arm_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> >      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> > @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc,
> void *data)
> >      cc->set_pc = arm_cpu_set_pc;
> >      cc->gdb_read_register = arm_cpu_gdb_read_register;
> >      cc->gdb_write_register = arm_cpu_gdb_write_register;
> > +    cc->parse_features = arm_cpu_parse_features;
> >  #ifdef CONFIG_USER_ONLY
> >      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
> >  #else
>
>

[-- Attachment #2: Type: text/html, Size: 4723 bytes --]

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Tue, 20 Jan 2015 15:34:23 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
> > Please do not use legacy +-feature format and support only foo=val format.
> > Other targets have it only for to being able support legacy setups
> > which use +- format.
> 
> I thought this was the standard format for CPU features. Do you
> have an example of a CPU feature being set using foo=val format?
Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
in addition to it we ca use canonized format for generic properties
like, foo1=on,foo2=off,foo3=on

We try to move out of legacy format, so that it would be possible
to reuse generic property parsing infrastructure like with any
device object. That would allow to use -device/device_add for CPUs.

> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Eduardo Habkost]

On Tue, Jan 20, 2015 at 09:34:39AM -0600, Greg Bellows wrote:
> On Tue, Jan 20, 2015 at 9:22 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Mon, 19 Jan 2015 16:30:17 -0600
> > Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> > > Adds a CPU feature parsing function and assigns to the CPU class.  The
> > only
> > > feature added was "-aarch64" which disabled the AArch64 execution state
> > on a
> > > 64-bit ARM CPU.
> > >
> > > Also adds stripping of features from CPU model string in acquiring the
> > ARM CPU
> > > by name.
> > >
> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > > ---
> > >  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > index 285947f..f327dd7 100644
> > > --- a/target-arm/cpu.c
> > > +++ b/target-arm/cpu.c
> > > @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const
> > char *cpu_model)
> > >  {
> > >      ObjectClass *oc;
> > >      char *typename;
> > > +    char *cpuname;
> > >
> > >      if (!cpu_model) {
> > >          return NULL;
> > >      }
> > >
> > > -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> > > +    cpuname = g_strdup(cpu_model);
> > > +    cpuname = strtok(cpuname, ",");
> > > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
> > >      oc = object_class_by_name(typename);
> > > +    g_free(cpuname);
> > >      g_free(typename);
> > >      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> > >          object_class_is_abstract(oc)) {
> > > @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
> > >      DEFINE_PROP_END_OF_LIST()
> > >  };
> > >
> > > +static void arm_cpu_parse_features(CPUState *cs, char *features,
> > > +                                   Error **errp)
> > > +{
> > > +    ARMCPU *cpu = ARM_CPU(cs);
> > > +    char *featurestr;
> > > +
> > > +    featurestr = features ? strtok(features, ",") : NULL;
> > > +    while (featurestr) {
> > > +        if (featurestr[0] == '-') {
> > ...
> > > +        } else if (featurestr[0] == '+') {
> > Please do not use legacy +-feature format and support only foo=val format.
> > Other targets have it only for to being able support legacy setups
> > which use +- format.
> >
> >
> ​Thanks Igor. I was under the impression that the +/- notation was still
> relevant. Perhaps it makes the most sense to convert to using object
> properties similar to how machine options are specified? ​What do you think
> Peter?

This is what we have been working on, on x86. "+FOO" should be
translated to setting QOM property "feat-FOO=on" (note that you
shouldn't need to use "feat-FOO" for the proeprty names, as you don't
need to support the legacy +-feature format).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Tue, 20 Jan 2015 09:34:39 -0600
Greg Bellows <greg.bellows@linaro.org> wrote:

> On Tue, Jan 20, 2015 at 9:22 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Mon, 19 Jan 2015 16:30:17 -0600
> > Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> > > Adds a CPU feature parsing function and assigns to the CPU class.  The
> > only
> > > feature added was "-aarch64" which disabled the AArch64 execution state
> > on a
> > > 64-bit ARM CPU.
> > >
> > > Also adds stripping of features from CPU model string in acquiring the
> > ARM CPU
> > > by name.
> > >
> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > > ---
> > >  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > index 285947f..f327dd7 100644
> > > --- a/target-arm/cpu.c
> > > +++ b/target-arm/cpu.c
> > > @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const
> > char *cpu_model)
> > >  {
> > >      ObjectClass *oc;
> > >      char *typename;
> > > +    char *cpuname;
> > >
> > >      if (!cpu_model) {
> > >          return NULL;
> > >      }
> > >
> > > -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> > > +    cpuname = g_strdup(cpu_model);
> > > +    cpuname = strtok(cpuname, ",");
> > > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
> > >      oc = object_class_by_name(typename);
> > > +    g_free(cpuname);
> > >      g_free(typename);
> > >      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> > >          object_class_is_abstract(oc)) {
> > > @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
> > >      DEFINE_PROP_END_OF_LIST()
> > >  };
> > >
> > > +static void arm_cpu_parse_features(CPUState *cs, char *features,
> > > +                                   Error **errp)
> > > +{
> > > +    ARMCPU *cpu = ARM_CPU(cs);
> > > +    char *featurestr;
> > > +
> > > +    featurestr = features ? strtok(features, ",") : NULL;
> > > +    while (featurestr) {
> > > +        if (featurestr[0] == '-') {
> > ...
> > > +        } else if (featurestr[0] == '+') {
> > Please do not use legacy +-feature format and support only foo=val format.
> > Other targets have it only for to being able support legacy setups
> > which use +- format.
> >
> >
> ​Thanks Igor. I was under the impression that the +/- notation was still
> relevant. Perhaps it makes the most sense to convert to using object
> properties similar to how machine options are specified? ​What do you think
> Peter?
Yep make features as object properties and reuse generic property parsing.
Since you do not have to support legacy format, you actually do not need
to define arm_cpu_parse_features() callback because foo=val format
can be parsed by generic property parsing infrastructure.

> 
> 
> > > +            /* Everything else is a bad format */
> > > +            error_setg(errp, "CPU property string '%s' not in format "
> > > +                             "(+feature|-feature|feature=xyz)",
> > featurestr);
> >
> >
> > > +            return;
> > > +        }
> > > +        featurestr = strtok(NULL, ",");
> > > +    }
> > > +}
> > > +
> > >  static void arm_cpu_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> > > @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc,
> > void *data)
> > >      cc->set_pc = arm_cpu_set_pc;
> > >      cc->gdb_read_register = arm_cpu_gdb_read_register;
> > >      cc->gdb_write_register = arm_cpu_gdb_write_register;
> > > +    cc->parse_features = arm_cpu_parse_features;
> > >  #ifdef CONFIG_USER_ONLY
> > >      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
> > >  #else
> >
> >

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Peter Maydell]

On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 20 Jan 2015 15:34:23 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
>> > Please do not use legacy +-feature format and support only foo=val format.
>> > Other targets have it only for to being able support legacy setups
>> > which use +- format.
>>
>> I thought this was the standard format for CPU features. Do you
>> have an example of a CPU feature being set using foo=val format?
> Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
> in addition to it we ca use canonized format for generic properties
> like, foo1=on,foo2=off,foo3=on
>
> We try to move out of legacy format, so that it would be possible
> to reuse generic property parsing infrastructure like with any
> device object. That would allow to use -device/device_add for CPUs.

-device/-device_add for CPUs is pretty fraught in the general
case because there's no obvious place to plug them and have
them be wired up properly. You'd need to use -global for CPU
properties, which is a nightmare...

Anyway, I'm not particularly attached to the exact command
line syntax we've used here -- I was just looking for "we have
a CPU property, and use the same syntax for specifying CPU
feature enable/disable that other CPUs do"...

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Tue, 20 Jan 2015 16:08:09 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 20 Jan 2015 15:34:23 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
> >> > Please do not use legacy +-feature format and support only foo=val format.
> >> > Other targets have it only for to being able support legacy setups
> >> > which use +- format.
> >>
> >> I thought this was the standard format for CPU features. Do you
> >> have an example of a CPU feature being set using foo=val format?
> > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
> > in addition to it we ca use canonized format for generic properties
> > like, foo1=on,foo2=off,foo3=on
> >
> > We try to move out of legacy format, so that it would be possible
> > to reuse generic property parsing infrastructure like with any
> > device object. That would allow to use -device/device_add for CPUs.
> 
> -device/-device_add for CPUs is pretty fraught in the general
> case because there's no obvious place to plug them and have
> them be wired up properly.
That depends on CPU of-cause, but we are close to having device_add
working with x86 CPUs.

> You'd need to use -global for CPU
> properties, which is a nightmare...
mine thoughts on it were that '-cpu type,feat...' would  eventually
do conversion of features to global properties transparently for
user using target specific cc->parse_features() callback. Which
Greg could actually do here. We would happy to reuse it with x86 CPUs.

> 
> Anyway, I'm not particularly attached to the exact command
> line syntax we've used here -- I was just looking for "we have
> a CPU property, and use the same syntax for specifying CPU
> feature enable/disable that other CPUs do"...
Then '-cpu arm_foo,featX=on/off' should do the job.


> 
> -- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Alex Bennée]

Greg Bellows <greg.bellows@linaro.org> writes:

> Add 32-bit to/from 64-bit register synchronization on register gets and puts.
> Set EL1_32BIT feature flag passed to KVM
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/kvm64.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index ba16821..0061099 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -81,8 +81,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      int ret;
>      ARMCPU *cpu = ARM_CPU(cs);
>  
> -    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
> -        !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
>          fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>          return -EINVAL;
>      }
> @@ -96,6 +95,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          cpu->psci_version = 2;
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>      }
> +    if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
> +    }
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> @@ -133,6 +135,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>  
> +    aarch64_sync_32_to_64(env);
>      for (i = 0; i < 31; i++) {
>          reg.id = AARCH64_CORE_REG(regs.regs[i]);
>          reg.addr = (uintptr_t) &env->xregs[i];
> @@ -162,7 +165,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      }
>  
>      /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
> -    val = pstate_read(env);
> +    if (is_a64(env)) {
> +        val = pstate_read(env);
> +    } else {
> +        val = cpsr_read(env);
> +    }

I know why we do this (especially given where my attempt ended up) but
perhaps we could at list have a single state aware accessor so we don't
end up duplicating this test all over the place?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 2/5] target-arm: Add feature parsing to virt

[Alex Bennée]

Greg Bellows <greg.bellows@linaro.org> writes:

> Added machvirt parsing of feature keywords added to the -cpu command line
> option.  Parsing occurs during machine initialization.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  hw/arm/virt.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 2353440..cd192ae 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -573,12 +573,19 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      const char *cpu_model = machine->cpu_model;
>      VirtBoardInfo *vbi;
> +    char *cpuname, *features;
>  
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
>      }
>  
> -    vbi = find_machine_info(cpu_model);
> +    /* Separate the actual CPU model name from any appended features */
> +    cpuname = g_strdup(cpu_model);
> +    cpuname = strtok(cpuname, ",");
> +    /* Keep track of the features for later parsing */
> +    features = strtok(NULL, ",");
<snip>

OK having read the strtok man page a bit more closely the second time
I'm happy this doesn't leak. My more-gliby request is a matter of
personal taste.

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Greg Bellows]

On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Greg Bellows <greg.bellows@linaro.org> writes:
>
>> Add 32-bit to/from 64-bit register synchronization on register gets and puts.
>> Set EL1_32BIT feature flag passed to KVM
>>
>> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>> ---
>>  target-arm/kvm64.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index ba16821..0061099 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -81,8 +81,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      int ret;
>>      ARMCPU *cpu = ARM_CPU(cs);
>>
>> -    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
>> -        !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
>>          fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>>          return -EINVAL;
>>      }
>> @@ -96,6 +95,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>          cpu->psci_version = 2;
>>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>>      }
>> +    if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>> +    }
>>
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>> @@ -133,6 +135,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>      ARMCPU *cpu = ARM_CPU(cs);
>>      CPUARMState *env = &cpu->env;
>>
>> +    aarch64_sync_32_to_64(env);
>>      for (i = 0; i < 31; i++) {
>>          reg.id = AARCH64_CORE_REG(regs.regs[i]);
>>          reg.addr = (uintptr_t) &env->xregs[i];
>> @@ -162,7 +165,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>      }
>>
>>      /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
>> -    val = pstate_read(env);
>> +    if (is_a64(env)) {
>> +        val = pstate_read(env);
>> +    } else {
>> +        val = cpsr_read(env);
>> +    }
>
> I know why we do this (especially given where my attempt ended up) but
> perhaps we could at list have a single state aware accessor so we don't
> end up duplicating this test all over the place?

I'd happily add an accessor function, but I only found 1 other
location that does this conditional so I'm not sure it is warranted.

>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Greg Bellows]

On Tue, Jan 20, 2015 at 10:25 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 20 Jan 2015 16:08:09 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Tue, 20 Jan 2015 15:34:23 +0000
>> > Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> >> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
>> >> > Please do not use legacy +-feature format and support only foo=val format.
>> >> > Other targets have it only for to being able support legacy setups
>> >> > which use +- format.
>> >>
>> >> I thought this was the standard format for CPU features. Do you
>> >> have an example of a CPU feature being set using foo=val format?
>> > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
>> > in addition to it we ca use canonized format for generic properties
>> > like, foo1=on,foo2=off,foo3=on
>> >
>> > We try to move out of legacy format, so that it would be possible
>> > to reuse generic property parsing infrastructure like with any
>> > device object. That would allow to use -device/device_add for CPUs.
>>
>> -device/-device_add for CPUs is pretty fraught in the general
>> case because there's no obvious place to plug them and have
>> them be wired up properly.
> That depends on CPU of-cause, but we are close to having device_add
> working with x86 CPUs.
>
>> You'd need to use -global for CPU
>> properties, which is a nightmare...
> mine thoughts on it were that '-cpu type,feat...' would  eventually
> do conversion of features to global properties transparently for
> user using target specific cc->parse_features() callback. Which
> Greg could actually do here. We would happy to reuse it with x86 CPUs.
>
>>
>> Anyway, I'm not particularly attached to the exact command
>> line syntax we've used here -- I was just looking for "we have
>> a CPU property, and use the same syntax for specifying CPU
>> feature enable/disable that other CPUs do"...
> Then '-cpu arm_foo,featX=on/off' should do the job.
>
>
>>
>> -- PMM
>

For now I went with the "-cpu arm_foo,aarch64=off" approach so as to
not complicate it right now with adding full-blown CPU properties.

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Alex Bennée]

Greg Bellows <greg.bellows@linaro.org> writes:

> On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Greg Bellows <greg.bellows@linaro.org> writes:
>>
>>> Add 32-bit to/from 64-bit register synchronization on register gets and puts.
>>> Set EL1_32BIT feature flag passed to KVM
>>>
>>> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
<snip>
>>>      }
>>>
>>>      /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
>>> -    val = pstate_read(env);
>>> +    if (is_a64(env)) {
>>> +        val = pstate_read(env);
>>> +    } else {
>>> +        val = cpsr_read(env);
>>> +    }
>>
>> I know why we do this (especially given where my attempt ended up) but
>> perhaps we could at list have a single state aware accessor so we don't
>> end up duplicating this test all over the place?
>
> I'd happily add an accessor function, but I only found 1 other
> location that does this conditional so I'm not sure it is warranted.

The migration/serialisation code? Today one other, tomorrow just one more?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Peter Maydell]

On 21 January 2015 at 10:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Greg Bellows <greg.bellows@linaro.org> writes:
>
>> On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> I know why we do this (especially given where my attempt ended up) but
>>> perhaps we could at list have a single state aware accessor so we don't
>>> end up duplicating this test all over the place?
>>
>> I'd happily add an accessor function, but I only found 1 other
>> location that does this conditional so I'm not sure it is warranted.
>
> The migration/serialisation code? Today one other, tomorrow just one more?

Meh. I suggested to Greg that just inlining this at point of use
was the simplest approach. Maybe one day we'll clean this stuff
up but yet another accessor function doesn't seem too great either.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Alex Bennée]

Greg Bellows <greg.bellows@linaro.org> writes:

> Thanks Alex comments inline....
>
<snip>
>>
>> Aren't we leaking here? strtok returns the next token (or NULL) so don't
>> we loose the original ptr?
>>
>>
> ​As I understand it, strtok uses static pointers to track the location
> within an existing string rather than allocating storage that the user must
> free.  This is apparently what makes the version I used non-reentrant.​  In
> which case, there should not be an leak due to its use.

Yeah - I realised this after re-reading the man page. Non-re-entrant
isn't a particular problem these days but it still feels dirty....

>> Also while using glib you might want to consider using glib's own
>> tokenising functions (e.g. g_strsplit). This has the advantage of having
>> helper functions like g_strfreev() which can clean-up everything in one go.
>>
>
> ​I certainly can use the glib version, but in this case I did not see the
> advantage. In fact, it actually may be less performant​ to use the glib
> version given it needs to allocate/free memory.  I am fine either way if
> anyone feels strongly.

I suspect this discussion is trumped by moving to the feat_foo=on/off
parsing style referenced elsewhere so we can use common code.

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Tue, 20 Jan 2015 16:45:19 -0600
Greg Bellows <greg.bellows@linaro.org> wrote:

> On Tue, Jan 20, 2015 at 10:25 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 20 Jan 2015 16:08:09 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote:
> >> > On Tue, 20 Jan 2015 15:34:23 +0000
> >> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >> >
> >> >> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote:
> >> >> > Please do not use legacy +-feature format and support only foo=val format.
> >> >> > Other targets have it only for to being able support legacy setups
> >> >> > which use +- format.
> >> >>
> >> >> I thought this was the standard format for CPU features. Do you
> >> >> have an example of a CPU feature being set using foo=val format?
> >> > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
> >> > in addition to it we ca use canonized format for generic properties
> >> > like, foo1=on,foo2=off,foo3=on
> >> >
> >> > We try to move out of legacy format, so that it would be possible
> >> > to reuse generic property parsing infrastructure like with any
> >> > device object. That would allow to use -device/device_add for CPUs.
> >>
> >> -device/-device_add for CPUs is pretty fraught in the general
> >> case because there's no obvious place to plug them and have
> >> them be wired up properly.
> > That depends on CPU of-cause, but we are close to having device_add
> > working with x86 CPUs.
> >
> >> You'd need to use -global for CPU
> >> properties, which is a nightmare...
> > mine thoughts on it were that '-cpu type,feat...' would  eventually
> > do conversion of features to global properties transparently for
> > user using target specific cc->parse_features() callback. Which
> > Greg could actually do here. We would happy to reuse it with x86 CPUs.
> >
> >>
> >> Anyway, I'm not particularly attached to the exact command
> >> line syntax we've used here -- I was just looking for "we have
> >> a CPU property, and use the same syntax for specifying CPU
> >> feature enable/disable that other CPUs do"...
> > Then '-cpu arm_foo,featX=on/off' should do the job.
> >
> >
> >>
> >> -- PMM
> >
> 
> For now I went with the "-cpu arm_foo,aarch64=off" approach so as to
> not complicate it right now with adding full-blown CPU properties.
I see that there is quite a mess with feature bits and properties
on ARM target already, so I won't ask to rewrite all of it to.
But since you need control only one feature than make it property
and set related feature bit from property setter or like it's done
for arm_cpu_has_el3_property.
Then in arm_cpu_parse_features() you can just do generic setting:

 object_property_parse(OBJECT(cpu), val, featurename, &local_err);

for foo=on/off format and forget about touching it again,
further down the road one would just need to add a property to
mange needed feature bits.