[Qemu-devel] [PATCH v3 0/6] spapr: add "compat" machine option

[Qemu-devel] [PATCH v3 0/6] spapr: add "compat" machine option

[Alexey Kardashevskiy]

The further I go, more questions I get.

Here are 6 patches.

The first three is what I would like to have in QEMU to support "compat"
option for a CPU. The option now accepts "power6"/"power7" as after all
we will limit number of threads per core (not in this series though) and
since 2.05 does not limit number of threads at all, referring to actual
CPU models seems right.

The last three is what I would suggest doing if we needed ability to
enable/disable CPU features the way x86 does this. I used "VSX" as an example
but this is just an example so "-cpu host,-vsx,+vsx,vsx=on" works.
Using this, I suspect I could try converting x86's parser for "-cpu",
would it work?


btw I am sure there must be macro like BITNR (convert mask with 1 bit set
to a number of the bit which is set) but I failed to find it. What did I miss?


Please, comment. Thanks.


Alexey Kardashevskiy (6):
  cpu: add suboptions support
  target-ppc: make use of new -cpu suboptions handling
  target-ppc: add "compat" CPU option
  qemu-option: support +foo/-foo command line agruments
  bitops: add BITNR macro
  target-ppc: demonstrate new "vsx" property

 hw/ppc/spapr.c              | 13 +++++++-
 include/qemu/bitops.h       | 12 ++++++++
 include/qom/cpu.h           | 29 ++++++++++++++++++
 include/sysemu/sysemu.h     |  1 +
 qom/cpu.c                   | 27 +++++++++++++++++
 target-ppc/cpu-models.h     | 10 +++++++
 target-ppc/cpu.h            |  4 +++
 target-ppc/translate_init.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-option.c          |  6 ++++
 vl.c                        | 42 ++++++++++++++++++++++++++
 10 files changed, 216 insertions(+), 1 deletion(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH v3 1/6] cpu: add suboptions support

[Alexey Kardashevskiy]

This adds suboptions support for -cpu.

Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/qom/cpu.h       | 29 +++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |  1 +
 qom/cpu.c               | 27 +++++++++++++++++++++++++++
 vl.c                    | 42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7739e00..7d3b043 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -124,6 +124,7 @@ typedef struct CPUClass {
                             int cpuid, void *opaque);
     int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
                                 void *opaque);
+    void (*parse_options)(CPUState *cpu, Error **errp);
 
     const struct VMStateDescription *vmsd;
     int gdb_num_core_regs;
@@ -327,6 +328,34 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
 #endif
 
 /**
+ * cpu_parse_options:
+ * @cpu: The CPU to set options for.
+ */
+static inline int cpu_parse_options(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    Error *err = NULL;
+
+    if (cc->parse_options) {
+        cc->parse_options(cpu, &err);
+        if (err) {
+            return -1;
+        }
+    }
+
+    /* No callback, let arch do it the old way */
+    return 0;
+}
+
+/**
+ * cpu_default_parse_options_func:
+ * The default handler for CPUClass::parse_options
+ * @cpu: the CPU to set option for.
+ * @errp: the handling error descriptor.
+ */
+void cpu_default_parse_options_func(CPUState *cpu, Error **errp);
+
+/**
  * cpu_reset:
  * @cpu: The CPU whose state is to be reset.
  */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index cd5791e..c6e3ea0 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -190,6 +190,7 @@ char *get_boot_devices_list(size_t *size);
 DeviceState *get_boot_device(uint32_t position);
 
 QemuOpts *qemu_get_machine_opts(void);
+QemuOpts *qemu_get_cpu_opts(void);
 
 bool usb_enabled(bool default_usb);
 
diff --git a/qom/cpu.c b/qom/cpu.c
index 818fb26..231dec5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,8 @@
 #include "qemu/notify.h"
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/config-file.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -186,6 +188,31 @@ void cpu_reset(CPUState *cpu)
     }
 }
 
+static int cpu_set_property(const char *name, const char *value, void *opaque)
+{
+    Error *err = NULL;
+
+    if (strcmp(name, "type") == 0) {
+        return 0;
+    }
+
+    object_property_parse(opaque, value, name, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        return -1;
+    }
+
+    return 0;
+}
+
+void cpu_default_parse_options_func(CPUState *cpu, Error **errp)
+{
+    if (qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, cpu, 1)) {
+        error_setg(errp, "Bad option");
+    }
+}
+
 static void cpu_common_reset(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/vl.c b/vl.c
index 4ad15b8..09f8e8b 100644
--- a/vl.c
+++ b/vl.c
@@ -433,6 +433,16 @@ static QemuOptsList qemu_machine_opts = {
     },
 };
 
+static QemuOptsList qemu_cpu_opts = {
+    .name = "cpu",
+    .implied_opt_name = "type",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head),
+    .desc = {
+        { /* End of list */ }
+    },
+};
+
 static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .implied_opt_name = "order",
@@ -548,6 +558,25 @@ QemuOpts *qemu_get_machine_opts(void)
     return opts;
 }
 
+/**
+ * Get CPU options
+ *
+ * Returns: CPU options (never null).
+ */
+QemuOpts *qemu_get_cpu_opts(void)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("cpu");
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create_nofail(list);
+    }
+    return opts;
+}
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -2877,6 +2906,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_trace_opts);
     qemu_add_opts(&qemu_option_rom_opts);
     qemu_add_opts(&qemu_machine_opts);
+    qemu_add_opts(&qemu_cpu_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_sandbox_opts);
@@ -2972,7 +3002,19 @@ int main(int argc, char **argv, char **envp)
             }
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
+
+                /* Store cpu_model for old style parser */
                 cpu_model = optarg;
+
+                /*
+                 * Parse and save options in the cpu options list
+                 * for a new parser
+                 */
+                olist = qemu_find_opts("cpu");
+                opts = qemu_opts_parse(olist, optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_hda:
                 {
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3 1/6] cpu: add suboptions support

[Alexey Kardashevskiy]

On 11/11/2013 06:44 PM, Alexey Kardashevskiy wrote:
> This adds suboptions support for -cpu.
> 
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/qom/cpu.h       | 29 +++++++++++++++++++++++++++++
>  include/sysemu/sysemu.h |  1 +
>  qom/cpu.c               | 27 +++++++++++++++++++++++++++
>  vl.c                    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 99 insertions(+)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7739e00..7d3b043 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -124,6 +124,7 @@ typedef struct CPUClass {
>                              int cpuid, void *opaque);
>      int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
>                                  void *opaque);
> +    void (*parse_options)(CPUState *cpu, Error **errp);
>  
>      const struct VMStateDescription *vmsd;
>      int gdb_num_core_regs;
> @@ -327,6 +328,34 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>  #endif
>  
>  /**
> + * cpu_parse_options:
> + * @cpu: The CPU to set options for.
> + */
> +static inline int cpu_parse_options(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    Error *err = NULL;
> +
> +    if (cc->parse_options) {
> +        cc->parse_options(cpu, &err);
> +        if (err) {
> +            return -1;
> +        }
> +    }
> +
> +    /* No callback, let arch do it the old way */
> +    return 0;
> +}
> +
> +/**
> + * cpu_default_parse_options_func:
> + * The default handler for CPUClass::parse_options
> + * @cpu: the CPU to set option for.
> + * @errp: the handling error descriptor.
> + */
> +void cpu_default_parse_options_func(CPUState *cpu, Error **errp);
> +
> +/**
>   * cpu_reset:
>   * @cpu: The CPU whose state is to be reset.
>   */
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index cd5791e..c6e3ea0 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -190,6 +190,7 @@ char *get_boot_devices_list(size_t *size);
>  DeviceState *get_boot_device(uint32_t position);
>  
>  QemuOpts *qemu_get_machine_opts(void);
> +QemuOpts *qemu_get_cpu_opts(void);
>  
>  bool usb_enabled(bool default_usb);
>  
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 818fb26..231dec5 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -24,6 +24,8 @@
>  #include "qemu/notify.h"
>  #include "qemu/log.h"
>  #include "sysemu/sysemu.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qemu/config-file.h"
>  
>  bool cpu_exists(int64_t id)
>  {
> @@ -186,6 +188,31 @@ void cpu_reset(CPUState *cpu)
>      }
>  }
>  
> +static int cpu_set_property(const char *name, const char *value, void *opaque)
> +{
> +    Error *err = NULL;
> +
> +    if (strcmp(name, "type") == 0) {
> +        return 0;
> +    }
> +
> +    object_property_parse(opaque, value, name, &err);
> +    if (err != NULL) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +void cpu_default_parse_options_func(CPUState *cpu, Error **errp)
> +{
> +    if (qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, cpu, 1)) {
> +        error_setg(errp, "Bad option");
> +    }
> +}

