[Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Marcel Apfelbaum]

Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.

This results in a Qemu crash if a non string option is queried using qemu opts.
Fix this by querying machine properties through designated wrappers.

I hope I didn't miss anything.
Comments are appreciated as always.

Thanks,
Marcel

Marcel Apfelbaum (8):
  machine: query iommu machine property rather than qemu opts
  hw/machine: kernel-irqchip property support for allowed/required
  machine: query kernel-irqchip machine property rather than qemu opts
  kvm: add machine state to kvm_arch_init
  machine: query kvm-shadow-mem machine property rather than qemu opts
  machine: query phandle-start machine property rather than qemu opts
  machine: query dump-guest-core machine property rather than qemu opts
  machine: query mem-merge machine property rather than qemu opts

 device_tree.c        |  5 ++---
 exec.c               |  6 +++---
 hw/core/machine.c    | 52 +++++++++++++++++++++++++++++++++++++++++++---------
 hw/pci-host/q35.c    |  2 +-
 hw/ppc/e500.c        | 16 +++++-----------
 hw/ppc/spapr.c       | 16 ++++++----------
 include/hw/boards.h  | 10 +++++++++-
 include/sysemu/kvm.h |  2 +-
 kvm-all.c            |  8 ++++----
 target-arm/kvm.c     |  2 +-
 target-i386/kvm.c    |  5 ++---
 target-mips/kvm.c    |  2 +-
 target-ppc/kvm.c     |  2 +-
 target-s390x/kvm.c   |  2 +-
 14 files changed, 80 insertions(+), 50 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/8] machine: query iommu machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing iommu parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c   | 5 +++++
 hw/pci-host/q35.c   | 2 +-
 include/hw/boards.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index fbd91be..096eb10 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
     return machine->usb;
 }
 
+bool machine_iommu(MachineState *machine)
+{
+    return machine->iommu;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b20bad8..dfd8bbf 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -415,7 +415,7 @@ static int mch_init(PCIDevice *d)
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
     /* Intel IOMMU (VT-d) */
-    if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) {
+    if (machine_iommu(current_machine)) {
         mch_init_dmar(mch);
     }
     return 0;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3ddc449..a12f041 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -66,6 +66,7 @@ MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
 bool machine_usb(MachineState *machine);
+bool machine_iommu(MachineState *machine);
 
 /**
  * MachineClass:
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/8] machine: query iommu machine property rather than qemu opts

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> Fixes a QEMU crash when passing iommu parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/core/machine.c   | 5 +++++
>  hw/pci-host/q35.c   | 2 +-
>  include/hw/boards.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fbd91be..096eb10 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>      return machine->usb;
>  }
>  
> +bool machine_iommu(MachineState *machine)
> +{
> +    return machine->iommu;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,

What does this buy us over a straight current_machine->iommu?

Same for the other wrapper functions.

[...]

Re: [Qemu-devel] [PATCH 1/8] machine: query iommu machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 06:47 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> Fixes a QEMU crash when passing iommu parameter in command line.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/core/machine.c   | 5 +++++
>>   hw/pci-host/q35.c   | 2 +-
>>   include/hw/boards.h | 1 +
>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index fbd91be..096eb10 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>>       return machine->usb;
>>   }
>>
>> +bool machine_iommu(MachineState *machine)
>> +{
>> +    return machine->iommu;
>> +}
>> +
>>   static const TypeInfo machine_info = {
>>       .name = TYPE_MACHINE,
>>       .parent = TYPE_OBJECT,
>
> What does this buy us over a straight current_machine->iommu?
Following QOM guidelines/conventions all object fields are private
to machine files only. The *only* ways that they can be exposed are:
1. A wrapper
2. object_property_get...

I chose the wrapper because the other variant would be really
ugly and should be used only on a generic code.

This is the real reason I used this, but pure theoretical speaking
using  wrappers will give us the opportunity to change machine_iommu
implementation without the need to change the call sites.

To wrap it up, I just followed previous comments I received on QOM handling.

Thanks,
Marcel

>
> Same for the other wrapper functions.
>
> [...]
>

Re: [Qemu-devel] [PATCH 1/8] machine: query iommu machine property rather than qemu opts

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 02/04/2015 06:47 PM, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>
>>> Fixes a QEMU crash when passing iommu parameter in command line.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>> ---
>>>   hw/core/machine.c   | 5 +++++
>>>   hw/pci-host/q35.c   | 2 +-
>>>   include/hw/boards.h | 1 +
>>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index fbd91be..096eb10 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>>>       return machine->usb;
>>>   }
>>>
>>> +bool machine_iommu(MachineState *machine)
>>> +{
>>> +    return machine->iommu;
>>> +}
>>> +
>>>   static const TypeInfo machine_info = {
>>>       .name = TYPE_MACHINE,
>>>       .parent = TYPE_OBJECT,
>>
>> What does this buy us over a straight current_machine->iommu?
> Following QOM guidelines/conventions all object fields are private
> to machine files only. The *only* ways that they can be exposed are:
> 1. A wrapper
> 2. object_property_get...
>
> I chose the wrapper because the other variant would be really
> ugly and should be used only on a generic code.
>
> This is the real reason I used this, but pure theoretical speaking
> using  wrappers will give us the opportunity to change machine_iommu
> implementation without the need to change the call sites.

So we pay the notational and cognitive overhead of wrappers now and
going forward to maybe some day save us fixing up a couple of
machine->iommu instances.

A theory that makes sense for interfacing complex machinery with
non-trivial implementation details becomes silly when applied to a
couple of straightforward configuration flags.  As Doug McIlroy
observed, "KISS became MICAHI: make it complicated and hide it."

> To wrap it up, I just followed previous comments I received on QOM handling.

In other words, it's somebody else's fault.  I'll shut up now ;)

[...]

Re: [Qemu-devel] [PATCH 1/8] machine: query iommu machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing iommu parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Please amend commit message:

Running
     x86_64-softmmu/qemu-system-x86_64 -machine pc,iommu=on -enable-kvm
leads to crash:
     qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
     Aborted (core dumped)

This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option descriptions and moved them to MachineState's QOM properties.

Fix this by querying machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>


> ---
>   hw/core/machine.c   | 5 +++++
>   hw/pci-host/q35.c   | 2 +-
>   include/hw/boards.h | 1 +
>   3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fbd91be..096eb10 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>       return machine->usb;
>   }
>
> +bool machine_iommu(MachineState *machine)
> +{
> +    return machine->iommu;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..dfd8bbf 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -415,7 +415,7 @@ static int mch_init(PCIDevice *d)
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
>       /* Intel IOMMU (VT-d) */
> -    if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) {
> +    if (machine_iommu(current_machine)) {
>           mch_init_dmar(mch);
>       }
>       return 0;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..a12f041 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -66,6 +66,7 @@ MachineClass *find_default_machine(void);
>   extern MachineState *current_machine;
>
>   bool machine_usb(MachineState *machine);
> +bool machine_iommu(MachineState *machine);
>
>   /**
>    * MachineClass:
>

[Qemu-devel] [PATCH 2/8] hw/machine: kernel-irqchip property support for allowed/required

[Marcel Apfelbaum]

The code using kernel-irqchip property requires 'allowed/required'
functionality. Replace machine's kernel_irqchip field with two fields
representing the new functionality and expose them through wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c   | 24 +++++++++++++++---------
 include/hw/boards.h |  5 ++++-
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 096eb10..e04e5ab 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -31,18 +31,12 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp)
     ms->accel = g_strdup(value);
 }
 
-static bool machine_get_kernel_irqchip(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return ms->kernel_irqchip;
-}
-
 static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
 
-    ms->kernel_irqchip = value;
+    ms->kernel_irqchip_allowed = value;
+    ms->kernel_irqchip_required = value;
 }
 
 static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
@@ -289,13 +283,15 @@ static void machine_initfn(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
 
+    ms->kernel_irqchip_allowed = true;
+
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
     object_property_set_description(obj, "accel",
                                     "Accelerator list",
                                     NULL);
     object_property_add_bool(obj, "kernel-irqchip",
-                             machine_get_kernel_irqchip,
+                             NULL,
                              machine_set_kernel_irqchip,
                              NULL);
     object_property_set_description(obj, "kernel-irqchip",
@@ -408,6 +404,16 @@ bool machine_iommu(MachineState *machine)
     return machine->iommu;
 }
 
+bool machine_kernel_irqchip_allowed(MachineState *machine)
+{
+    return machine->kernel_irqchip_allowed;
+}
+
+bool machine_kernel_irqchip_required(MachineState *machine)
+{
+    return machine->kernel_irqchip_required;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index a12f041..69ab606 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -67,6 +67,8 @@ extern MachineState *current_machine;
 
 bool machine_usb(MachineState *machine);
 bool machine_iommu(MachineState *machine);
+bool machine_kernel_irqchip_allowed(MachineState *machine);
+bool machine_kernel_irqchip_required(MachineState *machine);
 
 /**
  * MachineClass:
@@ -125,7 +127,8 @@ struct MachineState {
     /*< public >*/
 
     char *accel;
-    bool kernel_irqchip;
+    bool kernel_irqchip_allowed;
+    bool kernel_irqchip_required;
     int kvm_shadow_mem;
     char *dtb;
     char *dumpdtb;
-- 
2.1.0

[Qemu-devel] [PATCH 3/8] machine: query kernel-irqchip machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing kernel_irqchip parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ppc/e500.c  | 16 +++++-----------
 hw/ppc/spapr.c | 16 ++++++----------
 kvm-all.c      |  6 +++---
 3 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 7e17d18..641cab9 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -731,8 +731,8 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
     return dev;
 }
 