btw there is a problem - all this qemu_opt stuff normally lives in vl.c so
this patch does not compile with linux-user target.

How to solve this properly before I continue spamming the list? :)



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/6] cpu: add suboptions support

[Paolo Bonzini]

Il 30/11/2013 11:10, Alexey Kardashevskiy ha scritto:
> btw there is a problem - all this qemu_opt stuff normally lives in vl.c so
> this patch does not compile with linux-user target.

Which qemu_opt exactly?

Perhaps you can just move it to util/qemu-option.c or something like that.

Paolo

> How to solve this properly before I continue spamming the list? :)
> 

Re: [Qemu-devel] [PATCH v3 1/6] cpu: add suboptions support

[Alexey Kardashevskiy]

On 11/30/2013 10:00 PM, Paolo Bonzini wrote:
> Il 30/11/2013 11:10, Alexey Kardashevskiy ha scritto:
>> btw there is a problem - all this qemu_opt stuff normally lives in vl.c so
>> this patch does not compile with linux-user target.
> 
> Which qemu_opt exactly?
> 
> Perhaps you can just move it to util/qemu-option.c or something like that.


Hm. Doublechecked. I copied qemu_get_cpu_opts() from
qemu_get_machine_opts() and put it in vl.c but it can perfectly live in
qom/cpu.c along with cpu_default_parse_options_func() and others. Thanks
for the hint.


> 
> Paolo
> 
>> How to solve this properly before I continue spamming the list? :)
>>
> 


-- 
Alexey

[Qemu-devel] [PATCH v3 2/6] target-ppc: make use of new -cpu suboptions handling

[Alexey Kardashevskiy]

This enables -cpu subobtions parser. No option is supported yet.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c              | 4 +++-
 target-ppc/translate_init.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7e53a5f..2e18bdb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1101,7 +1101,7 @@ static SaveVMHandlers savevm_htab_handlers = {
 static void ppc_spapr_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
+    const char *cpu_model;
     const char *kernel_filename = args->kernel_filename;
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
@@ -1121,6 +1121,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     msi_supported = true;
 
+    cpu_model = qemu_opt_get(qemu_get_cpu_opts(), "type");
+
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);
 
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c030a20..9e4af56 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7381,6 +7381,10 @@ static void init_ppc_proc(PowerPCCPU *cpu)
     /* PowerPC implementation specific initialisations (SPRs, timers, ...) */
     (*pcc->init_proc)(env);
 
+    if (cpu_parse_options(CPU(cpu))) {
+        exit(1);
+    }
+
     /* MSR bits & flags consistency checks */
     if (env->msr_mask & (1 << 25)) {
         switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
@@ -8674,6 +8678,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 
     dc->fw_name = "PowerPC,UNKNOWN";
+    cc->parse_options = cpu_default_parse_options_func;
 }
 
 static const TypeInfo ppc_cpu_type_info = {
-- 
1.8.4.rc4

[Qemu-devel] [PATCH v3 3/6] target-ppc: add "compat" CPU option

[Alexey Kardashevskiy]

To be able to boot a guest on the hardware which is newer than the guest
actually supports, PowerISA defines a logical PVR per every PowerISA
specification version from 2.04.

This adds the "compat" option which takes values 205 or 206 and forces
QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
CPU_POWERPC_LOGICAL_2_06) which is stored in the "cpu-version" property of
CPU device nodes.

Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* "compat" option accepts "power6" and "power7" now
---
 hw/ppc/spapr.c              |  9 +++++++
 target-ppc/cpu-models.h     | 10 ++++++++
 target-ppc/cpu.h            |  4 +++
 target-ppc/translate_init.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2e18bdb..9fcbd96 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -206,6 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
 
     CPU_FOREACH(cpu) {
         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
+        CPUPPCState *env = &POWERPC_CPU(cpu)->env;
         uint32_t associativity[] = {cpu_to_be32(0x5),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(0x0),
@@ -238,6 +239,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
         if (ret < 0) {
             return ret;
         }
+
+        if (env->compat) {
+            ret = fdt_setprop(fdt, offset, "cpu-version",
+                              &env->compat, sizeof(env->compat));
+            if (ret < 0) {
+                return ret;
+            }
+        }
     }
     return ret;
 }
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index 49ba4a4..d7c033c 100644
--- a/target-ppc/cpu-models.h
+++ b/target-ppc/cpu-models.h
@@ -583,6 +583,16 @@ enum {
     CPU_POWERPC_RS64II             = 0x00340000,
     CPU_POWERPC_RS64III            = 0x00360000,
     CPU_POWERPC_RS64IV             = 0x00370000,
+
+    /* Logical CPUs */
+    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
+    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
+    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
+    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
+    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
+    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
+    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
+
 #endif /* defined(TARGET_PPC64) */
     /* Original POWER */
     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index bb84767..8e30518 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1006,6 +1006,9 @@ struct CPUPPCState {
     /* Device control registers */
     ppc_dcr_t *dcr_env;
 
+    /* Architecture compatibility mode PVR */
+    uint32_t compat;
+
     int dcache_line_size;
     int icache_line_size;
 
@@ -1330,6 +1333,7 @@ static inline int cpu_mmu_index (CPUPPCState *env)
 #define SPR_BOOKE_DVC1        (0x13E)
 #define SPR_BOOKE_DVC2        (0x13F)
 #define SPR_BOOKE_TSR         (0x150)
+#define SPR_PCR               (0x152)
 #define SPR_BOOKE_TCR         (0x154)
 #define SPR_BOOKE_TLB0PS      (0x158)
 #define SPR_BOOKE_TLB1PS      (0x159)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 9e4af56..df0d81c 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -28,6 +28,7 @@
 #include "mmu-hash32.h"
 #include "mmu-hash64.h"
 #include "qemu/error-report.h"
+#include "qapi/visitor.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -7135,8 +7136,60 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_BUS_CLK;
 }
 
+static void powerpc_get_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    char *value = (char *)"";
+
+    switch (cpu->env.compat) {
+    case CPU_POWERPC_LOGICAL_2_05:
+        value = (char *)"power6";
+        break;
+    case CPU_POWERPC_LOGICAL_2_06:
+        value = (char *)"power7";
+        break;
+    case 0:
+        break;
+    default:
+        error_setg(errp, "Internal error: compat is set to %x",
+                   cpu->env.compat);
+        break;
+    }
+
+    visit_type_str(v, &value, name, errp);
+}
+
+static void powerpc_set_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    Error *error = NULL;
+    /* TODO: Implement check for the value length */
+    char *value = NULL;
+
+    visit_type_str(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    if (strcmp(value, "power6") == 0) {
+        cpu->env.compat = CPU_POWERPC_LOGICAL_2_05;
+    } else if (strcmp(value, "power7") == 0) {
+        cpu->env.compat = CPU_POWERPC_LOGICAL_2_06;
+    } else {
+        error_setg(errp, "Invalid compatibility mode \"%s\", only \"power6\" and \"power7\" supported",
+                   value);
+    }
+
+    g_free(value);
+}
+
 static void init_proc_POWER7 (CPUPPCState *env)
 {
+    PowerPCCPU *cpu = container_of(env, PowerPCCPU, env);
+
     gen_spr_ne_601(env);
     gen_spr_7xx(env);
     /* Time base */
@@ -7201,6 +7254,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
                  &spr_read_generic, &spr_write_generic,
                  &spr_read_generic, &spr_write_generic,
                  0x00000000);
+    spr_register(env, SPR_PCR, "PCR",
+                 &spr_read_generic, &spr_write_generic,
+                 &spr_read_generic, &spr_write_generic,
+                 0x00000000);
 #if !defined(CONFIG_USER_ONLY)
     env->slb_nr = 32;
 #endif
@@ -7213,6 +7270,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
     /* Can't find information on what this should be on reset.  This
      * value is the one used by 74xx processors. */
     vscr_init(env, 0x00010000);
+
+    object_property_add(OBJECT(cpu), "compat", "int",
+                        powerpc_get_compat, powerpc_set_compat,
+                        NULL, NULL, NULL);
 }
 
 POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
-- 
1.8.4.rc4

[Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Alexey Kardashevskiy]

This converts +foo/-foo to "foo=on"/"foo=off" respectively when
QEMU parser is used for the command line options.

"-cpu" parsers in x86 and other architectures should be unaffected
by this change.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 util/qemu-option.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index efcb5dc..6c8667c 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
                 if (strncmp(option, "no", 2) == 0) {
                     memmove(option, option+2, strlen(option+2)+1);
                     pstrcpy(value, sizeof(value), "off");
+                } else if (strncmp(option, "-", 1) == 0) {
+                    memmove(option, option+1, strlen(option+1)+1);
+                    pstrcpy(value, sizeof(value), "off");
+                } else if (strncmp(option, "+", 1) == 0) {
+                    memmove(option, option+1, strlen(option+1)+1);
+                    pstrcpy(value, sizeof(value), "on");
                 } else {
                     pstrcpy(value, sizeof(value), "on");
                 }
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Andreas Färber]

Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> QEMU parser is used for the command line options.
> 
> "-cpu" parsers in x86 and other architectures should be unaffected
> by this change.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  util/qemu-option.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index efcb5dc..6c8667c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>                  if (strncmp(option, "no", 2) == 0) {
>                      memmove(option, option+2, strlen(option+2)+1);
>                      pstrcpy(value, sizeof(value), "off");
> +                } else if (strncmp(option, "-", 1) == 0) {
> +                    memmove(option, option+1, strlen(option+1)+1);
> +                    pstrcpy(value, sizeof(value), "off");
> +                } else if (strncmp(option, "+", 1) == 0) {
> +                    memmove(option, option+1, strlen(option+1)+1);
> +                    pstrcpy(value, sizeof(value), "on");
>                  } else {
>                      pstrcpy(value, sizeof(value), "on");
>                  }

This looks like an interesting idea! However this is much too big a
change to just CC ppc folks on...

Jan, I wonder if this might break slirp's hostfwd option?

Not sure what other options potentially starting with '-' might be
affected. Test cases would be a helpful way of demonstrating that this
change does not have undesired side effects.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Jan Kiszka]

On 2013-11-11 13:41, Andreas Färber wrote:
> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
>> QEMU parser is used for the command line options.
>>
>> "-cpu" parsers in x86 and other architectures should be unaffected
>> by this change.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  util/qemu-option.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index efcb5dc..6c8667c 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>                  if (strncmp(option, "no", 2) == 0) {
>>                      memmove(option, option+2, strlen(option+2)+1);
>>                      pstrcpy(value, sizeof(value), "off");
>> +                } else if (strncmp(option, "-", 1) == 0) {
>> +                    memmove(option, option+1, strlen(option+1)+1);
>> +                    pstrcpy(value, sizeof(value), "off");
>> +                } else if (strncmp(option, "+", 1) == 0) {
>> +                    memmove(option, option+1, strlen(option+1)+1);
>> +                    pstrcpy(value, sizeof(value), "on");
>>                  } else {
>>                      pstrcpy(value, sizeof(value), "on");
>>                  }
> 
> This looks like an interesting idea! However this is much too big a
> change to just CC ppc folks on...
> 
> Jan, I wonder if this might break slirp's hostfwd option?

hostfwd starts with ":" in the simplest case - or what pattern do you
have in mind?

Jan

> 
> Not sure what other options potentially starting with '-' might be
> affected. Test cases would be a helpful way of demonstrating that this
> change does not have undesired side effects.
> 
> Regards,
> Andreas
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Andreas Färber]

Am 11.11.2013 13:52, schrieb Jan Kiszka:
> On 2013-11-11 13:41, Andreas Färber wrote:
>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
>>> QEMU parser is used for the command line options.
>>>
>>> "-cpu" parsers in x86 and other architectures should be unaffected
>>> by this change.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  util/qemu-option.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>>> index efcb5dc..6c8667c 100644
>>> --- a/util/qemu-option.c
>>> +++ b/util/qemu-option.c
>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>                  if (strncmp(option, "no", 2) == 0) {
>>>                      memmove(option, option+2, strlen(option+2)+1);
>>>                      pstrcpy(value, sizeof(value), "off");
>>> +                } else if (strncmp(option, "-", 1) == 0) {
>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>> +                    pstrcpy(value, sizeof(value), "off");
>>> +                } else if (strncmp(option, "+", 1) == 0) {
>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>> +                    pstrcpy(value, sizeof(value), "on");
>>>                  } else {
>>>                      pstrcpy(value, sizeof(value), "on");
>>>                  }
>>
>> This looks like an interesting idea! However this is much too big a
>> change to just CC ppc folks on...
>>
>> Jan, I wonder if this might break slirp's hostfwd option?
> 
> hostfwd starts with ":" in the simplest case - or what pattern do you
> have in mind?

Ah right, I had :8022-:22 or so in mind and mixed up optional host name
with optional source port.

Basically I'm checking for anything which is using the generic QemuOpts
parsing and where a literal + or - may lead to unexpected parsing
changes with this patch. Without having looked up more context for this
hunk, it should not affect foo=-bar but only where foo= is optional,
such as type names for driver= starting with either character (not aware
of such types though).

Andreas

>> Not sure what other options potentially starting with '-' might be
>> affected. Test cases would be a helpful way of demonstrating that this
>> change does not have undesired side effects.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Igor Mammedov]