-static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
-                                   qemu_irq **irqs)
+static qemu_irq *ppce500_init_mpic(MachineState *machine, PPCE500Params *params,
+                                   MemoryRegion *ccsr, qemu_irq **irqs)
 {
     qemu_irq *mpic;
     DeviceState *dev = NULL;
@@ -742,17 +742,11 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     mpic = g_new0(qemu_irq, 256);
 
     if (kvm_enabled()) {
-        QemuOpts *machine_opts = qemu_get_machine_opts();
-        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
-                                                "kernel_irqchip", true);
-        bool irqchip_required = qemu_opt_get_bool(machine_opts,
-                                                  "kernel_irqchip", false);
-
-        if (irqchip_allowed) {
+        if (machine_kernel_irqchip_allowed(machine)) {
             dev = ppce500_init_mpic_kvm(params, irqs);
         }
 
-        if (irqchip_required && !dev) {
+        if (machine_kernel_irqchip_required(machine) && !dev) {
             fprintf(stderr, "%s: irqchip requested but unavailable\n",
                     __func__);
             abort();
@@ -876,7 +870,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     memory_region_add_subregion(address_space_mem, params->ccsrbar_base,
                                 ccsr_addr_space);
 
-    mpic = ppce500_init_mpic(params, ccsr_addr_space, irqs);
+    mpic = ppce500_init_mpic(machine, params, ccsr_addr_space, irqs);
 
     /* Serial */
     if (serial_hds[0]) {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..f4b6adb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -123,21 +123,16 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
     return XICS_COMMON(dev);
 }
 
-static XICSState *xics_system_init(int nr_servers, int nr_irqs)
+static XICSState *xics_system_init(MachineState *machine,
+                                   int nr_servers, int nr_irqs)
 {
     XICSState *icp = NULL;
-
     if (kvm_enabled()) {
-        QemuOpts *machine_opts = qemu_get_machine_opts();
-        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
-                                                "kernel_irqchip", true);
-        bool irqchip_required = qemu_opt_get_bool(machine_opts,
-                                                  "kernel_irqchip", false);
-        if (irqchip_allowed) {
+        if (machine_kernel_irqchip_allowed(machine)) {
             icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
         }
 
-        if (irqchip_required && !icp) {
+        if (machine_kernel_irqchip_required(machine) && !icp) {
             perror("Failed to create in-kernel XICS\n");
             abort();
         }
@@ -1420,7 +1415,8 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     /* Set up Interrupt Controller before we create the VCPUs */
-    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
+    spapr->icp = xics_system_init(machine,
+                                  smp_cpus * kvmppc_smt_threads() / smp_threads,
                                   XICS_IRQS);
 
     /* init CPUs */
diff --git a/kvm-all.c b/kvm-all.c
index 2f21a4e..cdb90c5 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1360,11 +1360,11 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
            false);
 }
 
-static int kvm_irqchip_create(KVMState *s)
+static int kvm_irqchip_create(MachineState *machine, KVMState *s)
 {
     int ret;
 
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
+    if (!machine_kernel_irqchip_allowed(machine) ||
         (!kvm_check_extension(s, KVM_CAP_IRQCHIP) &&
          (kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0) < 0))) {
         return 0;
@@ -1603,7 +1603,7 @@ static int kvm_init(MachineState *ms)
         goto err;
     }
 
-    ret = kvm_irqchip_create(s);
+    ret = kvm_irqchip_create(ms, s);
     if (ret < 0) {
         goto err;
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/8] machine: query kernel-irqchip machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing kernel_irqchip parameter in command line.
Please amend commit message:

Running
     x86_64-softmmu/qemu-system-x86_64 -machine pc,kernel_irqchip=on -enable-kvm
leads to crash:
     qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
     Aborted (core dumped)

This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option descriptions and moved them to MachineState's QOM properties.

Fix this by querying machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>   hw/ppc/e500.c  | 16 +++++-----------
>   hw/ppc/spapr.c | 16 ++++++----------
>   kvm-all.c      |  6 +++---
>   3 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 7e17d18..641cab9 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -731,8 +731,8 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
>       return dev;
>   }
>
> -static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
> -                                   qemu_irq **irqs)
> +static qemu_irq *ppce500_init_mpic(MachineState *machine, PPCE500Params *params,
> +                                   MemoryRegion *ccsr, qemu_irq **irqs)
>   {
>       qemu_irq *mpic;
>       DeviceState *dev = NULL;
> @@ -742,17 +742,11 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>       mpic = g_new0(qemu_irq, 256);
>
>       if (kvm_enabled()) {
> -        QemuOpts *machine_opts = qemu_get_machine_opts();
> -        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> -                                                "kernel_irqchip", true);
> -        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> -                                                  "kernel_irqchip", false);
> -
> -        if (irqchip_allowed) {
> +        if (machine_kernel_irqchip_allowed(machine)) {
>               dev = ppce500_init_mpic_kvm(params, irqs);
>           }
>
> -        if (irqchip_required && !dev) {
> +        if (machine_kernel_irqchip_required(machine) && !dev) {
>               fprintf(stderr, "%s: irqchip requested but unavailable\n",
>                       __func__);
>               abort();
> @@ -876,7 +870,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>       memory_region_add_subregion(address_space_mem, params->ccsrbar_base,
>                                   ccsr_addr_space);
>
> -    mpic = ppce500_init_mpic(params, ccsr_addr_space, irqs);
> +    mpic = ppce500_init_mpic(machine, params, ccsr_addr_space, irqs);
>
>       /* Serial */
>       if (serial_hds[0]) {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b560459..f4b6adb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -123,21 +123,16 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>       return XICS_COMMON(dev);
>   }
>
> -static XICSState *xics_system_init(int nr_servers, int nr_irqs)
> +static XICSState *xics_system_init(MachineState *machine,
> +                                   int nr_servers, int nr_irqs)
>   {
>       XICSState *icp = NULL;
> -
>       if (kvm_enabled()) {
> -        QemuOpts *machine_opts = qemu_get_machine_opts();
> -        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> -                                                "kernel_irqchip", true);
> -        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> -                                                  "kernel_irqchip", false);
> -        if (irqchip_allowed) {
> +        if (machine_kernel_irqchip_allowed(machine)) {
>               icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
>           }
>
> -        if (irqchip_required && !icp) {
> +        if (machine_kernel_irqchip_required(machine) && !icp) {
>               perror("Failed to create in-kernel XICS\n");
>               abort();
>           }
> @@ -1420,7 +1415,8 @@ static void ppc_spapr_init(MachineState *machine)
>       }
>
>       /* Set up Interrupt Controller before we create the VCPUs */
> -    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
> +    spapr->icp = xics_system_init(machine,
> +                                  smp_cpus * kvmppc_smt_threads() / smp_threads,
>                                     XICS_IRQS);
>
>       /* init CPUs */
> diff --git a/kvm-all.c b/kvm-all.c
> index 2f21a4e..cdb90c5 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1360,11 +1360,11 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
>              false);
>   }
>
> -static int kvm_irqchip_create(KVMState *s)
> +static int kvm_irqchip_create(MachineState *machine, KVMState *s)
>   {
>       int ret;
>
> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
> +    if (!machine_kernel_irqchip_allowed(machine) ||
>           (!kvm_check_extension(s, KVM_CAP_IRQCHIP) &&
>            (kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0) < 0))) {
>           return 0;
> @@ -1603,7 +1603,7 @@ static int kvm_init(MachineState *ms)
>           goto err;
>       }
>
> -    ret = kvm_irqchip_create(s);
> +    ret = kvm_irqchip_create(ms, s);
>       if (ret < 0) {
>           goto err;
>       }
>

[Qemu-devel] [PATCH 4/8] kvm: add machine state to kvm_arch_init

[Marcel Apfelbaum]

Needed to query machine's properties.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 include/sysemu/kvm.h | 2 +-
 kvm-all.c            | 2 +-
 target-arm/kvm.c     | 2 +-
 target-i386/kvm.c    | 2 +-
 target-mips/kvm.c    | 2 +-
 target-ppc/kvm.c     | 2 +-
 target-s390x/kvm.c   | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 30cb84d..3792463 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -259,7 +259,7 @@ int kvm_arch_get_registers(CPUState *cpu);
 
 int kvm_arch_put_registers(CPUState *cpu, int level);
 
-int kvm_arch_init(KVMState *s);
+int kvm_arch_init(MachineState *ms, KVMState *s);
 
 int kvm_arch_init_vcpu(CPUState *cpu);
 
diff --git a/kvm-all.c b/kvm-all.c
index cdb90c5..673ffb8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1598,7 +1598,7 @@ static int kvm_init(MachineState *ms)
     kvm_resamplefds_allowed =
         (kvm_check_extension(s, KVM_CAP_IRQFD_RESAMPLE) > 0);
 
-    ret = kvm_arch_init(s);
+    ret = kvm_arch_init(ms, s);
     if (ret < 0) {
         goto err;
     }
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 23cefe9..72c1fa1 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -150,7 +150,7 @@ static const TypeInfo host_arm_cpu_type_info = {
     .class_size = sizeof(ARMHostCPUClass),
 };
 
-int kvm_arch_init(KVMState *s)
+int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     /* For ARM interrupt delivery is always asynchronous,
      * whether we are using an in-kernel VGIC or not.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 40d6a14..ce554e4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -840,7 +840,7 @@ static int kvm_get_supported_msrs(KVMState *s)
     return ret;
 }
 
-int kvm_arch_init(KVMState *s)
+int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     uint64_t identity_base = 0xfffbc000;
     uint64_t shadow_mem;
diff --git a/target-mips/kvm.c b/target-mips/kvm.c
index b68191c..4d1f7ea 100644
--- a/target-mips/kvm.c
+++ b/target-mips/kvm.c
@@ -40,7 +40,7 @@ unsigned long kvm_arch_vcpu_id(CPUState *cs)
     return cs->cpu_index;
 }
 
-int kvm_arch_init(KVMState *s)
+int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     /* MIPS has 128 signals */
     kvm_set_sigmask_len(s, 16);
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1edf2b5..12328a4 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -95,7 +95,7 @@ static void kvm_kick_cpu(void *opaque)
 
 static int kvm_ppc_register_host_cpu_type(void);
 
-int kvm_arch_init(KVMState *s)
+int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ);
     cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 6f2d5b4..2c17a00 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -172,7 +172,7 @@ static void kvm_s390_enable_cmma(KVMState *s)
     trace_kvm_enable_cmma(rc);
 }
 
-int kvm_arch_init(KVMState *s)
+int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
     cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
-- 
2.1.0

[Qemu-devel] [PATCH 5/8] machine: query kvm-shadow-mem machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing kvm_shadow_mem parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c   | 6 ++++++
 include/hw/boards.h | 1 +
 target-i386/kvm.c   | 3 +--
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e04e5ab..0ad5b12 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -284,6 +284,7 @@ static void machine_initfn(Object *obj)
     MachineState *ms = MACHINE(obj);
 
     ms->kernel_irqchip_allowed = true;
+    ms->kvm_shadow_mem = -1;
 
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
@@ -414,6 +415,11 @@ bool machine_kernel_irqchip_required(MachineState *machine)
     return machine->kernel_irqchip_required;
 }
 
+int machine_kvm_shadow_mem(MachineState *machine)
+{
+    return machine->kvm_shadow_mem;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 69ab606..4be3cd1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -69,6 +69,7 @@ bool machine_usb(MachineState *machine);
 bool machine_iommu(MachineState *machine);
 bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
+int machine_kvm_shadow_mem(MachineState *machine);
 
 /**
  * MachineClass:
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ce554e4..acb6831 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -890,8 +890,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     }
     qemu_register_reset(kvm_unpoison_all, NULL);
 
-    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
-                                   "kvm_shadow_mem", -1);
+    shadow_mem = machine_kvm_shadow_mem(ms);
     if (shadow_mem != -1) {
         shadow_mem /= 4096;
         ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 5/8] machine: query kvm-shadow-mem machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing kvm_shadow_mem parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Please amend commit message:

Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option  descriptions and moved them to MachineState's QOM properties.

Query kvm-shadow-mem by accessing machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

> ---
>   hw/core/machine.c   | 6 ++++++
>   include/hw/boards.h | 1 +
>   target-i386/kvm.c   | 3 +--
>   3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e04e5ab..0ad5b12 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -284,6 +284,7 @@ static void machine_initfn(Object *obj)
>       MachineState *ms = MACHINE(obj);
>
>       ms->kernel_irqchip_allowed = true;
> +    ms->kvm_shadow_mem = -1;
>
>       object_property_add_str(obj, "accel",
>                               machine_get_accel, machine_set_accel, NULL);
> @@ -414,6 +415,11 @@ bool machine_kernel_irqchip_required(MachineState *machine)
>       return machine->kernel_irqchip_required;
>   }
>
> +int machine_kvm_shadow_mem(MachineState *machine)
> +{
> +    return machine->kvm_shadow_mem;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 69ab606..4be3cd1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -69,6 +69,7 @@ bool machine_usb(MachineState *machine);
>   bool machine_iommu(MachineState *machine);
>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>   bool machine_kernel_irqchip_required(MachineState *machine);
> +int machine_kvm_shadow_mem(MachineState *machine);
>
>   /**
>    * MachineClass:
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ce554e4..acb6831 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -890,8 +890,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       }
>       qemu_register_reset(kvm_unpoison_all, NULL);
>
> -    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
> -                                   "kvm_shadow_mem", -1);
> +    shadow_mem = machine_kvm_shadow_mem(ms);
>       if (shadow_mem != -1) {
>           shadow_mem /= 4096;
>           ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
>

[Qemu-devel] [PATCH 6/8] machine: query phandle-start machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing phandle_start parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 device_tree.c       | 5 ++---
 hw/core/machine.c   | 5 +++++
 include/hw/boards.h | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 4cb1cd5..3d119ef 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -24,7 +24,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/sysemu.h"
 #include "hw/loader.h"
-#include "qemu/option.h"
+#include "hw/boards.h"
 #include "qemu/config-file.h"
 
 #include <libfdt.h>
@@ -245,8 +245,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
      * which phandle id to start allocting phandles.
      */
     if (!phandle) {
-        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
-                                      "phandle_start", 0);
+        phandle = machine_phandle_start(current_machine);
     }
 
     if (!phandle) {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0ad5b12..5ad2409 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -420,6 +420,11 @@ int machine_kvm_shadow_mem(MachineState *machine)
     return machine->kvm_shadow_mem;
 }
 
+int machine_phandle_start(MachineState *machine)
+{
+    return machine->phandle_start;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 4be3cd1..1f21bdf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -70,6 +70,7 @@ bool machine_iommu(MachineState *machine);
 bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
 int machine_kvm_shadow_mem(MachineState *machine);
+int machine_phandle_start(MachineState *machine);
 
 /**
  * MachineClass:
-- 
2.1.0

Re: [Qemu-devel] [PATCH 6/8] machine: query phandle-start machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing phandle_start parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Please amend commit message:

Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option  descriptions and moved them to MachineState's QOM properties.

Query phandle-start by accephandle-startssing machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>


> ---
>   device_tree.c       | 5 ++---
>   hw/core/machine.c   | 5 +++++
>   include/hw/boards.h | 1 +
>   3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 4cb1cd5..3d119ef 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -24,7 +24,7 @@
>   #include "sysemu/device_tree.h"
>   #include "sysemu/sysemu.h"
>   #include "hw/loader.h"
> -#include "qemu/option.h"
> +#include "hw/boards.h"
>   #include "qemu/config-file.h"
>
>   #include <libfdt.h>
> @@ -245,8 +245,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
>        * which phandle id to start allocting phandles.
>        */
>       if (!phandle) {
> -        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
> -                                      "phandle_start", 0);
> +        phandle = machine_phandle_start(current_machine);
>       }
>
>       if (!phandle) {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 0ad5b12..5ad2409 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -420,6 +420,11 @@ int machine_kvm_shadow_mem(MachineState *machine)
>       return machine->kvm_shadow_mem;
>   }
>
> +int machine_phandle_start(MachineState *machine)
> +{
> +    return machine->phandle_start;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 4be3cd1..1f21bdf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -70,6 +70,7 @@ bool machine_iommu(MachineState *machine);
>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>   bool machine_kernel_irqchip_required(MachineState *machine);
>   int machine_kvm_shadow_mem(MachineState *machine);
> +int machine_phandle_start(MachineState *machine);
>
>   /**
>    * MachineClass:
>

Re: [Qemu-devel] [PATCH 6/8] machine: query phandle-start machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/11/2015 04:32 PM, Marcel Apfelbaum wrote:
> On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
>> Fixes a QEMU crash when passing phandle_start parameter in command line.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> Please amend commit message:
>
> Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
> removed the global option  descriptions and moved them to MachineState's QOM properties.
>
> Query phandle-start by accephandle-startssing machine properties through designated wrappers.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
Sorry for the typo:

Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option  descriptions and moved them to MachineState's QOM properties.

Query phandle-start by accessing machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>


>
>> ---
>>   device_tree.c       | 5 ++---
>>   hw/core/machine.c   | 5 +++++
>>   include/hw/boards.h | 1 +
>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index 4cb1cd5..3d119ef 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -24,7 +24,7 @@
>>   #include "sysemu/device_tree.h"
>>   #include "sysemu/sysemu.h"
>>   #include "hw/loader.h"
>> -#include "qemu/option.h"
>> +#include "hw/boards.h"
>>   #include "qemu/config-file.h"
>>
>>   #include <libfdt.h>
>> @@ -245,8 +245,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
>>        * which phandle id to start allocting phandles.
>>        */
>>       if (!phandle) {
>> -        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>> -                                      "phandle_start", 0);
>> +        phandle = machine_phandle_start(current_machine);
>>       }
>>
>>       if (!phandle) {
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 0ad5b12..5ad2409 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -420,6 +420,11 @@ int machine_kvm_shadow_mem(MachineState *machine)
>>       return machine->kvm_shadow_mem;
>>   }
>>
>> +int machine_phandle_start(MachineState *machine)
>> +{
>> +    return machine->phandle_start;
>> +}
>> +
>>   static const TypeInfo machine_info = {
>>       .name = TYPE_MACHINE,
>>       .parent = TYPE_OBJECT,
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 4be3cd1..1f21bdf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -70,6 +70,7 @@ bool machine_iommu(MachineState *machine);
>>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>>   bool machine_kernel_irqchip_required(MachineState *machine);
>>   int machine_kvm_shadow_mem(MachineState *machine);
>> +int machine_phandle_start(MachineState *machine);
>>
>>   /**
>>    * MachineClass:
>>
>

Re: [Qemu-devel] [PATCH 6/8] machine: query phandle-start machine property rather than qemu opts

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 04:32:39PM +0200, Marcel Apfelbaum wrote:
> On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> >Fixes a QEMU crash when passing phandle_start parameter in command line.
> >
> >Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> Please amend commit message:
> 
> Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
> removed the global option  descriptions and moved them to MachineState's QOM properties.
> 
> Query phandle-start by accephandle-startssing machine properties through designated wrappers.

What does this mean?

> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>


I don't think this makes it clearer.
Maybe just given an example on how to reproduce the crash?

> 
> >---
> >  device_tree.c       | 5 ++---
> >  hw/core/machine.c   | 5 +++++
> >  include/hw/boards.h | 1 +
> >  3 files changed, 8 insertions(+), 3 deletions(-)
> >
> >diff --git a/device_tree.c b/device_tree.c
> >index 4cb1cd5..3d119ef 100644
> >--- a/device_tree.c
> >+++ b/device_tree.c
> >@@ -24,7 +24,7 @@
> >  #include "sysemu/device_tree.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/loader.h"
> >-#include "qemu/option.h"
> >+#include "hw/boards.h"
> >  #include "qemu/config-file.h"
> >
> >  #include <libfdt.h>
> >@@ -245,8 +245,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
> >       * which phandle id to start allocting phandles.
> >       */
> >      if (!phandle) {
> >-        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
> >-                                      "phandle_start", 0);
> >+        phandle = machine_phandle_start(current_machine);
> >      }
> >
> >      if (!phandle) {
> >diff --git a/hw/core/machine.c b/hw/core/machine.c
> >index 0ad5b12..5ad2409 100644
> >--- a/hw/core/machine.c
> >+++ b/hw/core/machine.c
> >@@ -420,6 +420,11 @@ int machine_kvm_shadow_mem(MachineState *machine)
> >      return machine->kvm_shadow_mem;
> >  }
> >
> >+int machine_phandle_start(MachineState *machine)
> >+{
> >+    return machine->phandle_start;
> >+}
> >+
> >  static const TypeInfo machine_info = {
> >      .name = TYPE_MACHINE,
> >      .parent = TYPE_OBJECT,
> >diff --git a/include/hw/boards.h b/include/hw/boards.h
> >index 4be3cd1..1f21bdf 100644
> >--- a/include/hw/boards.h
> >+++ b/include/hw/boards.h
> >@@ -70,6 +70,7 @@ bool machine_iommu(MachineState *machine);
> >  bool machine_kernel_irqchip_allowed(MachineState *machine);
> >  bool machine_kernel_irqchip_required(MachineState *machine);
> >  int machine_kvm_shadow_mem(MachineState *machine);
> >+int machine_phandle_start(MachineState *machine);
> >
> >  /**
> >   * MachineClass:
> >

Re: [Qemu-devel] [PATCH 6/8] machine: query phandle-start machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/11/2015 04:39 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 04:32:39PM +0200, Marcel Apfelbaum wrote:
>> On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
>>> Fixes a QEMU crash when passing phandle_start parameter in command line.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> Please amend commit message:
>>
>> Commit e79d5a6 ("machine: remove qemu_machine_opts global list")
>> removed the global option  descriptions and moved them to MachineState's QOM properties.
>>
>> Query phandle-start by accephandle-startssing machine properties through designated wrappers.
>
> What does this mean?
Please look at the fixed mail (sent a few seconds later)

Thanks,
Marcel

>
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
>
> I don't think this makes it clearer.
> Maybe just given an example on how to reproduce the crash?
>
>>
>>> ---
>>>   device_tree.c       | 5 ++---
>>>   hw/core/machine.c   | 5 +++++
>>>   include/hw/boards.h | 1 +
>>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 4cb1cd5..3d119ef 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -24,7 +24,7 @@
>>>   #include "sysemu/device_tree.h"
>>>   #include "sysemu/sysemu.h"
>>>   #include "hw/loader.h"
>>> -#include "qemu/option.h"
>>> +#include "hw/boards.h"
>>>   #include "qemu/config-file.h"
>>>
>>>   #include <libfdt.h>
>>> @@ -245,8 +245,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
>>>        * which phandle id to start allocting phandles.
>>>        */
>>>       if (!phandle) {
>>> -        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>>> -                                      "phandle_start", 0);
>>> +        phandle = machine_phandle_start(current_machine);
>>>       }
>>>
>>>       if (!phandle) {
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index 0ad5b12..5ad2409 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -420,6 +420,11 @@ int machine_kvm_shadow_mem(MachineState *machine)
>>>       return machine->kvm_shadow_mem;
>>>   }
>>>
>>> +int machine_phandle_start(MachineState *machine)
>>> +{
>>> +    return machine->phandle_start;
>>> +}
>>> +
>>>   static const TypeInfo machine_info = {
>>>       .name = TYPE_MACHINE,
>>>       .parent = TYPE_OBJECT,
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 4be3cd1..1f21bdf 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -70,6 +70,7 @@ bool machine_iommu(MachineState *machine);
>>>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>>>   bool machine_kernel_irqchip_required(MachineState *machine);
>>>   int machine_kvm_shadow_mem(MachineState *machine);
>>> +int machine_phandle_start(MachineState *machine);
>>>
>>>   /**
>>>    * MachineClass:
>>>

[Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing dump_guest_core parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 exec.c              | 4 ++--
 hw/core/machine.c   | 6 ++++++
 include/hw/boards.h | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 6b79ad1..bfca528 100644
--- a/exec.c
+++ b/exec.c
@@ -26,6 +26,7 @@
 #include "cpu.h"
 #include "tcg.h"
 #include "hw/hw.h"
+#include "hw/boards.h"
 #include "hw/qdev.h"
 #include "qemu/osdep.h"
 #include "sysemu/kvm.h"
@@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
     int ret;
 
     /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
-                           "dump-guest-core", true)) {
+    if (!machine_dump_guest_core(current_machine)) {
         ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
         if (ret) {
             perror("qemu_madvise");
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 5ad2409..8033683 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -285,6 +285,7 @@ static void machine_initfn(Object *obj)
 
     ms->kernel_irqchip_allowed = true;
     ms->kvm_shadow_mem = -1;
+    ms->dump_guest_core = true;
 
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
@@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine)
     return machine->phandle_start;
 }
 
+bool machine_dump_guest_core(MachineState *machine)
+{
+    return machine->dump_guest_core;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 1f21bdf..7de308e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
 int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
+bool machine_dump_guest_core(MachineState *machine);
 
 /**
  * MachineClass:
-- 
2.1.0

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Andreas Färber]

Hi,

Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> Fixes a QEMU crash when passing dump_guest_core parameter in command line.

Explain that, please?

> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  exec.c              | 4 ++--
>  hw/core/machine.c   | 6 ++++++
>  include/hw/boards.h | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6b79ad1..bfca528 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -26,6 +26,7 @@
>  #include "cpu.h"
>  #include "tcg.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  #include "hw/qdev.h"
>  #include "qemu/osdep.h"
>  #include "sysemu/kvm.h"
> @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
>      int ret;
>  
>      /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
> -                           "dump-guest-core", true)) {
> +    if (!machine_dump_guest_core(current_machine)) {

linux-user doesn't have a current_machine, and machine.c shouldn't even
be compiled in, so I'm guessing this function is #ifdef'ed, which means
the header should be #ifdef'ed, too. hw/boards.h has no business being
in *-user.

Regards,
Andreas

>          ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
>          if (ret) {
>              perror("qemu_madvise");
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 5ad2409..8033683 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj)
>  
>      ms->kernel_irqchip_allowed = true;
>      ms->kvm_shadow_mem = -1;
> +    ms->dump_guest_core = true;
>  
>      object_property_add_str(obj, "accel",
>                              machine_get_accel, machine_set_accel, NULL);
> @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine)
>      return machine->phandle_start;
>  }
>  
> +bool machine_dump_guest_core(MachineState *machine)
> +{
> +    return machine->dump_guest_core;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1f21bdf..7de308e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine);
>  bool machine_kernel_irqchip_required(MachineState *machine);
>  int machine_kvm_shadow_mem(MachineState *machine);
>  int machine_phandle_start(MachineState *machine);
> +bool machine_dump_guest_core(MachineState *machine);
>  
>  /**
>   * MachineClass:
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Michael S. Tsirkin]

On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
> Hi,
> 
> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> > Fixes a QEMU crash when passing dump_guest_core parameter in command line.
> 
> Explain that, please?

Pls note the submission date.  It's 1 month late to ask for
basic clarifications.

I've merged the patches, I'll fix up issues such as prettifying
includes by adding patches on top.



> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > ---
> >  exec.c              | 4 ++--
> >  hw/core/machine.c   | 6 ++++++
> >  include/hw/boards.h | 1 +
> >  3 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 6b79ad1..bfca528 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -26,6 +26,7 @@
> >  #include "cpu.h"
> >  #include "tcg.h"
> >  #include "hw/hw.h"
> > +#include "hw/boards.h"
> >  #include "hw/qdev.h"
> >  #include "qemu/osdep.h"
> >  #include "sysemu/kvm.h"
> > @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
> >      int ret;
> >  
> >      /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
> > -    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
> > -                           "dump-guest-core", true)) {
> > +    if (!machine_dump_guest_core(current_machine)) {
> 
> linux-user doesn't have a current_machine, and machine.c shouldn't even
> be compiled in, so I'm guessing this function is #ifdef'ed, which means
> the header should be #ifdef'ed, too. hw/boards.h has no business being
> in *-user.
> 
> Regards,
> Andreas
> 
> >          ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
> >          if (ret) {
> >              perror("qemu_madvise");
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 5ad2409..8033683 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj)
> >  
> >      ms->kernel_irqchip_allowed = true;
> >      ms->kvm_shadow_mem = -1;
> > +    ms->dump_guest_core = true;
> >  
> >      object_property_add_str(obj, "accel",
> >                              machine_get_accel, machine_set_accel, NULL);
> > @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine)
> >      return machine->phandle_start;
> >  }
> >  
> > +bool machine_dump_guest_core(MachineState *machine)
> > +{
> > +    return machine->dump_guest_core;
> > +}
> > +
> >  static const TypeInfo machine_info = {
> >      .name = TYPE_MACHINE,
> >      .parent = TYPE_OBJECT,
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 1f21bdf..7de308e 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine);
> >  bool machine_kernel_irqchip_required(MachineState *machine);
> >  int machine_kvm_shadow_mem(MachineState *machine);
> >  int machine_phandle_start(MachineState *machine);
> > +bool machine_dump_guest_core(MachineState *machine);
> >  
> >  /**
> >   * MachineClass:
> > 
> 
> 
> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Andreas Färber]

Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>> Hi,
>>
>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>
>> Explain that, please?
> 
> Pls note the submission date.  It's 1 month late to ask for
> basic clarifications.
> 
> I've merged the patches, I'll fix up issues such as prettifying
> includes by adding patches on top.

No, since the patch is not in qemu.git (it builds!) it is not too late
to fix it, nor too late to ask why a patch that introduces a breakage
does what it does. (Moving the info from the cover letter into the
commit message would've been a good idea, Marcel.)

All QEMU patches are supposed to be bisectable. It's our job as
maintainers to build-test each. If you do that 1 month later, that's not
my fault.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>>> Hi,
>>>
>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>>
>>> Explain that, please?
>> 
>> Pls note the submission date.  It's 1 month late to ask for
>> basic clarifications.
>> 
>> I've merged the patches, I'll fix up issues such as prettifying
>> includes by adding patches on top.
>
> No, since the patch is not in qemu.git (it builds!) it is not too late
> to fix it, nor too late to ask why a patch that introduces a breakage
> does what it does.

Getting review that late is decidedly suboptimal, but no excuse to
invoke maintainer privilege to ram the patch through unchanged.

Cosmetic issues can be tidied up on top.  The ongoing review may produce
nothing but cosmetic issues, but we don't know that, yet.

Commit messages can't be tidied up on top, and they're dirt cheap to
improve right in place, so let's do that, please.

[...]

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 08:34:09AM +0100, Markus Armbruster wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
> > Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
> >> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
> >>> Hi,
> >>>
> >>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> >>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
> >>>
> >>> Explain that, please?
> >> 
> >> Pls note the submission date.  It's 1 month late to ask for
> >> basic clarifications.
> >> 
> >> I've merged the patches, I'll fix up issues such as prettifying
> >> includes by adding patches on top.
> >
> > No, since the patch is not in qemu.git (it builds!) it is not too late
> > to fix it, nor too late to ask why a patch that introduces a breakage
> > does what it does.
> 
> Getting review that late is decidedly suboptimal, but no excuse to
> invoke maintainer privilege to ram the patch through unchanged.
> 
> Cosmetic issues can be tidied up on top.  The ongoing review may produce
> nothing but cosmetic issues, but we don't know that, yet.

Cool, review is good. What I wanted to say though is that I'm not
holding up a patchset that's been around for a month just because
of cosmetics and basic questions.
So I intend to send pull request this evening - I don't think we want to
live with known crashers any longer - crashes waste tester's time.

> Commit messages can't be tidied up on top, and they're dirt cheap to
> improve right in place, so let's do that, please.
> 
> [...]

Sure. Marcel, can you pls supply the command line that
produces the crash? I'll include that.

-- 
MST

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/11/2015 10:45 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 08:34:09AM +0100, Markus Armbruster wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>>
>>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>>>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>>>>> Hi,
>>>>>
>>>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>>>>
>>>>> Explain that, please?
>>>>
>>>> Pls note the submission date.  It's 1 month late to ask for
>>>> basic clarifications.
>>>>
>>>> I've merged the patches, I'll fix up issues such as prettifying
>>>> includes by adding patches on top.
>>>
>>> No, since the patch is not in qemu.git (it builds!) it is not too late
>>> to fix it, nor too late to ask why a patch that introduces a breakage
>>> does what it does.
>>
>> Getting review that late is decidedly suboptimal, but no excuse to
>> invoke maintainer privilege to ram the patch through unchanged.
>>
>> Cosmetic issues can be tidied up on top.  The ongoing review may produce
>> nothing but cosmetic issues, but we don't know that, yet.
>
> Cool, review is good. What I wanted to say though is that I'm not
> holding up a patchset that's been around for a month just because
> of cosmetics and basic questions.
> So I intend to send pull request this evening - I don't think we want to
> live with known crashers any longer - crashes waste tester's time.
>
>> Commit messages can't be tidied up on top, and they're dirt cheap to
>> improve right in place, so let's do that, please.
>>
>> [...]
>
> Sure. Marcel, can you pls supply the command line that
> produces the crash? I'll include that.
Sure you just have to use the option:
qemu-bin ... -machine pc,dump-guest-core=on

x86_64-softmmu/qemu-system-x86_64 -machine pc,dump-guest-core=on
qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
Aborted (core dumped)

Thanks,
Marcel


>

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Michael S. Tsirkin]

On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote:
> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
> > On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
> >> Hi,
> >>
> >> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> >>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
> >>
> >> Explain that, please?
> > 
> > Pls note the submission date.  It's 1 month late to ask for
> > basic clarifications.
> > 
> > I've merged the patches, I'll fix up issues such as prettifying
> > includes by adding patches on top.
> 
> No, since the patch is not in qemu.git (it builds!) it is not too late
> to fix it, nor too late to ask why a patch that introduces a breakage
> does what it does.


I tried to say that I'm not holding this patch set up
because there are some basic questions. Paolo reviewed
it and gave an ack. If others want to re-start review 1 month
afterwards, that's fine, but I don't want to defer pull
request with this any longer. If someone can quickly spot
a serious non-cosmetic problem there, that's another
matter, and would make me defer the pull request.


> (Moving the info from the cover letter into the
> commit message would've been a good idea, Marcel.)

I can tweak commit messages, sure, since that does not require
re-testing it all.

> All QEMU patches are supposed to be bisectable. It's our job as
> maintainers to build-test each. If you do that 1 month later, that's not
> my fault.
> 
> Regards,
> Andreas

I have this patch in my tree and there's
no bisect issue, just test-built before and after this patch.
That's because I had the ifdefs in boards.h which you and
Peter objected to, but that is about cosmetics, I fixed that
with a patch on top to hopefully make you both happy.

Don't take my word for it, you can check out my tree and verify,
that would be very wellcome.

> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Andreas Färber]

Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin:
> On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote:
>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>>>> Hi,
>>>>
>>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>>>
>>>> Explain that, please?
>>>
>>> Pls note the submission date.  It's 1 month late to ask for
>>> basic clarifications.
>>>
>>> I've merged the patches, I'll fix up issues such as prettifying
>>> includes by adding patches on top.
>>
>> No, since the patch is not in qemu.git (it builds!) it is not too late
>> to fix it, nor too late to ask why a patch that introduces a breakage
>> does what it does.
> 
> 
> I tried to say that I'm not holding this patch set up
> because there are some basic questions. Paolo reviewed
> it and gave an ack. If others want to re-start review 1 month
> afterwards, that's fine, but I don't want to defer pull
> request with this any longer. If someone can quickly spot
> a serious non-cosmetic problem there, that's another
> matter, and would make me defer the pull request.
> 
> 
>> (Moving the info from the cover letter into the
>> commit message would've been a good idea, Marcel.)
> 
> I can tweak commit messages, sure, since that does not require
> re-testing it all.
> 
>> All QEMU patches are supposed to be bisectable. It's our job as
>> maintainers to build-test each. If you do that 1 month later, that's not
>> my fault.
>>
>> Regards,
>> Andreas
> 
> I have this patch in my tree and there's
> no bisect issue, just test-built before and after this patch.
> That's because I had the ifdefs in boards.h which you and
> Peter objected to, but that is about cosmetics, I fixed that
> with a patch on top to hopefully make you both happy.

All I was asking for is, please squash the patch(es) that fix(es) the
build issue. In particular if you applied the patch just yesterday when
we complained. We've been required to, so I expect the same rules to
apply to everyone.

In order to propose a better fix I tried to understand what the patch is
fixing, that's all. If an improvement of the commit message comes out of
that, good, but that was not the main purpose.

Thanks,
Andreas

P.S. I was sick most of February and my Chromebook has a broken DRM
driver, not allowing for much bedside-hacking. ;)

> 
> Don't take my word for it, you can check out my tree and verify,
> that would be very wellcome.
> 
>> -- 
>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
>> Graham Norton; HRB 21284 (AG Nürnberg)


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/11/2015 01:06 PM, Andreas Färber wrote:
> Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin:
>> On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote:
>>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>>>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>>>>> Hi,
>>>>>
>>>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>>>>
>>>>> Explain that, please?
>>>>
>>>> Pls note the submission date.  It's 1 month late to ask for
>>>> basic clarifications.
>>>>
>>>> I've merged the patches, I'll fix up issues such as prettifying
>>>> includes by adding patches on top.
>>>
>>> No, since the patch is not in qemu.git (it builds!) it is not too late
>>> to fix it, nor too late to ask why a patch that introduces a breakage
>>> does what it does.
>>
>>
>> I tried to say that I'm not holding this patch set up
>> because there are some basic questions. Paolo reviewed
>> it and gave an ack. If others want to re-start review 1 month
>> afterwards, that's fine, but I don't want to defer pull
>> request with this any longer. If someone can quickly spot
>> a serious non-cosmetic problem there, that's another
>> matter, and would make me defer the pull request.
>>
>>
>>> (Moving the info from the cover letter into the
>>> commit message would've been a good idea, Marcel.)
>>
>> I can tweak commit messages, sure, since that does not require
>> re-testing it all.
>>
>>> All QEMU patches are supposed to be bisectable. It's our job as
>>> maintainers to build-test each. If you do that 1 month later, that's not
>>> my fault.
>>>
>>> Regards,
>>> Andreas
>>
>> I have this patch in my tree and there's
>> no bisect issue, just test-built before and after this patch.
>> That's because I had the ifdefs in boards.h which you and
>> Peter objected to, but that is about cosmetics, I fixed that
>> with a patch on top to hopefully make you both happy.
>
> All I was asking for is, please squash the patch(es) that fix(es) the
> build issue. In particular if you applied the patch just yesterday when
> we complained. We've been required to, so I expect the same rules to
> apply to everyone.
>
> In order to propose a better fix I tried to understand what the patch is
> fixing, that's all. If an improvement of the commit message comes out of
> that, good, but that was not the main purpose.
>
> Thanks,
> Andreas
>
> P.S. I was sick most of February and my Chromebook has a broken DRM
> driver, not allowing for much bedside-hacking. ;)
Hi Andreas,
I hope you are feeling better now!

The main issue I see here (and believe me is not the reviews, they are always welcomed!)
is that more than a month ago several developers complained about these crashes.
I stopped what I was doing and posted a series ASAP that was almost immediately
reviewed by Paolo.

I pinged twice already and nobody did anything about it.
Michael took it because nobody else did and now we have a situation:
"No good deed goes unpunished"

Now, we need a way to not let this happen.
I am afraid that next time I will not get lucky and nobody will take the patches :(.

Thanks,
Marcel



>
>>
>> Don't take my word for it, you can check out my tree and verify,
>> that would be very wellcome.
>>
>>> --
>>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
>>> Graham Norton; HRB 21284 (AG Nürnberg)
>
>

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 12:06:48PM +0100, Andreas Färber wrote:
> Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin:
> > On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote:
> >> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
> >>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
> >>>> Hi,
> >>>>
> >>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> >>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
> >>>>
> >>>> Explain that, please?
> >>>
> >>> Pls note the submission date.  It's 1 month late to ask for
> >>> basic clarifications.
> >>>
> >>> I've merged the patches, I'll fix up issues such as prettifying
> >>> includes by adding patches on top.
> >>
> >> No, since the patch is not in qemu.git (it builds!) it is not too late
> >> to fix it, nor too late to ask why a patch that introduces a breakage
> >> does what it does.
> > 
> > 
> > I tried to say that I'm not holding this patch set up
> > because there are some basic questions. Paolo reviewed
> > it and gave an ack. If others want to re-start review 1 month
> > afterwards, that's fine, but I don't want to defer pull
> > request with this any longer. If someone can quickly spot
> > a serious non-cosmetic problem there, that's another
> > matter, and would make me defer the pull request.
> > 
> > 
> >> (Moving the info from the cover letter into the
> >> commit message would've been a good idea, Marcel.)
> > 
> > I can tweak commit messages, sure, since that does not require
> > re-testing it all.
> > 
> >> All QEMU patches are supposed to be bisectable. It's our job as
> >> maintainers to build-test each. If you do that 1 month later, that's not
> >> my fault.
> >>
> >> Regards,
> >> Andreas
> > 
> > I have this patch in my tree and there's
> > no bisect issue, just test-built before and after this patch.
> > That's because I had the ifdefs in boards.h which you and
> > Peter objected to, but that is about cosmetics, I fixed that
> > with a patch on top to hopefully make you both happy.
> 
> All I was asking for is, please squash the patch(es) that fix(es) the
> build issue.

Hmm as far as I can see, there's no build issue.
My patch is required before Marcel's one.
Then there's another one by me to address the comments
you had.

I could squash them all but this just messes up attribution,
bisect build is fine, and I verified that.

> In particular if you applied the patch just yesterday when
> we complained. We've been required to, so I expect the same rules to
> apply to everyone.
> 
> In order to propose a better fix I tried to understand what the patch is
> fixing, that's all. If an improvement of the commit message comes out of
> that, good, but that was not the main purpose.
> 
> Thanks,
> Andreas
> 
> P.S. I was sick most of February and my Chromebook has a broken DRM
> driver, not allowing for much bedside-hacking. ;)

I certainly didn't intend to blame anyone, sorry if it
sounded like that. I was merely saying, it's been on review
for a while, got ack and not objections, so let's apply it unless
someone sees significant issues, I don't want to hold it up
until all questions are answered.

> > 
> > Don't take my word for it, you can check out my tree and verify,
> > that would be very wellcome.
> > 
> >> -- 
> >> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> >> Graham Norton; HRB 21284 (AG Nürnberg)
> 
> 
> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote:
>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>> > On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>> >> Hi,
>> >>
>> >> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>> >>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>> >>
>> >> Explain that, please?
>> > 
>> > Pls note the submission date.  It's 1 month late to ask for
>> > basic clarifications.
>> > 
>> > I've merged the patches, I'll fix up issues such as prettifying
>> > includes by adding patches on top.
>> 
>> No, since the patch is not in qemu.git (it builds!) it is not too late
>> to fix it, nor too late to ask why a patch that introduces a breakage
>> does what it does.
>
>
> I tried to say that I'm not holding this patch set up
> because there are some basic questions. Paolo reviewed
> it and gave an ack. If others want to re-start review 1 month
> afterwards, that's fine, but I don't want to defer pull
> request with this any longer. If someone can quickly spot
> a serious non-cosmetic problem there, that's another
> matter, and would make me defer the pull request.

Okay, sounds much more reasonable than your first reply.  Thanks for
clarifying.

[...]

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/10/2015 11:36 PM, Andreas Färber wrote:
> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin:
>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote:
>>> Hi,
>>>
>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>>>
>>> Explain that, please?
>>
>> Pls note the submission date.  It's 1 month late to ask for
>> basic clarifications.
>>
>> I've merged the patches, I'll fix up issues such as prettifying
>> includes by adding patches on top.
>
> No, since the patch is not in qemu.git (it builds!) it is not too late
> to fix it, nor too late to ask why a patch that introduces a breakage
> does what it does. (Moving the info from the cover letter into the
> commit message would've been a good idea, Marcel.)
Hi Andreas,
Agreed.


Thanks,
Marcel
>
> All QEMU patches are supposed to be bisectable. It's our job as
> maintainers to build-test each. If you do that 1 month later, that's not
> my fault.
>
> Regards,
> Andreas
>

Re: [Qemu-devel] [PATCH 7/8] machine: query dump-guest-core machine property rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing dump_guest_core parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Please amend commit message:

Running
     qemu-bin ... -machine pc,dump-guest-core=on
leads to crash:
     x86_64-softmmu/qemu-system-x86_64 -machine pc,dump-guest-core=on
     qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
     Aborted (core dumped)

This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option descriptions and moved them to MachineState's QOM properties.

Fix this by querying machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>



> ---
>   exec.c              | 4 ++--
>   hw/core/machine.c   | 6 ++++++
>   include/hw/boards.h | 1 +
>   3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 6b79ad1..bfca528 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -26,6 +26,7 @@
>   #include "cpu.h"
>   #include "tcg.h"
>   #include "hw/hw.h"
> +#include "hw/boards.h"
>   #include "hw/qdev.h"
>   #include "qemu/osdep.h"
>   #include "sysemu/kvm.h"
> @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
>       int ret;
>
>       /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
> -                           "dump-guest-core", true)) {
> +    if (!machine_dump_guest_core(current_machine)) {
>           ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
>           if (ret) {
>               perror("qemu_madvise");
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 5ad2409..8033683 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj)
>
>       ms->kernel_irqchip_allowed = true;
>       ms->kvm_shadow_mem = -1;
> +    ms->dump_guest_core = true;
>
>       object_property_add_str(obj, "accel",
>                               machine_get_accel, machine_set_accel, NULL);
> @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine)
>       return machine->phandle_start;
>   }
>
> +bool machine_dump_guest_core(MachineState *machine)
> +{
> +    return machine->dump_guest_core;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1f21bdf..7de308e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine);
>   bool machine_kernel_irqchip_required(MachineState *machine);
>   int machine_kvm_shadow_mem(MachineState *machine);
>   int machine_phandle_start(MachineState *machine);
> +bool machine_dump_guest_core(MachineState *machine);
>
>   /**
>    * MachineClass:
>

[Qemu-devel] [PATCH 8/8] machine: query mem-merge machine property rather than qemu opts

[Marcel Apfelbaum]

Fixes a QEMU crash when passing mem_merge parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 exec.c              | 2 +-
 hw/core/machine.c   | 6 ++++++
 include/hw/boards.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index bfca528..becd122 100644
--- a/exec.c
+++ b/exec.c
@@ -1277,7 +1277,7 @@ void qemu_ram_unset_idstr(ram_addr_t addr)
 
 static int memory_try_enable_merging(void *addr, size_t len)
 {
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
+    if (!machine_mem_merge(current_machine)) {
         /* disabled by the user */
         return 0;
     }
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8033683..e3a3e2a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -286,6 +286,7 @@ static void machine_initfn(Object *obj)
     ms->kernel_irqchip_allowed = true;
     ms->kvm_shadow_mem = -1;
     ms->dump_guest_core = true;
+    ms->mem_merge = true;
 
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
@@ -431,6 +432,11 @@ bool machine_dump_guest_core(MachineState *machine)
     return machine->dump_guest_core;
 }
 
+bool machine_mem_merge(MachineState *machine)
+{
+    return machine->mem_merge;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7de308e..f44d6f5 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -72,6 +72,7 @@ bool machine_kernel_irqchip_required(MachineState *machine);
 int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
+bool machine_mem_merge(MachineState *machine);
 
 /**
  * MachineClass:
-- 
2.1.0

Re: [Qemu-devel] [PATCH 8/8] machine: query mem-merge machine property rather than qemu opts

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 05:43:55PM +0200, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing mem_merge parameter in command line.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Can you rebase this one on top of my pci branch pls?

> ---
>  exec.c              | 2 +-
>  hw/core/machine.c   | 6 ++++++
>  include/hw/boards.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index bfca528..becd122 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1277,7 +1277,7 @@ void qemu_ram_unset_idstr(ram_addr_t addr)
>  
>  static int memory_try_enable_merging(void *addr, size_t len)
>  {
> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
> +    if (!machine_mem_merge(current_machine)) {
>          /* disabled by the user */
>          return 0;
>      }
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8033683..e3a3e2a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -286,6 +286,7 @@ static void machine_initfn(Object *obj)
>      ms->kernel_irqchip_allowed = true;
>      ms->kvm_shadow_mem = -1;
>      ms->dump_guest_core = true;
> +    ms->mem_merge = true;
>  
>      object_property_add_str(obj, "accel",
>                              machine_get_accel, machine_set_accel, NULL);
> @@ -431,6 +432,11 @@ bool machine_dump_guest_core(MachineState *machine)
>      return machine->dump_guest_core;
>  }
>  
> +bool machine_mem_merge(MachineState *machine)
> +{
> +    return machine->mem_merge;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7de308e..f44d6f5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -72,6 +72,7 @@ bool machine_kernel_irqchip_required(MachineState *machine);
>  int machine_kvm_shadow_mem(MachineState *machine);
>  int machine_phandle_start(MachineState *machine);
>  bool machine_dump_guest_core(MachineState *machine);
> +bool machine_mem_merge(MachineState *machine);
>  
>  /**
>   * MachineClass:
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH 8/8] machine: query mem-merge machine property rather than qemu opts

[Marcel Apfelbaum]

On 03/10/2015 05:11 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 04, 2015 at 05:43:55PM +0200, Marcel Apfelbaum wrote:
>> Fixes a QEMU crash when passing mem_merge parameter in command line.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> Can you rebase this one on top of my pci branch pls?
I'll submit again only 8/8 after rebase.

Thanks,
Marcel
>
>> ---
>>   exec.c              | 2 +-
>>   hw/core/machine.c   | 6 ++++++
>>   include/hw/boards.h | 1 +
>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index bfca528..becd122 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1277,7 +1277,7 @@ void qemu_ram_unset_idstr(ram_addr_t addr)
>>
>>   static int memory_try_enable_merging(void *addr, size_t len)
>>   {
>> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
>> +    if (!machine_mem_merge(current_machine)) {
>>           /* disabled by the user */
>>           return 0;
>>       }
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 8033683..e3a3e2a 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -286,6 +286,7 @@ static void machine_initfn(Object *obj)
>>       ms->kernel_irqchip_allowed = true;
>>       ms->kvm_shadow_mem = -1;
>>       ms->dump_guest_core = true;
>> +    ms->mem_merge = true;
>>
>>       object_property_add_str(obj, "accel",
>>                               machine_get_accel, machine_set_accel, NULL);
>> @@ -431,6 +432,11 @@ bool machine_dump_guest_core(MachineState *machine)
>>       return machine->dump_guest_core;
>>   }
>>
>> +bool machine_mem_merge(MachineState *machine)
>> +{
>> +    return machine->mem_merge;
>> +}
>> +
>>   static const TypeInfo machine_info = {
>>       .name = TYPE_MACHINE,
>>       .parent = TYPE_OBJECT,
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 7de308e..f44d6f5 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -72,6 +72,7 @@ bool machine_kernel_irqchip_required(MachineState *machine);
>>   int machine_kvm_shadow_mem(MachineState *machine);
>>   int machine_phandle_start(MachineState *machine);
>>   bool machine_dump_guest_core(MachineState *machine);
>> +bool machine_mem_merge(MachineState *machine);
>>
>>   /**
>>    * MachineClass:
>> --
>> 2.1.0

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Paolo Bonzini]

On 04/02/2015 16:43, Marcel Apfelbaum wrote:
> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
> 
> This results in a Qemu crash if a non string option is queried using qemu opts.
> Fix this by querying machine properties through designated wrappers.

Wrappers vs. direct access to fields is just a matter of taste, so

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Christian Borntraeger]

Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
> 
> This results in a Qemu crash if a non string option is queried using qemu opts.
> Fix this by querying machine properties through designated wrappers.
> 
> I hope I didn't miss anything.
> Comments are appreciated as always.
> 
> Thanks,
> Marcel
> 
> Marcel Apfelbaum (8):
>   machine: query iommu machine property rather than qemu opts
>   hw/machine: kernel-irqchip property support for allowed/required
>   machine: query kernel-irqchip machine property rather than qemu opts
>   kvm: add machine state to kvm_arch_init
>   machine: query kvm-shadow-mem machine property rather than qemu opts
>   machine: query phandle-start machine property rather than qemu opts
>   machine: query dump-guest-core machine property rather than qemu opts
>   machine: query mem-merge machine property rather than qemu opts

In general this seems to work.
I have a question, though:

What I like is a way to do some wrappers in the specific machines.
For example, we plan to add

static inline void s390_machine_initfn(Object *obj)
{
    object_property_add_bool(obj, "aes-key-wrap",
                             machine_get_aes_key_wrap,
                             machine_set_aes_key_wrap, NULL);
    object_property_set_description(obj, "aes-key-wrap",
            "enable/disable AES key wrapping using the CPACF wrapping key",
            NULL);
    object_property_add_bool(obj, "dea-key-wrap",
                             machine_get_dea_key_wrap,
                             machine_set_dea_key_wrap, NULL);
    object_property_set_description(obj, "dea-key-wrap",
            "enable/disable DEA key wrapping using the CPACF wrapping key",
            NULL);
}



Previously we used   
 if (qemu_opt_get_bool(opts, "aes-key-wrap", false)) 
if target-s390/kvm.c
to query that.

Now, these options are pretty specific to s390 and adding them to hw/core/machine.c
to create wrappers seems strange. So implementing them in hw/s390/s390-virtio-ccw.c
seems a much better place.
Would a function there that does  gets S390_CCW_MACHINE(current_machine)->aes_key_wrap 
considered ok, or do we need to pollute hw/core/machine.c with architecture specific
options?

Christian

PS: The same is somewhat true for qemu-options.hx. Having such specific machine option
in the global help offers room for improvement

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 09:45 PM, Christian Borntraeger wrote:
> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
>> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>>
>> This results in a Qemu crash if a non string option is queried using qemu opts.
>> Fix this by querying machine properties through designated wrappers.
>>
>> I hope I didn't miss anything.
>> Comments are appreciated as always.
>>
>> Thanks,
>> Marcel
>>
>> Marcel Apfelbaum (8):
>>    machine: query iommu machine property rather than qemu opts
>>    hw/machine: kernel-irqchip property support for allowed/required
>>    machine: query kernel-irqchip machine property rather than qemu opts
>>    kvm: add machine state to kvm_arch_init
>>    machine: query kvm-shadow-mem machine property rather than qemu opts
>>    machine: query phandle-start machine property rather than qemu opts
>>    machine: query dump-guest-core machine property rather than qemu opts
>>    machine: query mem-merge machine property rather than qemu opts
>
> In general this seems to work.
Thanks!

> I have a question, though:
>
> What I like is a way to do some wrappers in the specific machines.
> For example, we plan to add
>
> static inline void s390_machine_initfn(Object *obj)
> {
>      object_property_add_bool(obj, "aes-key-wrap",
>                               machine_get_aes_key_wrap,
>                               machine_set_aes_key_wrap, NULL);
>      object_property_set_description(obj, "aes-key-wrap",
>              "enable/disable AES key wrapping using the CPACF wrapping key",
>              NULL);
>      object_property_add_bool(obj, "dea-key-wrap",
>                               machine_get_dea_key_wrap,
>                               machine_set_dea_key_wrap, NULL);
>      object_property_set_description(obj, "dea-key-wrap",
>              "enable/disable DEA key wrapping using the CPACF wrapping key",
>              NULL);
> }
OK, You add 2 QOM properties to TYPE_S390_CCW_MACHINE instances.
Of course your setters/getters need to save the property values into an actual field,
so you will need a S390State (derived from MachineState)

typedef struct {
   MachineState parent;

   bool   aes_key_wrap;
   ...

} S390State

>
>
>
> Previously we used
>   if (qemu_opt_get_bool(opts, "aes-key-wrap", false))
> if target-s390/kvm.c
> to query that.
>
> Now, these options are pretty specific to s390 and adding them to hw/core/machine.c
> to create wrappers seems strange.
Completely agree. This is the reason we wanted options(properties) per machine type.

  So implementing them in hw/s390/s390-virtio-ccw.c
> seems a much better place.
Indeed.

> Would a function there that does  gets S390_CCW_MACHINE(current_machine)->aes_key_wrap
> considered ok, or do we need to pollute hw/core/machine.c with architecture specific
> options?
We surely don't add this to hw/core/machine.c because is specific to TYPE_S390_CCW_MACHINE.

Let's say you want to use this property in kvm_arch_init of target-s390x/kvm.c.
  - After this series you already have an instance of MachineState initialized with your new properties.
  - My assumption is that TYPE_S390_CCW_MACHINE is the only s390 machine or the base type of all s390 machines.
You have three options here:
  1. Use QOM infrastucture:
     bool aes_key_wrap = object_property_get_bool(OBJECT(machine), "aes-key-wrap", NULL);
  2. Add a wrapper somewhere in  include/hw/s390x/
     that gets MachineState, cast it into S390State and return the field value.
  3. Directly downcast MachineState to S390State and get the value.
* All of the above use my assumption that all s390 machines derive from this one.

>
> Christian
>
> PS: The same is somewhat true for qemu-options.hx. Having such specific machine option
> in the global help offers room for improvement
Can you please elaborate? I didn't fully understand what exactly are you referring to.

Hope I helped,
Marcel
>
>

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Christian Borntraeger]

Am 04.02.2015 um 22:35 schrieb Marcel Apfelbaum:
[...]
>> Would a function there that does  gets S390_CCW_MACHINE(current_machine)->aes_key_wrap
>> considered ok, or do we need to pollute hw/core/machine.c with architecture specific
>> options?
> We surely don't add this to hw/core/machine.c because is specific to TYPE_S390_CCW_MACHINE.
> 
> Let's say you want to use this property in kvm_arch_init of target-s390x/kvm.c.
>  - After this series you already have an instance of MachineState initialized with your new properties.
>  - My assumption is that TYPE_S390_CCW_MACHINE is the only s390 machine or the base type of all s390 machines.
> You have three options here:
>  1. Use QOM infrastucture:
>     bool aes_key_wrap = object_property_get_bool(OBJECT(machine), "aes-key-wrap", NULL);
>  2. Add a wrapper somewhere in  include/hw/s390x/
>     that gets MachineState, cast it into S390State and return the field value.
>  3. Directly downcast MachineState to S390State and get the value.
> * All of the above use my assumption that all s390 machines derive from this one.

Yes, we derive CCW_MACHINE from the base machine type. Thanks

> 
>>
>> Christian
>>
>> PS: The same is somewhat true for qemu-options.hx. Having such specific machine option
>> in the global help offers room for improvement
> Can you please elaborate? I didn't fully understand what exactly are you referring to.

lets take for example vmport. This is only relevant for x86, but it seems that there
is nothing like QEMU_ARCH_ALL  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32
for machine properties. Now that we actually move this into the backends, we could do it
here as well. 
 
> Hope I helped,
> Marcel

Absolutely

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Marcel Apfelbaum]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>
> This results in a Qemu crash if a non string option is queried using qemu opts.
> Fix this by querying machine properties through designated wrappers.
>
> I hope I didn't miss anything.
> Comments are appreciated as always.
Ping

Hi, who can take this series?
It solves some command line bugs and people were pretty annoyed about it.

Thanks,
Marcel

>
> Thanks,
> Marcel
>
> Marcel Apfelbaum (8):
>    machine: query iommu machine property rather than qemu opts
>    hw/machine: kernel-irqchip property support for allowed/required
>    machine: query kernel-irqchip machine property rather than qemu opts
>    kvm: add machine state to kvm_arch_init
>    machine: query kvm-shadow-mem machine property rather than qemu opts
>    machine: query phandle-start machine property rather than qemu opts
>    machine: query dump-guest-core machine property rather than qemu opts
>    machine: query mem-merge machine property rather than qemu opts
>
>   device_tree.c        |  5 ++---
>   exec.c               |  6 +++---
>   hw/core/machine.c    | 52 +++++++++++++++++++++++++++++++++++++++++++---------
>   hw/pci-host/q35.c    |  2 +-
>   hw/ppc/e500.c        | 16 +++++-----------
>   hw/ppc/spapr.c       | 16 ++++++----------
>   include/hw/boards.h  | 10 +++++++++-
>   include/sysemu/kvm.h |  2 +-
>   kvm-all.c            |  8 ++++----
>   target-arm/kvm.c     |  2 +-
>   target-i386/kvm.c    |  5 ++---
>   target-mips/kvm.c    |  2 +-
>   target-ppc/kvm.c     |  2 +-
>   target-s390x/kvm.c   |  2 +-
>   14 files changed, 80 insertions(+), 50 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Marcel Apfelbaum]

On 02/25/2015 01:55 PM, Marcel Apfelbaum wrote:
> On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
>> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
>> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>>
>> This results in a Qemu crash if a non string option is queried using qemu opts.
>> Fix this by querying machine properties through designated wrappers.
>>
>> I hope I didn't miss anything.
>> Comments are appreciated as always.
> Ping
Ping ping :)

Thanks,
Marcel
>
> Hi, who can take this series?
> It solves some command line bugs and people were pretty annoyed about it.
>
> Thanks,
> Marcel
>
>>
>> Thanks,
>> Marcel
>>
>> Marcel Apfelbaum (8):
>>    machine: query iommu machine property rather than qemu opts
>>    hw/machine: kernel-irqchip property support for allowed/required
>>    machine: query kernel-irqchip machine property rather than qemu opts
>>    kvm: add machine state to kvm_arch_init
>>    machine: query kvm-shadow-mem machine property rather than qemu opts
>>    machine: query phandle-start machine property rather than qemu opts
>>    machine: query dump-guest-core machine property rather than qemu opts
>>    machine: query mem-merge machine property rather than qemu opts
>>
>>   device_tree.c        |  5 ++---
>>   exec.c               |  6 +++---
>>   hw/core/machine.c    | 52 +++++++++++++++++++++++++++++++++++++++++++---------
>>   hw/pci-host/q35.c    |  2 +-
>>   hw/ppc/e500.c        | 16 +++++-----------
>>   hw/ppc/spapr.c       | 16 ++++++----------
>>   include/hw/boards.h  | 10 +++++++++-
>>   include/sysemu/kvm.h |  2 +-
>>   kvm-all.c            |  8 ++++----
>>   target-arm/kvm.c     |  2 +-
>>   target-i386/kvm.c    |  5 ++---
>>   target-mips/kvm.c    |  2 +-
>>   target-ppc/kvm.c     |  2 +-
>>   target-s390x/kvm.c   |  2 +-
>>   14 files changed, 80 insertions(+), 50 deletions(-)
>>
>