On Mon, 11 Nov 2013 13:41:05 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> > This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> > QEMU parser is used for the command line options.
> > 
> > "-cpu" parsers in x86 and other architectures should be unaffected
> > by this change.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >  util/qemu-option.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index efcb5dc..6c8667c 100644
> > --- a/util/qemu-option.c
> > +++ b/util/qemu-option.c
> > @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> >                  if (strncmp(option, "no", 2) == 0) {
> >                      memmove(option, option+2, strlen(option+2)+1);
> >                      pstrcpy(value, sizeof(value), "off");
> > +                } else if (strncmp(option, "-", 1) == 0) {
> > +                    memmove(option, option+1, strlen(option+1)+1);
> > +                    pstrcpy(value, sizeof(value), "off");
> > +                } else if (strncmp(option, "+", 1) == 0) {
> > +                    memmove(option, option+1, strlen(option+1)+1);
> > +                    pstrcpy(value, sizeof(value), "on");
> >                  } else {
> >                      pstrcpy(value, sizeof(value), "on");
> >                  }
> 
> This looks like an interesting idea! However this is much too big a
> change to just CC ppc folks on...
> 
> Jan, I wonder if this might break slirp's hostfwd option?
> 
> Not sure what other options potentially starting with '-' might be
> affected. Test cases would be a helpful way of demonstrating that this
> change does not have undesired side effects.
on x86 there is several value fixups for compatibility reason and a manual
value parsing in cpu_x86_parse_featurestr(), so above won't just work there.

> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Alexey Kardashevskiy]

On 11/12/2013 01:25 AM, Igor Mammedov wrote:
> On Mon, 11 Nov 2013 13:41:05 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
>>> QEMU parser is used for the command line options.
>>>
>>> "-cpu" parsers in x86 and other architectures should be unaffected
>>> by this change.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  util/qemu-option.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>>> index efcb5dc..6c8667c 100644
>>> --- a/util/qemu-option.c
>>> +++ b/util/qemu-option.c
>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>                  if (strncmp(option, "no", 2) == 0) {
>>>                      memmove(option, option+2, strlen(option+2)+1);
>>>                      pstrcpy(value, sizeof(value), "off");
>>> +                } else if (strncmp(option, "-", 1) == 0) {
>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>> +                    pstrcpy(value, sizeof(value), "off");
>>> +                } else if (strncmp(option, "+", 1) == 0) {
>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>> +                    pstrcpy(value, sizeof(value), "on");
>>>                  } else {
>>>                      pstrcpy(value, sizeof(value), "on");
>>>                  }
>>
>> This looks like an interesting idea! However this is much too big a
>> change to just CC ppc folks on...
>>
>> Jan, I wonder if this might break slirp's hostfwd option?
>>
>> Not sure what other options potentially starting with '-' might be
>> affected. Test cases would be a helpful way of demonstrating that this
>> change does not have undesired side effects.
> on x86 there is several value fixups for compatibility reason and a manual
> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.


What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
handled two patches below? As I see, even static properties will work there
fine.




-- 
Alexey

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Igor Mammedov]

On Tue, 12 Nov 2013 10:49:58 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
> > On Mon, 11 Nov 2013 13:41:05 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> > 
> >> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> >>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> >>> QEMU parser is used for the command line options.
> >>>
> >>> "-cpu" parsers in x86 and other architectures should be unaffected
> >>> by this change.
> >>>
> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> >>>  util/qemu-option.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/util/qemu-option.c b/util/qemu-option.c
> >>> index efcb5dc..6c8667c 100644
> >>> --- a/util/qemu-option.c
> >>> +++ b/util/qemu-option.c
> >>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> >>>                  if (strncmp(option, "no", 2) == 0) {
> >>>                      memmove(option, option+2, strlen(option+2)+1);
> >>>                      pstrcpy(value, sizeof(value), "off");
> >>> +                } else if (strncmp(option, "-", 1) == 0) {
> >>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>> +                    pstrcpy(value, sizeof(value), "off");
> >>> +                } else if (strncmp(option, "+", 1) == 0) {
> >>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>> +                    pstrcpy(value, sizeof(value), "on");
> >>>                  } else {
> >>>                      pstrcpy(value, sizeof(value), "on");
> >>>                  }
> >>
> >> This looks like an interesting idea! However this is much too big a
> >> change to just CC ppc folks on...
> >>
> >> Jan, I wonder if this might break slirp's hostfwd option?
> >>
> >> Not sure what other options potentially starting with '-' might be
> >> affected. Test cases would be a helpful way of demonstrating that this
> >> change does not have undesired side effects.
> > on x86 there is several value fixups for compatibility reason and a manual
> > value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
> 
> 
> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
> handled two patches below? As I see, even static properties will work there
> fine.
There is legacy code that is kept for CLI compatibility reasons.
Please, look at following features in cpu_x86_parse_featurestr():
  xlevel, tsc-freq hv-spinlocks
the rest feature flags on x86 should be handled just fine by your patch,
once x86properties series is applied. 

that's why we are talking about parser hook that could be overridden
by target if necessary.

PS:
extending QemuOpts to parsing +/-opts format, seems like good workaround
above problem. But I was under impression that general movement was to convert
custom formats to canonical format "prop=value".

> 
> 
> -- 
> Alexey
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Alexey Kardashevskiy]

On 12.11.2013 20:58, Igor Mammedov wrote:
> On Tue, 12 Nov 2013 10:49:58 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
>>> On Mon, 11 Nov 2013 13:41:05 +0100
>>> Andreas Färber <afaerber@suse.de> wrote:
>>>
>>>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>>>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
>>>>> QEMU parser is used for the command line options.
>>>>>
>>>>> "-cpu" parsers in x86 and other architectures should be unaffected
>>>>> by this change.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>>  util/qemu-option.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>>>>> index efcb5dc..6c8667c 100644
>>>>> --- a/util/qemu-option.c
>>>>> +++ b/util/qemu-option.c
>>>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>>>                  if (strncmp(option, "no", 2) == 0) {
>>>>>                      memmove(option, option+2, strlen(option+2)+1);
>>>>>                      pstrcpy(value, sizeof(value), "off");
>>>>> +                } else if (strncmp(option, "-", 1) == 0) {
>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>>>> +                    pstrcpy(value, sizeof(value), "off");
>>>>> +                } else if (strncmp(option, "+", 1) == 0) {
>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>>>> +                    pstrcpy(value, sizeof(value), "on");
>>>>>                  } else {
>>>>>                      pstrcpy(value, sizeof(value), "on");
>>>>>                  }
>>>>
>>>> This looks like an interesting idea! However this is much too big a
>>>> change to just CC ppc folks on...
>>>>
>>>> Jan, I wonder if this might break slirp's hostfwd option?
>>>>
>>>> Not sure what other options potentially starting with '-' might be
>>>> affected. Test cases would be a helpful way of demonstrating that this
>>>> change does not have undesired side effects.
>>> on x86 there is several value fixups for compatibility reason and a manual
>>> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
>>
>>
>> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
>> handled two patches below? As I see, even static properties will work there
>> fine.
> There is legacy code that is kept for CLI compatibility reasons.
> Please, look at following features in cpu_x86_parse_featurestr():
>   xlevel, tsc-freq hv-spinlocks

Ok, I do not know for sure if static properties support setters/getters
(they do not if I remember correct) but what does prevent these x86
properties from being _dynamic_?


> the rest feature flags on x86 should be handled just fine by your patch,
> once x86properties series is applied. 
> 
> that's why we are talking about parser hook that could be overridden
> by target if necessary.

This part confuses me the most. I thought I added the hook and I did not
change other than PPC archs so my patches should have gone quite easily
to upstream but instead I was told (I think I was but I could
misunderstand) that other folks may be unhappy that my stuff does not
support +foo/-foo (which could be added later).

Could you please point me to the x86properties patch(es) which everybody
is waiting for? Thanks!

> PS:
> extending QemuOpts to parsing +/-opts format, seems like good workaround
> above problem. But I was under impression that general movement was to convert
> custom formats to canonical format "prop=value".

Heh. I do not understand movements in the qemu project most of the time
:) I thought I could have added "compat" to PowerPC CPU as others did
but I was so wrong :)



-- 
With best regards

Alexey Kardashevskiy -- icq: 52150396

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Andreas Färber]

Am 12.11.2013 13:39, schrieb Alexey Kardashevskiy:
> On 12.11.2013 20:58, Igor Mammedov wrote:
>> PS:
>> extending QemuOpts to parsing +/-opts format, seems like good workaround
>> above problem. But I was under impression that general movement was to convert
>> custom formats to canonical format "prop=value".
> 
> Heh. I do not understand movements in the qemu project most of the time
> :) I thought I could have added "compat" to PowerPC CPU as others did
> but I was so wrong :)

Hey, I instructed you how to do exactly that, with a const char *
argument, but you chose rather to experiment more with QemuOpts. ;)
Don't blame us! :-)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Alexey Kardashevskiy]

On 11/12/2013 11:45 PM, Andreas Färber wrote:
> Am 12.11.2013 13:39, schrieb Alexey Kardashevskiy:
>> On 12.11.2013 20:58, Igor Mammedov wrote:
>>> PS:
>>> extending QemuOpts to parsing +/-opts format, seems like good workaround
>>> above problem. But I was under impression that general movement was to convert
>>> custom formats to canonical format "prop=value".
>>
>> Heh. I do not understand movements in the qemu project most of the time
>> :) I thought I could have added "compat" to PowerPC CPU as others did
>> but I was so wrong :)
> 
> Hey, I instructed you how to do exactly that, with a const char *
> argument, but you chose rather to experiment more with QemuOpts. ;)
> Don't blame us! :-)


So instead of reusing QemuOpts infrastructure, you suggest to re-implement
the parser one more time, don't you? I still cannot believe it.



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Paolo Bonzini]

Il 12/11/2013 13:45, Andreas Färber ha scritto:
>> > Heh. I do not understand movements in the qemu project most of the time
>> > :) I thought I could have added "compat" to PowerPC CPU as others did
>> > but I was so wrong :)
> Hey, I instructed you how to do exactly that, with a const char *
> argument, but you chose rather to experiment more with QemuOpts. ;)
> Don't blame us! :-)

At least you had a healthy dose of smileys! :)  (Me too, apparently).

I like Alexey's idea.

Paolo

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Igor Mammedov]

On Tue, 12 Nov 2013 23:39:27 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 12.11.2013 20:58, Igor Mammedov wrote:
> > On Tue, 12 Nov 2013 10:49:58 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
> >>> On Mon, 11 Nov 2013 13:41:05 +0100
> >>> Andreas Färber <afaerber@suse.de> wrote:
> >>>
> >>>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> >>>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> >>>>> QEMU parser is used for the command line options.
> >>>>>
> >>>>> "-cpu" parsers in x86 and other architectures should be unaffected
> >>>>> by this change.
> >>>>>
> >>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>> ---
> >>>>>  util/qemu-option.c | 6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
> >>>>> index efcb5dc..6c8667c 100644
> >>>>> --- a/util/qemu-option.c
> >>>>> +++ b/util/qemu-option.c
> >>>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> >>>>>                  if (strncmp(option, "no", 2) == 0) {
> >>>>>                      memmove(option, option+2, strlen(option+2)+1);
> >>>>>                      pstrcpy(value, sizeof(value), "off");
> >>>>> +                } else if (strncmp(option, "-", 1) == 0) {
> >>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>> +                    pstrcpy(value, sizeof(value), "off");
> >>>>> +                } else if (strncmp(option, "+", 1) == 0) {
> >>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>> +                    pstrcpy(value, sizeof(value), "on");
> >>>>>                  } else {
> >>>>>                      pstrcpy(value, sizeof(value), "on");
> >>>>>                  }
> >>>>
> >>>> This looks like an interesting idea! However this is much too big a
> >>>> change to just CC ppc folks on...
> >>>>
> >>>> Jan, I wonder if this might break slirp's hostfwd option?
> >>>>
> >>>> Not sure what other options potentially starting with '-' might be
> >>>> affected. Test cases would be a helpful way of demonstrating that this
> >>>> change does not have undesired side effects.
> >>> on x86 there is several value fixups for compatibility reason and a manual
> >>> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
> >>
> >>
> >> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
> >> handled two patches below? As I see, even static properties will work there
> >> fine.
> > There is legacy code that is kept for CLI compatibility reasons.
> > Please, look at following features in cpu_x86_parse_featurestr():
> >   xlevel, tsc-freq hv-spinlocks
> 
> Ok, I do not know for sure if static properties support setters/getters
> (they do not if I remember correct) but what does prevent these x86
> properties from being _dynamic_?
nothing, except of:
 * it's better to keep CPU device model clean from legacy hacks so that legacy
   silent fixups of invalid values won't be available via other interfaces
   except of CLI. That will force users to use correct property names/values
   and not break old users that use legacy CLI options.


> 
> > the rest feature flags on x86 should be handled just fine by your patch,
> > once x86properties series is applied. 
> > 
> > that's why we are talking about parser hook that could be overridden
> > by target if necessary.
> 
> This part confuses me the most. I thought I added the hook and I did not
> change other than PPC archs so my patches should have gone quite easily
> to upstream but instead I was told (I think I was but I could
> misunderstand) that other folks may be unhappy that my stuff does not
> support +foo/-foo (which could be added later).
> 
> Could you please point me to the x86properties patch(es) which everybody
> is waiting for? Thanks!
latest is available at
https://github.com/imammedo/qemu/tree/x86-cpu-properties.v10.1
which basically is a rebase with fixed conflicts of v9
http://comments.gmane.org/gmane.comp.emulators.qemu/222284

> > PS:
> > extending QemuOpts to parsing +/-opts format, seems like good workaround
> > above problem. But I was under impression that general movement was to convert
> > custom formats to canonical format "prop=value".
> 
> Heh. I do not understand movements in the qemu project most of the time
> :) I thought I could have added "compat" to PowerPC CPU as others did
> but I was so wrong :)
> 
> 
> 
> -- 
> With best regards
> 
> Alexey Kardashevskiy -- icq: 52150396


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Alexey Kardashevskiy]

On 11/13/2013 12:11 AM, Igor Mammedov wrote:
> On Tue, 12 Nov 2013 23:39:27 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 12.11.2013 20:58, Igor Mammedov wrote:
>>> On Tue, 12 Nov 2013 10:49:58 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
>>>>> On Mon, 11 Nov 2013 13:41:05 +0100
>>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>>
>>>>>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>>>>>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
>>>>>>> QEMU parser is used for the command line options.
>>>>>>>
>>>>>>> "-cpu" parsers in x86 and other architectures should be unaffected
>>>>>>> by this change.
>>>>>>>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> ---
>>>>>>>  util/qemu-option.c | 6 ++++++
>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>>>>>>> index efcb5dc..6c8667c 100644
>>>>>>> --- a/util/qemu-option.c
>>>>>>> +++ b/util/qemu-option.c
>>>>>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>>>>>                  if (strncmp(option, "no", 2) == 0) {
>>>>>>>                      memmove(option, option+2, strlen(option+2)+1);
>>>>>>>                      pstrcpy(value, sizeof(value), "off");
>>>>>>> +                } else if (strncmp(option, "-", 1) == 0) {
>>>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>>>>>> +                    pstrcpy(value, sizeof(value), "off");
>>>>>>> +                } else if (strncmp(option, "+", 1) == 0) {
>>>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
>>>>>>> +                    pstrcpy(value, sizeof(value), "on");
>>>>>>>                  } else {
>>>>>>>                      pstrcpy(value, sizeof(value), "on");
>>>>>>>                  }
>>>>>>
>>>>>> This looks like an interesting idea! However this is much too big a
>>>>>> change to just CC ppc folks on...
>>>>>>
>>>>>> Jan, I wonder if this might break slirp's hostfwd option?
>>>>>>
>>>>>> Not sure what other options potentially starting with '-' might be
>>>>>> affected. Test cases would be a helpful way of demonstrating that this
>>>>>> change does not have undesired side effects.
>>>>> on x86 there is several value fixups for compatibility reason and a manual
>>>>> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
>>>>
>>>>
>>>> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
>>>> handled two patches below? As I see, even static properties will work there
>>>> fine.
>>> There is legacy code that is kept for CLI compatibility reasons.
>>> Please, look at following features in cpu_x86_parse_featurestr():
>>>   xlevel, tsc-freq hv-spinlocks
>>
>> Ok, I do not know for sure if static properties support setters/getters
>> (they do not if I remember correct) but what does prevent these x86
>> properties from being _dynamic_?
> nothing, except of:
>  * it's better to keep CPU device model clean from legacy hacks so that legacy
>    silent fixups of invalid values won't be available via other interfaces
>    except of CLI. That will force users to use correct property names/values
>    and not break old users that use legacy CLI options.
>
>>> the rest feature flags on x86 should be handled just fine by your patch,
>>> once x86properties series is applied. 
>>>
>>> that's why we are talking about parser hook that could be overridden
>>> by target if necessary.
>>
>> This part confuses me the most. I thought I added the hook and I did not
>> change other than PPC archs so my patches should have gone quite easily
>> to upstream but instead I was told (I think I was but I could
>> misunderstand) that other folks may be unhappy that my stuff does not
>> support +foo/-foo (which could be added later).
>>
>> Could you please point me to the x86properties patch(es) which everybody
>> is waiting for? Thanks!
> latest is available at
> https://github.com/imammedo/qemu/tree/x86-cpu-properties.v10.1
> which basically is a rebase with fixed conflicts of v9
> http://comments.gmane.org/gmane.comp.emulators.qemu/222284


Wow. This explains a lot. Thanks. Is there any plan to use QemuOpts for all
of this, instead of cpu_x86_parse_featurestr()?


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Igor Mammedov]

On Wed, 13 Nov 2013 13:07:26 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 11/13/2013 12:11 AM, Igor Mammedov wrote:
> > On Tue, 12 Nov 2013 23:39:27 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> On 12.11.2013 20:58, Igor Mammedov wrote:
> >>> On Tue, 12 Nov 2013 10:49:58 +1100
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
> >>>>> On Mon, 11 Nov 2013 13:41:05 +0100
> >>>>> Andreas Färber <afaerber@suse.de> wrote:
> >>>>>
> >>>>>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> >>>>>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> >>>>>>> QEMU parser is used for the command line options.
> >>>>>>>
> >>>>>>> "-cpu" parsers in x86 and other architectures should be unaffected
> >>>>>>> by this change.
> >>>>>>>
> >>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>>> ---
> >>>>>>>  util/qemu-option.c | 6 ++++++
> >>>>>>>  1 file changed, 6 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
> >>>>>>> index efcb5dc..6c8667c 100644
> >>>>>>> --- a/util/qemu-option.c
> >>>>>>> +++ b/util/qemu-option.c
> >>>>>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> >>>>>>>                  if (strncmp(option, "no", 2) == 0) {
> >>>>>>>                      memmove(option, option+2, strlen(option+2)+1);
> >>>>>>>                      pstrcpy(value, sizeof(value), "off");
> >>>>>>> +                } else if (strncmp(option, "-", 1) == 0) {
> >>>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>>>> +                    pstrcpy(value, sizeof(value), "off");
> >>>>>>> +                } else if (strncmp(option, "+", 1) == 0) {
> >>>>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>>>> +                    pstrcpy(value, sizeof(value), "on");
> >>>>>>>                  } else {
> >>>>>>>                      pstrcpy(value, sizeof(value), "on");
> >>>>>>>                  }
> >>>>>>
> >>>>>> This looks like an interesting idea! However this is much too big a
> >>>>>> change to just CC ppc folks on...
> >>>>>>
> >>>>>> Jan, I wonder if this might break slirp's hostfwd option?
> >>>>>>
> >>>>>> Not sure what other options potentially starting with '-' might be
> >>>>>> affected. Test cases would be a helpful way of demonstrating that this
> >>>>>> change does not have undesired side effects.
> >>>>> on x86 there is several value fixups for compatibility reason and a manual
> >>>>> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
> >>>>
> >>>>
> >>>> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
> >>>> handled two patches below? As I see, even static properties will work there
> >>>> fine.
> >>> There is legacy code that is kept for CLI compatibility reasons.
> >>> Please, look at following features in cpu_x86_parse_featurestr():
> >>>   xlevel, tsc-freq hv-spinlocks
> >>
> >> Ok, I do not know for sure if static properties support setters/getters
> >> (they do not if I remember correct) but what does prevent these x86
> >> properties from being _dynamic_?
> > nothing, except of:
> >  * it's better to keep CPU device model clean from legacy hacks so that legacy
> >    silent fixups of invalid values won't be available via other interfaces
> >    except of CLI. That will force users to use correct property names/values
> >    and not break old users that use legacy CLI options.
> >
> >>> the rest feature flags on x86 should be handled just fine by your patch,
> >>> once x86properties series is applied. 
> >>>
> >>> that's why we are talking about parser hook that could be overridden
> >>> by target if necessary.
> >>
> >> This part confuses me the most. I thought I added the hook and I did not
> >> change other than PPC archs so my patches should have gone quite easily
> >> to upstream but instead I was told (I think I was but I could
> >> misunderstand) that other folks may be unhappy that my stuff does not
> >> support +foo/-foo (which could be added later).
> >>
> >> Could you please point me to the x86properties patch(es) which everybody
> >> is waiting for? Thanks!
> > latest is available at
> > https://github.com/imammedo/qemu/tree/x86-cpu-properties.v10.1
> > which basically is a rebase with fixed conflicts of v9
> > http://comments.gmane.org/gmane.comp.emulators.qemu/222284
> 
> 
> Wow. This explains a lot. Thanks. Is there any plan to use QemuOpts for all
> of this, instead of cpu_x86_parse_featurestr()?
the plan was to keep, +/-/fixups as legacy in target specific code (x86,
sparc) not polluting the rest targets. For not affected targets use only
foo=val notation in CLI/monitor.

So providing a generic parser of cpu_model string for most targets and having
a hook override with custom parser on x86,sparc would be one of simplest
solutions.

> 
> 
> -- 
> Alexey


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Paolo Bonzini]

Il 12/11/2013 10:58, Igor Mammedov ha scritto:
> extending QemuOpts to parsing ±opts format, seems like good workaround
> above problem. But I was under impression that general movement was to convert
> custom formats to canonical format "prop=value".

I think the general movement is to convert things to QemuOpts.  As long
as all compound options use QemuOpts, they are consistent and any
syntactic sugar will apply to all in the same way.

I, for one, can never remember if it is =on, =true, =yes (and
interestingly =no works but =yes doesn't).  So I welcome a new
completely different syntax that doesn't have this problem. :)


Paolo

[Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Alexey Kardashevskiy]

This adds a macro to calculate the highest bit set. If used on constant
values, no code will be generated.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/qemu/bitops.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 304c90c..98ba42a 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -23,6 +23,18 @@
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
 
+#define __BITNR(m, n)  ((m) == ((m) & (1<<(n)))) ? (n) :
+#define BITNR(m) \
+    __BITNR((m), 31) __BITNR((m), 30) __BITNR((m), 29) __BITNR((m), 28) \
+    __BITNR((m), 27) __BITNR((m), 26) __BITNR((m), 25) __BITNR((m), 24) \
+    __BITNR((m), 23) __BITNR((m), 22) __BITNR((m), 21) __BITNR((m), 20) \
+    __BITNR((m), 19) __BITNR((m), 18) __BITNR((m), 17) __BITNR((m), 16) \
+    __BITNR((m), 15) __BITNR((m), 14) __BITNR((m), 13) __BITNR((m), 12) \
+    __BITNR((m), 11) __BITNR((m), 10) __BITNR((m),  9) __BITNR((m),  8) \
+    __BITNR((m),  7) __BITNR((m),  6) __BITNR((m),  5) __BITNR((m),  4) \
+    __BITNR((m),  3) __BITNR((m),  2) __BITNR((m),  1) __BITNR((m),  0) \
+    -1
+
 /**
  * set_bit - Set a bit in memory
  * @nr: the bit to set
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Andreas Färber]

Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> This adds a macro to calculate the highest bit set.

Isn't that already available as ffs / clz GCC builtin with wrapper in
qemu/bitops.h? What's the difference to your macro? CC'ing Paolo.

Andreas

> If used on constant
> values, no code will be generated.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/qemu/bitops.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index 304c90c..98ba42a 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -23,6 +23,18 @@
>  #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
>  #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>  
> +#define __BITNR(m, n)  ((m) == ((m) & (1<<(n)))) ? (n) :
> +#define BITNR(m) \
> +    __BITNR((m), 31) __BITNR((m), 30) __BITNR((m), 29) __BITNR((m), 28) \
> +    __BITNR((m), 27) __BITNR((m), 26) __BITNR((m), 25) __BITNR((m), 24) \
> +    __BITNR((m), 23) __BITNR((m), 22) __BITNR((m), 21) __BITNR((m), 20) \
> +    __BITNR((m), 19) __BITNR((m), 18) __BITNR((m), 17) __BITNR((m), 16) \
> +    __BITNR((m), 15) __BITNR((m), 14) __BITNR((m), 13) __BITNR((m), 12) \
> +    __BITNR((m), 11) __BITNR((m), 10) __BITNR((m),  9) __BITNR((m),  8) \
> +    __BITNR((m),  7) __BITNR((m),  6) __BITNR((m),  5) __BITNR((m),  4) \
> +    __BITNR((m),  3) __BITNR((m),  2) __BITNR((m),  1) __BITNR((m),  0) \
> +    -1
> +
>  /**
>   * set_bit - Set a bit in memory
>   * @nr: the bit to set
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Alexey Kardashevskiy]

On 11/11/2013 10:57 PM, Andreas Färber wrote:
> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>> This adds a macro to calculate the highest bit set.
> 
> Isn't that already available as ffs / clz GCC builtin with wrapper in
> qemu/bitops.h? What's the difference to your macro? CC'ing Paolo.
>


I am ignorant and did not know this simple fact. Sorry. Drop this patch.




> Andreas
> 
>> If used on constant
>> values, no code will be generated.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/qemu/bitops.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index 304c90c..98ba42a 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>> @@ -23,6 +23,18 @@
>>  #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
>>  #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>>  
>> +#define __BITNR(m, n)  ((m) == ((m) & (1<<(n)))) ? (n) :
>> +#define BITNR(m) \
>> +    __BITNR((m), 31) __BITNR((m), 30) __BITNR((m), 29) __BITNR((m), 28) \
>> +    __BITNR((m), 27) __BITNR((m), 26) __BITNR((m), 25) __BITNR((m), 24) \
>> +    __BITNR((m), 23) __BITNR((m), 22) __BITNR((m), 21) __BITNR((m), 20) \
>> +    __BITNR((m), 19) __BITNR((m), 18) __BITNR((m), 17) __BITNR((m), 16) \
>> +    __BITNR((m), 15) __BITNR((m), 14) __BITNR((m), 13) __BITNR((m), 12) \
>> +    __BITNR((m), 11) __BITNR((m), 10) __BITNR((m),  9) __BITNR((m),  8) \
>> +    __BITNR((m),  7) __BITNR((m),  6) __BITNR((m),  5) __BITNR((m),  4) \
>> +    __BITNR((m),  3) __BITNR((m),  2) __BITNR((m),  1) __BITNR((m),  0) \
>> +    -1
>> +
>>  /**
>>   * set_bit - Set a bit in memory
>>   * @nr: the bit to set
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Alexey Kardashevskiy]

On 11/11/2013 10:57 PM, Andreas Färber wrote:
> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>> This adds a macro to calculate the highest bit set.
> 
> Isn't that already available as ffs / clz GCC builtin with wrapper in
> qemu/bitops.h? What's the difference to your macro? CC'ing Paolo.


I looked further and did not find any use of ffs/clz so I wonder what did
you mean about bitops.h and what did I miss? I am confused.

So I would suggest the following instead (if I really needed this BITNR but
I really do not :) )

===
    bitops: add BITNR macro

    This adds a macro to calculate the highest single bit set. If more than
    one bit is set, returns -1.

    Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 304c90c..24361b2 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -23,6 +23,8 @@
 #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
 #define BITS_TO_LONGS(nr)      DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))

+#define BITNR(m)   (__builtin_popcount(m) == 1)?(__builtin_ffs(m) - 1) : -1
+
 /**
  * set_bit - Set a bit in memory
  * @nr: the bit to set
===



> 
> Andreas
> 
>> If used on constant
>> values, no code will be generated.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/qemu/bitops.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index 304c90c..98ba42a 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>> @@ -23,6 +23,18 @@
>>  #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
>>  #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>>  
>> +#define __BITNR(m, n)  ((m) == ((m) & (1<<(n)))) ? (n) :
>> +#define BITNR(m) \
>> +    __BITNR((m), 31) __BITNR((m), 30) __BITNR((m), 29) __BITNR((m), 28) \
>> +    __BITNR((m), 27) __BITNR((m), 26) __BITNR((m), 25) __BITNR((m), 24) \
>> +    __BITNR((m), 23) __BITNR((m), 22) __BITNR((m), 21) __BITNR((m), 20) \
>> +    __BITNR((m), 19) __BITNR((m), 18) __BITNR((m), 17) __BITNR((m), 16) \
>> +    __BITNR((m), 15) __BITNR((m), 14) __BITNR((m), 13) __BITNR((m), 12) \
>> +    __BITNR((m), 11) __BITNR((m), 10) __BITNR((m),  9) __BITNR((m),  8) \
>> +    __BITNR((m),  7) __BITNR((m),  6) __BITNR((m),  5) __BITNR((m),  4) \
>> +    __BITNR((m),  3) __BITNR((m),  2) __BITNR((m),  1) __BITNR((m),  0) \
>> +    -1
>> +
>>  /**
>>   * set_bit - Set a bit in memory
>>   * @nr: the bit to set
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Paolo Bonzini]

Il 13/11/2013 03:40, Alexey Kardashevskiy ha scritto:
> I looked further and did not find any use of ffs/clz so I wonder what did
> you mean about bitops.h and what did I miss? I am confused.

It's host-utils.h actually.  The reason for the wrappers is twofold:

(1) provide 32/64-bit functions instead of int/long/longlong;

(2) support GCC <3.4.

The wrappers are not usable where you have constant expressions as in
your case.  So your original patch is good IMO, except for the __ at the
beginning of __BITNR.

Paolo

> So I would suggest the following instead (if I really needed this BITNR but
> I really do not :) )
> 
> ===
>     bitops: add BITNR macro
> 
>     This adds a macro to calculate the highest single bit set. If more than
>     one bit is set, returns -1.

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Alexey Kardashevskiy]

On 11/13/2013 11:04 PM, Paolo Bonzini wrote:
> Il 13/11/2013 03:40, Alexey Kardashevskiy ha scritto:
>> I looked further and did not find any use of ffs/clz so I wonder what did
>> you mean about bitops.h and what did I miss? I am confused.
> 
> It's host-utils.h actually.  The reason for the wrappers is twofold:
> 
> (1) provide 32/64-bit functions instead of int/long/longlong;
> 
> (2) support GCC <3.4.
> 
> The wrappers are not usable where you have constant expressions as in
> your case.  So your original patch is good IMO, except for the __ at the
> beginning of __BITNR.


There are 2 macros, one (BITNR) uses the other (__BITNR). What would the
good name be for BITNR then?


> 
> Paolo
> 
>> So I would suggest the following instead (if I really needed this BITNR but
>> I really do not :) )
>>
>> ===
>>     bitops: add BITNR macro
>>
>>     This adds a macro to calculate the highest single bit set. If more than
>>     one bit is set, returns -1.
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 5/6] bitops: add BITNR macro

[Paolo Bonzini]

Il 14/11/2013 06:51, Alexey Kardashevskiy ha scritto:
>> > The wrappers are not usable where you have constant expressions as in
>> > your case.  So your original patch is good IMO, except for the __ at the
>> > beginning of __BITNR.
> 
> There are 2 macros, one (BITNR) uses the other (__BITNR). What would the
> good name be for BITNR then?

I don't know, perhaps BITNR and BITNR_?

Names starting with __ or _+uppercase are reserved by the C standard.

Paolo

[Qemu-devel] [PATCH v3 6/6] target-ppc: demonstrate new "vsx" property

[Alexey Kardashevskiy]

This patch is to demonstrate a static property handling in PowerPC.
Running QEMU with -cpu host,-vsx disables VSX bit in
PowerPCCPU::env::flags.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-ppc/translate_init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index df0d81c..60ea235 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -29,6 +29,7 @@
 #include "mmu-hash64.h"
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
+#include "hw/qdev-properties.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -8740,6 +8741,12 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 
     dc->fw_name = "PowerPC,UNKNOWN";
     cc->parse_options = cpu_default_parse_options_func;
+
+    static Property powerpc_properties[] = {
+        DEFINE_PROP_BIT("vsx", PowerPCCPU, env.flags, BITNR(POWERPC_FLAG_VSX), false),
+        DEFINE_PROP_END_OF_LIST(),
+    };
+    dc->props = powerpc_properties;
 }
 
 static const TypeInfo ppc_cpu_type_info = {
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3 6/6] target-ppc: demonstrate new "vsx" property

[Andreas Färber]

Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> This patch is to demonstrate a static property handling in PowerPC.
> Running QEMU with -cpu host,-vsx disables VSX bit in
> PowerPCCPU::env::flags.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  target-ppc/translate_init.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index df0d81c..60ea235 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -29,6 +29,7 @@
>  #include "mmu-hash64.h"
>  #include "qemu/error-report.h"
>  #include "qapi/visitor.h"
> +#include "hw/qdev-properties.h"
>  
>  //#define PPC_DUMP_CPU
>  //#define PPC_DEBUG_SPR
> @@ -8740,6 +8741,12 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
>      cc->parse_options = cpu_default_parse_options_func;
> +
> +    static Property powerpc_properties[] = {
> +        DEFINE_PROP_BIT("vsx", PowerPCCPU, env.flags, BITNR(POWERPC_FLAG_VSX), false),
> +        DEFINE_PROP_END_OF_LIST(),
> +    };
> +    dc->props = powerpc_properties;
>  }
>  
>  static const TypeInfo ppc_cpu_type_info = {

This type of static property looks good to me, but two problems apart
from the BITNR() discussed elsewhere:

1) Won't the default value of false always disable VSX for all models
including POWER7 and higher?

2) Please move the array out of the function to just before the
containing function.

Igor has been facing a similar issue in his refactoring of x86 models
and I believe evaluated to define the properties per model so that they
have different defaults. Another way is overwriting that default in the
model's instance_init or via globals specific to the subtype.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 6/6] target-ppc: demonstrate new "vsx" property

[Alexey Kardashevskiy]

On 11/12/2013 12:31 AM, Andreas Färber wrote:
> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>> This patch is to demonstrate a static property handling in PowerPC.
>> Running QEMU with -cpu host,-vsx disables VSX bit in
>> PowerPCCPU::env::flags.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  target-ppc/translate_init.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index df0d81c..60ea235 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -29,6 +29,7 @@
>>  #include "mmu-hash64.h"
>>  #include "qemu/error-report.h"
>>  #include "qapi/visitor.h"
>> +#include "hw/qdev-properties.h"
>>  
>>  //#define PPC_DUMP_CPU
>>  //#define PPC_DEBUG_SPR
>> @@ -8740,6 +8741,12 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>  
>>      dc->fw_name = "PowerPC,UNKNOWN";
>>      cc->parse_options = cpu_default_parse_options_func;
>> +
>> +    static Property powerpc_properties[] = {
>> +        DEFINE_PROP_BIT("vsx", PowerPCCPU, env.flags, BITNR(POWERPC_FLAG_VSX), false),
>> +        DEFINE_PROP_END_OF_LIST(),
>> +    };
>> +    dc->props = powerpc_properties;
>>  }
>>  
>>  static const TypeInfo ppc_cpu_type_info = {
> 
> This type of static property looks good to me, but two problems apart
> from the BITNR() discussed elsewhere:
> 
> 1) Won't the default value of false always disable VSX for all models
> including POWER7 and higher?


I am not really planning on defining properties per CPU feature, this all
was just to demonstrate an ability to define those.

But even if I was planning, I would set default to true and move the whole
set of properties to POWER7 family and that would be it.

> 2) Please move the array out of the function to just before the
> containing function.

Again, this is just to show. But. Why do you want those properties to be
outside of the function? Less visibility is usually a good thing, no?

> Igor has been facing a similar issue in his refactoring of x86 models
> and I believe evaluated to define the properties per model so that they
> have different defaults. Another way is overwriting that default in the
> model's instance_init or via globals specific to the subtype.

Current x86 in QEMU does not have those CPU families as POWERPC does,
correct? So we have more flexibility with properties and everything...


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 6/6] target-ppc: demonstrate new "vsx" property

[Andreas Färber]

Am 14.11.2013 06:20, schrieb Alexey Kardashevskiy:
> On 11/12/2013 12:31 AM, Andreas Färber wrote:
>> 2) Please move the array out of the function to just before the
>> containing function.
> 
> Again, this is just to show. But. Why do you want those properties to be
> outside of the function? Less visibility is usually a good thing, no?

We used to have ->props = (...[]) {...}; and Anthony specifically
refactored it to have a static variable one level down, just before the
function assigning it. It was a scripted conversion in the context of
QOM introduction, maybe the archives will give you insight about why
exactly it was done.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg