Finish cleaning up qemu_init

[PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Paolo Bonzini 5 years, 2 months ago

Machine options can be retrieved as properties of the machine object.
Encourage that by removing the "easy" accessor to machine options.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c     | 11 ++++-------
 hw/arm/boot.c           |  2 +-
 hw/microblaze/boot.c    |  9 ++++-----
 hw/nios2/boot.c         |  9 ++++-----
 hw/ppc/e500.c           |  5 ++---
 hw/ppc/spapr_nvdimm.c   |  4 ++--
 hw/ppc/virtex_ml507.c   |  2 +-
 hw/riscv/sifive_u.c     |  6 ++----
 hw/riscv/virt.c         |  6 ++----
 hw/xtensa/xtfpga.c      |  9 ++++-----
 include/sysemu/sysemu.h |  2 --
 softmmu/device_tree.c   |  2 +-
 softmmu/vl.c            |  2 +-
 13 files changed, 28 insertions(+), 41 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index baaa54249d..666b9ab96c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2013,7 +2013,6 @@ static int kvm_init(MachineState *ms)
     const KVMCapabilityInfo *missing_cap;
     int ret;
     int type = 0;
-    const char *kvm_type;
     uint64_t dirty_log_manual_caps;
 
     s = KVM_STATE(ms->accelerator);
@@ -2069,13 +2068,11 @@ static int kvm_init(MachineState *ms)
     }
     s->as = g_new0(struct KVMAs, s->nr_as);
 
-    kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
-    if (mc->kvm_type) {
+    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+        g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine),
+                                                            "kvm-type",
+                                                            &error_abort);
         type = mc->kvm_type(ms, kvm_type);
-    } else if (kvm_type) {
-        ret = -EINVAL;
-        fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
-        goto err;
     }
 
     do {
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 4d9d47ba1c..e56c42ac22 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1299,7 +1299,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
     info->kernel_filename = ms->kernel_filename;
     info->kernel_cmdline = ms->kernel_cmdline;
     info->initrd_filename = ms->initrd_filename;
-    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+    info->dtb_filename = ms->dtb;
     info->dtb_limit = 0;
 
     /* Load the kernel.  */
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 6715ba2ff9..caaba1aa4c 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -34,6 +34,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
+#include "hw/boards.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "qemu/cutils.h"
@@ -116,16 +117,14 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
                             const char *dtb_filename,
                             void (*machine_cpu_reset)(MicroBlazeCPU *))
 {
-    QemuOpts *machine_opts;
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *dtb_arg;
     char *filename = NULL;
 
-    machine_opts = qemu_get_machine_opts();
-    kernel_filename = qemu_opt_get(machine_opts, "kernel");
-    kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    dtb_arg = qemu_opt_get(machine_opts, "dtb");
+    kernel_filename = current_machine->kernel_filename;
+    kernel_cmdline = current_machine->kernel_cmdline;
+    dtb_arg = current_machine->dtb;
     /* default to pcbios dtb as passed by machine_init */
     if (!dtb_arg && dtb_filename) {
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 95a8697906..d9969ac148 100644
--- a/hw/nios2/boot.c
+++ b/hw/nios2/boot.c
@@ -39,6 +39,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
+#include "hw/boards.h"
 #include "hw/loader.h"
 #include "elf.h"
 
@@ -120,16 +121,14 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr ddr_base,
                             const char *dtb_filename,
                             void (*machine_cpu_reset)(Nios2CPU *))
 {
-    QemuOpts *machine_opts;
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *dtb_arg;
     char *filename = NULL;
 
-    machine_opts = qemu_get_machine_opts();
-    kernel_filename = qemu_opt_get(machine_opts, "kernel");
-    kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    dtb_arg = qemu_opt_get(machine_opts, "dtb");
+    kernel_filename = current_machine->kernel_filename;
+    kernel_cmdline = current_machine->kernel_cmdline;
+    dtb_arg = current_machine->dtb;
     /* default to pcbios dtb as passed by machine_init */
     if (!dtb_arg) {
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 6a64eb31ab..41dad2e583 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -343,9 +343,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
             pmc->pci_pio_base >> 32, pmc->pci_pio_base,
             0x0, 0x10000,
         };
-    QemuOpts *machine_opts = qemu_get_machine_opts();
-    const char *dtb_file = qemu_opt_get(machine_opts, "dtb");
-    const char *toplevel_compat = qemu_opt_get(machine_opts, "dt_compatible");
+    const char *dtb_file = machine->dtb;
+    const char *toplevel_compat = machine->dt_compatible;
 
     if (dtb_file) {
         char *filename;
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..84715a4d78 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 {
     const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     const MachineState *ms = MACHINE(hotplug_dev);
-    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
     g_autofree char *uuidstr = NULL;
     QemuUUID uuid;
     int ret;
@@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
      * ensure that, if the user sets nvdimm=off, we error out
      * regardless of being 5.1 or newer.
      */
-    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
         error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
         return false;
     }
+    ms->nvdimms_state->is_enabled = true;
 
     if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
                                 &error_abort) == 0) {
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 7f1bca928c..07fe49da0d 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -152,7 +152,7 @@ static int xilinx_load_device_tree(hwaddr addr,
     int r;
     const char *dtb_filename;
 
-    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+    dtb_filename = current_machine->dtb;
     if (dtb_filename) {
         fdt = load_device_tree(dtb_filename, &fdt_size);
         if (!fdt) {
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 2f19a9cda2..e7f6dc5fb3 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -100,14 +100,12 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
     int cpu;
     uint32_t *cells;
     char *nodename;
-    const char *dtb_filename;
     char ethclk_names[] = "pclk\0hclk";
     uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1;
     uint32_t hfclk_phandle, rtcclk_phandle, phy_phandle;
 
-    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
-    if (dtb_filename) {
-        fdt = s->fdt = load_device_tree(dtb_filename, &s->fdt_size);
+    if (ms->dtb) {
+        fdt = s->fdt = load_device_tree(ms->dtb, &s->fdt_size);
         if (!fdt) {
             error_report("load_device_tree() failed");
             exit(1);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 25cea7aa67..3cc18a76e7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -181,7 +181,6 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
 {
     void *fdt;
     int i, cpu, socket;
-    const char *dtb_filename;
     MachineState *mc = MACHINE(s);
     uint64_t addr, size;
     uint32_t *clint_cells, *plic_cells;
@@ -195,9 +194,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
     hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
 
-    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
-    if (dtb_filename) {
-        fdt = s->fdt = load_device_tree(dtb_filename, &s->fdt_size);
+    if (mc->dtb) {
+        fdt = s->fdt = load_device_tree(mc->dtb, &s->fdt_size);
         if (!fdt) {
             error_report("load_device_tree() failed");
             exit(1);
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index b1470b88e6..7be53f1895 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -233,11 +233,10 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
     qemu_irq *extints;
     DriveInfo *dinfo;
     PFlashCFI01 *flash = NULL;
-    QemuOpts *machine_opts = qemu_get_machine_opts();
-    const char *kernel_filename = qemu_opt_get(machine_opts, "kernel");
-    const char *kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    const char *dtb_filename = qemu_opt_get(machine_opts, "dtb");
-    const char *initrd_filename = qemu_opt_get(machine_opts, "initrd");
+    const char *kernel_filename = machine->kernel_filename;
+    const char *kernel_cmdline = machine->kernel_cmdline;
+    const char *dtb_filename = machine->dtb;
+    const char *initrd_filename = machine->initrd_filename;
     const unsigned system_io_size = 224 * MiB;
     uint32_t freq = 10000000;
     int n;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9b47cdca55..e8f463ff30 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -104,8 +104,6 @@ typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
 void qemu_boot_set(const char *boot_order, Error **errp);
 
-QemuOpts *qemu_get_machine_opts(void);
-
 bool defaults_enabled(void);
 
 void qemu_init(int argc, char **argv, char **envp);
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index b335dae707..b9a3ddc518 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -526,7 +526,7 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
 
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
-    const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
+    const char *dumpdtb = current_machine->dumpdtb;
 
     if (dumpdtb) {
         /* Dump the dtb to a file and quit */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4fece1b9db..0f7222af31 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -481,7 +481,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
  *
  * Returns: machine options (never null).
  */
-QemuOpts *qemu_get_machine_opts(void)
+static QemuOpts *qemu_get_machine_opts(void)
 {
     return qemu_find_opts_singleton("machine");
 }
-- 
2.26.2

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Igor Mammedov 5 years, 2 months ago

On Wed,  2 Dec 2020 03:18:49 -0500
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Machine options can be retrieved as properties of the machine object.
> Encourage that by removing the "easy" accessor to machine options.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/kvm/kvm-all.c     | 11 ++++-------
>  hw/arm/boot.c           |  2 +-
>  hw/microblaze/boot.c    |  9 ++++-----
>  hw/nios2/boot.c         |  9 ++++-----
>  hw/ppc/e500.c           |  5 ++---
>  hw/ppc/spapr_nvdimm.c   |  4 ++--
>  hw/ppc/virtex_ml507.c   |  2 +-
>  hw/riscv/sifive_u.c     |  6 ++----
>  hw/riscv/virt.c         |  6 ++----
>  hw/xtensa/xtfpga.c      |  9 ++++-----
>  include/sysemu/sysemu.h |  2 --
>  softmmu/device_tree.c   |  2 +-
>  softmmu/vl.c            |  2 +-
>  13 files changed, 28 insertions(+), 41 deletions(-)
> 
[...]
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..84715a4d78 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>  {
>      const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      const MachineState *ms = MACHINE(hotplug_dev);
> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
>      g_autofree char *uuidstr = NULL;
>      QemuUUID uuid;
>      int ret;
> @@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>       * ensure that, if the user sets nvdimm=off, we error out
>       * regardless of being 5.1 or newer.
>       */
> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> +    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
>          error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>          return false;
>      }
> +    ms->nvdimms_state->is_enabled = true;

it looks like nvdimm is always enabled since 5.0 and there is no way to disable it
how about dropping 9/15 and just error out is it's not enabled, something like:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..d63f095bff 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2668,6 +2668,7 @@ static void spapr_machine_init(MachineState *machine)
     char *filename;
     Error *resize_hpt_err = NULL;
+    if (mc->nvdimm_supported) { // by default it is always enabled
+        ms->nvdimms_state->is_enabled = true;
+    }
     msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..b11c85dbaa 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -57,7 +57,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
      * ensure that, if the user sets nvdimm=off, we error out
      * regardless of being 5.1 or newer.
      */
-    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+    if (!ms->nvdimms_state->is_enabled) {
         error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
         return false;
     }

if I didn't miss something, that way we do not abuse nvdimm_opt and still
have nvdimm enabled by default and error out if it turns to disabled for whatever reason.


>      if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
>                                  &error_abort) == 0) {
[...]

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Paolo Bonzini 5 years, 2 months ago

On 07/12/20 17:07, Igor Mammedov wrote:
> On Wed,  2 Dec 2020 03:18:49 -0500
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Machine options can be retrieved as properties of the machine object.
>> Encourage that by removing the "easy" accessor to machine options.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c     | 11 ++++-------
>>   hw/arm/boot.c           |  2 +-
>>   hw/microblaze/boot.c    |  9 ++++-----
>>   hw/nios2/boot.c         |  9 ++++-----
>>   hw/ppc/e500.c           |  5 ++---
>>   hw/ppc/spapr_nvdimm.c   |  4 ++--
>>   hw/ppc/virtex_ml507.c   |  2 +-
>>   hw/riscv/sifive_u.c     |  6 ++----
>>   hw/riscv/virt.c         |  6 ++----
>>   hw/xtensa/xtfpga.c      |  9 ++++-----
>>   include/sysemu/sysemu.h |  2 --
>>   softmmu/device_tree.c   |  2 +-
>>   softmmu/vl.c            |  2 +-
>>   13 files changed, 28 insertions(+), 41 deletions(-)
>>
> [...]
>> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
>> index a833a63b5e..84715a4d78 100644
>> --- a/hw/ppc/spapr_nvdimm.c
>> +++ b/hw/ppc/spapr_nvdimm.c
>> @@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>>   {
>>       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>>       const MachineState *ms = MACHINE(hotplug_dev);
>> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
>>       g_autofree char *uuidstr = NULL;
>>       QemuUUID uuid;
>>       int ret;
>> @@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>>        * ensure that, if the user sets nvdimm=off, we error out
>>        * regardless of being 5.1 or newer.
>>        */
>> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
>> +    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
>>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>>           return false;
>>       }
>> +    ms->nvdimms_state->is_enabled = true;
> 
> it looks like nvdimm is always enabled since 5.0 and there is no way to disable it
> how about dropping 9/15 and just error out is it's not enabled, something like:
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..d63f095bff 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2668,6 +2668,7 @@ static void spapr_machine_init(MachineState *machine)
>       char *filename;
>       Error *resize_hpt_err = NULL;
> +    if (mc->nvdimm_supported) { // by default it is always enabled
> +        ms->nvdimms_state->is_enabled = true;
> +    }
>       msi_nonbroken = true;
>   
>       QLIST_INIT(&spapr->phbs);
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..b11c85dbaa 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -57,7 +57,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>        * ensure that, if the user sets nvdimm=off, we error out
>        * regardless of being 5.1 or newer.
>        */
> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> +    if (!ms->nvdimms_state->is_enabled) {
>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>           return false;
>       }
> 
> if I didn't miss something, that way we do not abuse nvdimm_opt and still
> have nvdimm enabled by default and error out if it turns to disabled for whatever reason.

Yes, this looks correct.  I see you have already CCed Daniel and David.

Paolo

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Daniel Henrique Barboza 5 years, 2 months ago


On 12/7/20 1:07 PM, Igor Mammedov wrote:
> On Wed,  2 Dec 2020 03:18:49 -0500
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Machine options can be retrieved as properties of the machine object.
>> Encourage that by removing the "easy" accessor to machine options.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c     | 11 ++++-------
>>   hw/arm/boot.c           |  2 +-
>>   hw/microblaze/boot.c    |  9 ++++-----
>>   hw/nios2/boot.c         |  9 ++++-----
>>   hw/ppc/e500.c           |  5 ++---
>>   hw/ppc/spapr_nvdimm.c   |  4 ++--
>>   hw/ppc/virtex_ml507.c   |  2 +-
>>   hw/riscv/sifive_u.c     |  6 ++----
>>   hw/riscv/virt.c         |  6 ++----
>>   hw/xtensa/xtfpga.c      |  9 ++++-----
>>   include/sysemu/sysemu.h |  2 --
>>   softmmu/device_tree.c   |  2 +-
>>   softmmu/vl.c            |  2 +-
>>   13 files changed, 28 insertions(+), 41 deletions(-)
>>
> [...]
>> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
>> index a833a63b5e..84715a4d78 100644
>> --- a/hw/ppc/spapr_nvdimm.c
>> +++ b/hw/ppc/spapr_nvdimm.c
>> @@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>>   {
>>       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>>       const MachineState *ms = MACHINE(hotplug_dev);
>> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
>>       g_autofree char *uuidstr = NULL;
>>       QemuUUID uuid;
>>       int ret;
>> @@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>>        * ensure that, if the user sets nvdimm=off, we error out
>>        * regardless of being 5.1 or newer.
>>        */
>> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
>> +    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
>>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>>           return false;
>>       }
>> +    ms->nvdimms_state->is_enabled = true;
> 
> it looks like nvdimm is always enabled since 5.0 and there is no way to disable it


I'm not sure I'm following this statement. Testing here with all patches
up to this one applied, in a x86 machine:


$ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc,nvdimm=off -object memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device nvdimm,id=dimm0,memdev=mem0
qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: missing 'nvdimm' in '-M'
$
$ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc -object memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device nvdimm,id=dimm0,memdev=mem0
qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: missing 'nvdimm' in '-M'
$

This is the x86 NVDIMM behavior I considered when patching pseries NVDIMM support.
As Paolo mentioned in patch 09, pseries has different default values. We screwed
it up when pushing the first version of pseries NVDIMM support and we ended up
enabling it by default, as opposed to disabling it by default like x86. One release
later people complained that we weren't honoring 'nvdimm=off' to disable NVDIMM
support. The path of less pain that I chose was to at the very least disable
the support when "nvdimm=off" was specified by the user.





> how about dropping 9/15 and just error out is it's not enabled, something like:
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..d63f095bff 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2668,6 +2668,7 @@ static void spapr_machine_init(MachineState *machine)
>       char *filename;
>       Error *resize_hpt_err = NULL;
> +    if (mc->nvdimm_supported) { // by default it is always enabled
> +        ms->nvdimms_state->is_enabled = true;
> +    }
>       msi_nonbroken = true;
>   
>       QLIST_INIT(&spapr->phbs);
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..b11c85dbaa 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -57,7 +57,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>        * ensure that, if the user sets nvdimm=off, we error out
>        * regardless of being 5.1 or newer.
>        */
> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> +    if (!ms->nvdimms_state->is_enabled) {
>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>           return false;
>       }
> 
> if I didn't miss something, that way we do not abuse nvdimm_opt and still
> have nvdimm enabled by default and error out if it turns to disabled for whatever reason.


Just tried this out. This doesn't disable the NVDIMM support when passing 'nvdimm=off'
machine option.


As far pseries NVDIMM support goes, we'll need patch 09 and to consider nvdimm_opt
to keep the same behavior we already have today, like Paolo is already doing in
this patch.



Thanks,


DHB

> 
> 
>>       if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
>>                                   &error_abort) == 0) {
> [...]
> 
>   
>

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Igor Mammedov 5 years, 2 months ago

On Mon, 7 Dec 2020 23:32:55 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 12/7/20 1:07 PM, Igor Mammedov wrote:
> > On Wed,  2 Dec 2020 03:18:49 -0500
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> Machine options can be retrieved as properties of the machine object.
> >> Encourage that by removing the "easy" accessor to machine options.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>   accel/kvm/kvm-all.c     | 11 ++++-------
> >>   hw/arm/boot.c           |  2 +-
> >>   hw/microblaze/boot.c    |  9 ++++-----
> >>   hw/nios2/boot.c         |  9 ++++-----
> >>   hw/ppc/e500.c           |  5 ++---
> >>   hw/ppc/spapr_nvdimm.c   |  4 ++--
> >>   hw/ppc/virtex_ml507.c   |  2 +-
> >>   hw/riscv/sifive_u.c     |  6 ++----
> >>   hw/riscv/virt.c         |  6 ++----
> >>   hw/xtensa/xtfpga.c      |  9 ++++-----
> >>   include/sysemu/sysemu.h |  2 --
> >>   softmmu/device_tree.c   |  2 +-
> >>   softmmu/vl.c            |  2 +-
> >>   13 files changed, 28 insertions(+), 41 deletions(-)
> >>  
> > [...]  
> >> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> >> index a833a63b5e..84715a4d78 100644
> >> --- a/hw/ppc/spapr_nvdimm.c
> >> +++ b/hw/ppc/spapr_nvdimm.c
> >> @@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> >>   {
> >>       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> >>       const MachineState *ms = MACHINE(hotplug_dev);
> >> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
> >>       g_autofree char *uuidstr = NULL;
> >>       QemuUUID uuid;
> >>       int ret;
> >> @@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> >>        * ensure that, if the user sets nvdimm=off, we error out
> >>        * regardless of being 5.1 or newer.
> >>        */
> >> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> >> +    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
> >>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
> >>           return false;
> >>       }
> >> +    ms->nvdimms_state->is_enabled = true;  
> > 
> > it looks like nvdimm is always enabled since 5.0 and there is no way to disable it  
> 
> 
> I'm not sure I'm following this statement. Testing here with all patches
> up to this one applied, in a x86 machine:
> 
> 
> $ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc,nvdimm=off -object memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device nvdimm,id=dimm0,memdev=mem0
> qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: missing 'nvdimm' in '-M'
> $
> $ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc -object memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device nvdimm,id=dimm0,memdev=mem0
> qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: missing 'nvdimm' in '-M'
> $
> 
> This is the x86 NVDIMM behavior I considered when patching pseries NVDIMM support.
> As Paolo mentioned in patch 09, pseries has different default values. We screwed
> it up when pushing the first version of pseries NVDIMM support and we ended up
> enabling it by default, as opposed to disabling it by default like x86. One release
> later people complained that we weren't honoring 'nvdimm=off' to disable NVDIMM
> support. The path of less pain that I chose was to at the very least disable
> the support when "nvdimm=off" was specified by the user.
> 
> 
> 
> 
> 
> > how about dropping 9/15 and just error out is it's not enabled, something like:
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b7e0894019..d63f095bff 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2668,6 +2668,7 @@ static void spapr_machine_init(MachineState *machine)
> >       char *filename;
> >       Error *resize_hpt_err = NULL;
> > +    if (mc->nvdimm_supported) { // by default it is always enabled
> > +        ms->nvdimms_state->is_enabled = true;
> > +    }
> >       msi_nonbroken = true;
> >   
> >       QLIST_INIT(&spapr->phbs);
> > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> > index a833a63b5e..b11c85dbaa 100644
> > --- a/hw/ppc/spapr_nvdimm.c
> > +++ b/hw/ppc/spapr_nvdimm.c
> > @@ -57,7 +57,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> >        * ensure that, if the user sets nvdimm=off, we error out
> >        * regardless of being 5.1 or newer.
> >        */
> > -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> > +    if (!ms->nvdimms_state->is_enabled) {
> >           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
> >           return false;
> >       }
> > 
> > if I didn't miss something, that way we do not abuse nvdimm_opt and still
> > have nvdimm enabled by default and error out if it turns to disabled for whatever reason.  
> 
> 
> Just tried this out. This doesn't disable the NVDIMM support when passing 'nvdimm=off'
> machine option.

that's not really working, but rather idea (spapr_machine_init is too late for the task).
I'll post a path that should do the job in a minute.

> 
> As far pseries NVDIMM support goes, we'll need patch 09 and to consider nvdimm_opt
> to keep the same behavior we already have today, like Paolo is already doing in
> this patch.
> 
> 
> 
> Thanks,
> 
> 
> DHB
> 
> > 
> >   
> >>       if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
> >>                                   &error_abort) == 0) {  
> > [...]
> > 
> >   
> >   
>

[PATCH] ppc/spapr: cleanup -machine pseries,nvdimm=X handling

Posted by Igor Mammedov 5 years, 2 months ago

Since NVDIMM support was introduced on pseries machine,
it ignored machine's nvdimm=on|off option and effectively
was always enabled on machines that support NVDIMM.
Later on commit
  (28f5a716212 ppc/spapr_nvdimm: do not enable support with 'nvdimm=off')
makes QEMU error out in case user explicitly set 'nvdimm=off'
on CLI by peeking at machine_opts.

However that's a bit hacking and going away (in world where
machine configured over QMP) and it also leaves
  nvdimms_state->is_enabled
in inconsistent state (false) even when it should be set true
by default.

Instead of using on machine_opts, implement per machine
"nvdimm enabled" default handling and set default enabled value
at machine class init time (which is set to true for pseries)
and properly handle it generic machine code.
That way pseries will have, nvdimm enabled by default and
will honor user provided 'nvdimm=on|off'.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
Patch should be applied on top of:
  [PATCH 08/15] machine: introduce MachineInitPhase
---
 include/hw/boards.h   |  1 +
 hw/core/machine.c     |  1 +
 hw/ppc/spapr.c        |  8 ++++++++
 hw/ppc/spapr_nvdimm.c | 14 +-------------
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index b9233af54a..1730f151aa 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -206,6 +206,7 @@ struct MachineClass {
     bool ignore_boot_device_suffixes;
     bool smbus_no_migration_support;
     bool nvdimm_supported;
+    bool nvdimm_enabled_default;
     bool numa_mem_supported;
     bool auto_enable_numa;
     const char *default_ram_id;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2c0bc15143..12f04bed58 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -893,6 +893,7 @@ static void machine_initfn(Object *obj)
         Object *obj = OBJECT(ms);
 
         ms->nvdimms_state = g_new0(NVDIMMState, 1);
+        ms->nvdimms_state->is_enabled = mc->nvdimm_enabled_default;
         object_property_add_bool(obj, "nvdimm",
                                  machine_get_nvdimm, machine_set_nvdimm);
         object_property_set_description(obj, "nvdimm",
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..5a0cf79bbe 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4413,6 +4413,14 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
     mc->has_hotpluggable_cpus = true;
     mc->nvdimm_supported = true;
+    /*
+     * NVDIMM support went live in 5.1 without considering that, in
+     * other archs, the user needs to enable NVDIMM support with the
+     * 'nvdimm' machine option and the default behavior is NVDIMM
+     * support disabled. It is too late to roll back to the standard
+     * behavior without breaking 5.1 guests.
+     */
+    mc->nvdimm_enabled_default = true;
     smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..66cd3dc13f 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -27,10 +27,8 @@
 #include "hw/ppc/spapr_nvdimm.h"
 #include "hw/mem/nvdimm.h"
 #include "qemu/nvdimm-utils.h"
-#include "qemu/option.h"
 #include "hw/ppc/fdt.h"
 #include "qemu/range.h"
-#include "sysemu/sysemu.h"
 #include "hw/ppc/spapr_numa.h"
 
 bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
@@ -38,7 +36,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 {
     const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     const MachineState *ms = MACHINE(hotplug_dev);
-    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
     g_autofree char *uuidstr = NULL;
     QemuUUID uuid;
     int ret;
@@ -48,16 +45,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
         return false;
     }
 
-    /*
-     * NVDIMM support went live in 5.1 without considering that, in
-     * other archs, the user needs to enable NVDIMM support with the
-     * 'nvdimm' machine option and the default behavior is NVDIMM
-     * support disabled. It is too late to roll back to the standard
-     * behavior without breaking 5.1 guests. What we can do is to
-     * ensure that, if the user sets nvdimm=off, we error out
-     * regardless of being 5.1 or newer.
-     */
-    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+    if (!ms->nvdimms_state->is_enabled) {
         error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
         return false;
     }
-- 
2.27.0

[PATCH v2] ppc/spapr: cleanup -machine pseries,nvdimm=X handling

Posted by Igor Mammedov 5 years, 2 months ago

Since NVDIMM support was introduced on pseries machine,
it ignored machine's nvdimm=on|off option and effectively
was always enabled on machines that support NVDIMM.
Later on commit
  (28f5a716212 ppc/spapr_nvdimm: do not enable support with 'nvdimm=off')
makes QEMU error out in case user explicitly set 'nvdimm=off'
on CLI by peeking at machine_opts.

However that's a workaround and leaves 'nvdimms_state->is_enabled'
in inconsistent state (false) when it should be set true
by default.

Instead of using on machine_opts, set default to true for pseries
machine in initfn time. If user sets manually 'nvdimm=off'
it will overwrite default value to false and QEMU will error
as expected without need to peek into machine_opts.

That way pseries will have, nvdimm enabled by default and
will honor user provided 'nvdimm=on|off'.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: danielhb413@gmail.com
CC: david@gibson.dropbear.id.au
CC: pbonzini@redhat.com

v2:
  - simplify a bit more by using spapr_instance_init() to set
    default value instead of doing it in generic machine code

PS:
Patch should be applied on top of:
  [PATCH 08/15] machine: introduce MachineInitPhase
---
 hw/ppc/spapr.c        | 13 +++++++++++++
 hw/ppc/spapr_nvdimm.c | 14 +-------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..803a6f52a2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3267,6 +3267,19 @@ static void spapr_instance_init(Object *obj)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(obj);
     SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    MachineState *ms = MACHINE(spapr);
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    /*
+     * NVDIMM support went live in 5.1 without considering that, in
+     * other archs, the user needs to enable NVDIMM support with the
+     * 'nvdimm' machine option and the default behavior is NVDIMM
+     * support disabled. It is too late to roll back to the standard
+     * behavior without breaking 5.1 guests.
+     */
+    if (mc->nvdimm_supported) {
+        ms->nvdimms_state->is_enabled = true;
+    }
 
     spapr->htab_fd = -1;
     spapr->use_hotplug_event_source = true;
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..66cd3dc13f 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -27,10 +27,8 @@
 #include "hw/ppc/spapr_nvdimm.h"
 #include "hw/mem/nvdimm.h"
 #include "qemu/nvdimm-utils.h"
-#include "qemu/option.h"
 #include "hw/ppc/fdt.h"
 #include "qemu/range.h"
-#include "sysemu/sysemu.h"
 #include "hw/ppc/spapr_numa.h"
 
 bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
@@ -38,7 +36,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 {
     const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     const MachineState *ms = MACHINE(hotplug_dev);
-    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
     g_autofree char *uuidstr = NULL;
     QemuUUID uuid;
     int ret;
@@ -48,16 +45,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
         return false;
     }
 
-    /*
-     * NVDIMM support went live in 5.1 without considering that, in
-     * other archs, the user needs to enable NVDIMM support with the
-     * 'nvdimm' machine option and the default behavior is NVDIMM
-     * support disabled. It is too late to roll back to the standard
-     * behavior without breaking 5.1 guests. What we can do is to
-     * ensure that, if the user sets nvdimm=off, we error out
-     * regardless of being 5.1 or newer.
-     */
-    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+    if (!ms->nvdimms_state->is_enabled) {
         error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
         return false;
     }
-- 
2.27.0

Re: [PATCH v2] ppc/spapr: cleanup -machine pseries,nvdimm=X handling

Posted by Daniel Henrique Barboza 5 years, 2 months ago


On 12/8/20 1:46 PM, Igor Mammedov wrote:
> Since NVDIMM support was introduced on pseries machine,
> it ignored machine's nvdimm=on|off option and effectively
> was always enabled on machines that support NVDIMM.
> Later on commit
>    (28f5a716212 ppc/spapr_nvdimm: do not enable support with 'nvdimm=off')
> makes QEMU error out in case user explicitly set 'nvdimm=off'
> on CLI by peeking at machine_opts.
> 
> However that's a workaround and leaves 'nvdimms_state->is_enabled'
> in inconsistent state (false) when it should be set true
> by default.
> 
> Instead of using on machine_opts, set default to true for pseries
> machine in initfn time. If user sets manually 'nvdimm=off'
> it will overwrite default value to false and QEMU will error
> as expected without need to peek into machine_opts.
> 
> That way pseries will have, nvdimm enabled by default and

nit: extra ',' here

> will honor user provided 'nvdimm=on|off'.

I believe it's plausible to add a:

Fixes: 28f5a716212 ("ppc/spapr_nvdimm: do not enable support with 'nvdimm=off'")

To indicate that this is amending my commit you mentioned up there.


> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

Thanks for taking the time patching this up. Tested on top of Patch 08 in a
Power 9 host and it works as intended.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


ps: I'm assuming that that this is deprecating Paolo's patch:

"[PATCH 09/15] machine: record whether nvdimm= was set"

and also the chunks of Patch 10/15 that are changing spapr_nvdimm.c. If that's
not the case, let me know and I'll re-test.



Thanks,


DHB



> CC: danielhb413@gmail.com
> CC: david@gibson.dropbear.id.au
> CC: pbonzini@redhat.com
> 
> v2:
>    - simplify a bit more by using spapr_instance_init() to set
>      default value instead of doing it in generic machine code
> 
> PS:
> Patch should be applied on top of:
>    [PATCH 08/15] machine: introduce MachineInitPhase
> ---
>   hw/ppc/spapr.c        | 13 +++++++++++++
>   hw/ppc/spapr_nvdimm.c | 14 +-------------
>   2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..803a6f52a2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3267,6 +3267,19 @@ static void spapr_instance_init(Object *obj)
>   {
>       SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>       SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    MachineState *ms = MACHINE(spapr);
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    /*
> +     * NVDIMM support went live in 5.1 without considering that, in
> +     * other archs, the user needs to enable NVDIMM support with the
> +     * 'nvdimm' machine option and the default behavior is NVDIMM
> +     * support disabled. It is too late to roll back to the standard
> +     * behavior without breaking 5.1 guests.
> +     */
> +    if (mc->nvdimm_supported) {
> +        ms->nvdimms_state->is_enabled = true;
> +    }
>   
>       spapr->htab_fd = -1;
>       spapr->use_hotplug_event_source = true;
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..66cd3dc13f 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -27,10 +27,8 @@
>   #include "hw/ppc/spapr_nvdimm.h"
>   #include "hw/mem/nvdimm.h"
>   #include "qemu/nvdimm-utils.h"
> -#include "qemu/option.h"
>   #include "hw/ppc/fdt.h"
>   #include "qemu/range.h"
> -#include "sysemu/sysemu.h"
>   #include "hw/ppc/spapr_numa.h"
>   
>   bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> @@ -38,7 +36,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>   {
>       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>       const MachineState *ms = MACHINE(hotplug_dev);
> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
>       g_autofree char *uuidstr = NULL;
>       QemuUUID uuid;
>       int ret;
> @@ -48,16 +45,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>           return false;
>       }
>   
> -    /*
> -     * NVDIMM support went live in 5.1 without considering that, in
> -     * other archs, the user needs to enable NVDIMM support with the
> -     * 'nvdimm' machine option and the default behavior is NVDIMM
> -     * support disabled. It is too late to roll back to the standard
> -     * behavior without breaking 5.1 guests. What we can do is to
> -     * ensure that, if the user sets nvdimm=off, we error out
> -     * regardless of being 5.1 or newer.
> -     */
> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> +    if (!ms->nvdimms_state->is_enabled) {
>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>           return false;
>       }
>

Re: [PATCH v2] ppc/spapr: cleanup -machine pseries,nvdimm=X handling

Posted by Igor Mammedov 5 years, 2 months ago

On Tue, 8 Dec 2020 14:24:22 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 12/8/20 1:46 PM, Igor Mammedov wrote:
> > Since NVDIMM support was introduced on pseries machine,
> > it ignored machine's nvdimm=on|off option and effectively
> > was always enabled on machines that support NVDIMM.
> > Later on commit
> >    (28f5a716212 ppc/spapr_nvdimm: do not enable support with 'nvdimm=off')
> > makes QEMU error out in case user explicitly set 'nvdimm=off'
> > on CLI by peeking at machine_opts.
> > 
> > However that's a workaround and leaves 'nvdimms_state->is_enabled'
> > in inconsistent state (false) when it should be set true
> > by default.
> > 
> > Instead of using on machine_opts, set default to true for pseries
> > machine in initfn time. If user sets manually 'nvdimm=off'
> > it will overwrite default value to false and QEMU will error
> > as expected without need to peek into machine_opts.
> > 
> > That way pseries will have, nvdimm enabled by default and  
> 
> nit: extra ',' here
> 
> > will honor user provided 'nvdimm=on|off'.  
> 
> I believe it's plausible to add a:
> 
> Fixes: 28f5a716212 ("ppc/spapr_nvdimm: do not enable support with 'nvdimm=off'")
> 
> To indicate that this is amending my commit you mentioned up there.
> 
> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---  
> 
> Thanks for taking the time patching this up. Tested on top of Patch 08 in a
> Power 9 host and it works as intended.
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
> ps: I'm assuming that that this is deprecating Paolo's patch:
> 
> "[PATCH 09/15] machine: record whether nvdimm= was set"
> 
> and also the chunks of Patch 10/15 that are changing spapr_nvdimm.c. If that's
> not the case, let me know and I'll re-test.

yes, it does deprecate those.
And it is based on this series, so I'd expect Paolo to incorporate it,
to avoid churn/conflicts.

> 
> 
> 
> Thanks,
> 
> 
> DHB
> 
> 
> 
> > CC: danielhb413@gmail.com
> > CC: david@gibson.dropbear.id.au
> > CC: pbonzini@redhat.com
> > 
> > v2:
> >    - simplify a bit more by using spapr_instance_init() to set
> >      default value instead of doing it in generic machine code
> > 
> > PS:
> > Patch should be applied on top of:
> >    [PATCH 08/15] machine: introduce MachineInitPhase
> > ---
> >   hw/ppc/spapr.c        | 13 +++++++++++++
> >   hw/ppc/spapr_nvdimm.c | 14 +-------------
> >   2 files changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b7e0894019..803a6f52a2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3267,6 +3267,19 @@ static void spapr_instance_init(Object *obj)
> >   {
> >       SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> >       SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > +    MachineState *ms = MACHINE(spapr);
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > +    /*
> > +     * NVDIMM support went live in 5.1 without considering that, in
> > +     * other archs, the user needs to enable NVDIMM support with the
> > +     * 'nvdimm' machine option and the default behavior is NVDIMM
> > +     * support disabled. It is too late to roll back to the standard
> > +     * behavior without breaking 5.1 guests.
> > +     */
> > +    if (mc->nvdimm_supported) {
> > +        ms->nvdimms_state->is_enabled = true;
> > +    }
> >   
> >       spapr->htab_fd = -1;
> >       spapr->use_hotplug_event_source = true;
> > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> > index a833a63b5e..66cd3dc13f 100644
> > --- a/hw/ppc/spapr_nvdimm.c
> > +++ b/hw/ppc/spapr_nvdimm.c
> > @@ -27,10 +27,8 @@
> >   #include "hw/ppc/spapr_nvdimm.h"
> >   #include "hw/mem/nvdimm.h"
> >   #include "qemu/nvdimm-utils.h"
> > -#include "qemu/option.h"
> >   #include "hw/ppc/fdt.h"
> >   #include "qemu/range.h"
> > -#include "sysemu/sysemu.h"
> >   #include "hw/ppc/spapr_numa.h"
> >   
> >   bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> > @@ -38,7 +36,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> >   {
> >       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> >       const MachineState *ms = MACHINE(hotplug_dev);
> > -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
> >       g_autofree char *uuidstr = NULL;
> >       QemuUUID uuid;
> >       int ret;
> > @@ -48,16 +45,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> >           return false;
> >       }
> >   
> > -    /*
> > -     * NVDIMM support went live in 5.1 without considering that, in
> > -     * other archs, the user needs to enable NVDIMM support with the
> > -     * 'nvdimm' machine option and the default behavior is NVDIMM
> > -     * support disabled. It is too late to roll back to the standard
> > -     * behavior without breaking 5.1 guests. What we can do is to
> > -     * ensure that, if the user sets nvdimm=off, we error out
> > -     * regardless of being 5.1 or newer.
> > -     */
> > -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> > +    if (!ms->nvdimms_state->is_enabled) {
> >           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
> >           return false;
> >       }
> >   
>

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Daniel Henrique Barboza 5 years, 2 months ago


On 12/2/20 5:18 AM, Paolo Bonzini wrote:
> Machine options can be retrieved as properties of the machine object.
> Encourage that by removing the "easy" accessor to machine options.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   accel/kvm/kvm-all.c     | 11 ++++-------
>   hw/arm/boot.c           |  2 +-
>   hw/microblaze/boot.c    |  9 ++++-----
>   hw/nios2/boot.c         |  9 ++++-----
>   hw/ppc/e500.c           |  5 ++---
>   hw/ppc/spapr_nvdimm.c   |  4 ++--
>   hw/ppc/virtex_ml507.c   |  2 +-
>   hw/riscv/sifive_u.c     |  6 ++----
>   hw/riscv/virt.c         |  6 ++----
>   hw/xtensa/xtfpga.c      |  9 ++++-----
>   include/sysemu/sysemu.h |  2 --
>   softmmu/device_tree.c   |  2 +-
>   softmmu/vl.c            |  2 +-
>   13 files changed, 28 insertions(+), 41 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index baaa54249d..666b9ab96c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2013,7 +2013,6 @@ static int kvm_init(MachineState *ms)
>       const KVMCapabilityInfo *missing_cap;
>       int ret;
>       int type = 0;
> -    const char *kvm_type;
>       uint64_t dirty_log_manual_caps;
>   
>       s = KVM_STATE(ms->accelerator);
> @@ -2069,13 +2068,11 @@ static int kvm_init(MachineState *ms)
>       }
>       s->as = g_new0(struct KVMAs, s->nr_as);
>   
> -    kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
> -    if (mc->kvm_type) {
> +    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
> +        g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine),
> +                                                            "kvm-type",
> +                                                            &error_abort);
>           type = mc->kvm_type(ms, kvm_type);



I'm afraid that this code has unintended consequences for pseries. When starting the VM
with '--enable-kvm' in a Power host I'm getting an error:


$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
qemu-system-ppc64: Unknown kvm-type specified ''


The reason is that spapr_kvm_type() expects kvm-type to be either NULL (i.e. no option
set, in the previous semantic), "HV" or "PR". This was the case for the previous
qemu_opt_get(), but with object_property_get_str() the absence of the option is
returning kvm_type = ''.

This means that, as far as pseries goes, inserting "kvm-type=" (blank string)
has the same effect:


$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries,accel=kvm,kvm-type=
qemu-system-ppc64: Unknown kvm-type specified ''


I investigated object_property_get_str() inner workings a bit and I wasn't able
to find a way for it to return NULL if kvm_type isn't specified.


I see three possible solutions for it:

1) interpret kvm_type='' as if kvm_type=NULL inside spapr_kvm_type():

static int spapr_kvm_type(MachineState *machine, const char *vm_type)
{
     if (!vm_type || strcmp(vm_type, "")) {
         return 0;
     }
(...)


This is kind of ugly because we're validating a "kvm_type=" scenario with it, but
it works.


2) find a way to make object_property_get_str() to return kvm_type = NULL if the
'kvm_type' option is absent  and keep spapr code untouched. I don't have the
knowledge to assess how hard this would be.


3) I can change the pseries logic to add an explicit default value for kvm_type=NULL
or kvm_type='' (i.e. no user input is given). We're already doing that in a sense, but
it's not exposed to the user. I would call it 'auto' and expose it to the user
as default value if no kvm_type is explictly set. This would enhance user experience
a bit and avoid having to deal with a scenario where kvm_type=(blank) is
a valid input.


David, if you think (3) is tolerable let me know and I can send a patch. IMO
this change adds a bit of value regardless of Paolo's change.


Thanks,



DHB



> -    } else if (kvm_type) {
> -        ret = -EINVAL;
> -        fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
> -        goto err;
>       }
>   
>       do {
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 4d9d47ba1c..e56c42ac22 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1299,7 +1299,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
>       info->kernel_filename = ms->kernel_filename;
>       info->kernel_cmdline = ms->kernel_cmdline;
>       info->initrd_filename = ms->initrd_filename;
> -    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> +    info->dtb_filename = ms->dtb;
>       info->dtb_limit = 0;
>   
>       /* Load the kernel.  */
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 6715ba2ff9..caaba1aa4c 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -34,6 +34,7 @@
>   #include "sysemu/device_tree.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>   #include "hw/loader.h"
>   #include "elf.h"
>   #include "qemu/cutils.h"
> @@ -116,16 +117,14 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>                               const char *dtb_filename,
>                               void (*machine_cpu_reset)(MicroBlazeCPU *))
>   {
> -    QemuOpts *machine_opts;
>       const char *kernel_filename;
>       const char *kernel_cmdline;
>       const char *dtb_arg;
>       char *filename = NULL;
>   
> -    machine_opts = qemu_get_machine_opts();
> -    kernel_filename = qemu_opt_get(machine_opts, "kernel");
> -    kernel_cmdline = qemu_opt_get(machine_opts, "append");
> -    dtb_arg = qemu_opt_get(machine_opts, "dtb");
> +    kernel_filename = current_machine->kernel_filename;
> +    kernel_cmdline = current_machine->kernel_cmdline;
> +    dtb_arg = current_machine->dtb;
>       /* default to pcbios dtb as passed by machine_init */
>       if (!dtb_arg && dtb_filename) {
>           filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
> index 95a8697906..d9969ac148 100644
> --- a/hw/nios2/boot.c
> +++ b/hw/nios2/boot.c
> @@ -39,6 +39,7 @@
>   #include "sysemu/device_tree.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>   #include "hw/loader.h"
>   #include "elf.h"
>   
> @@ -120,16 +121,14 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr ddr_base,
>                               const char *dtb_filename,
>                               void (*machine_cpu_reset)(Nios2CPU *))
>   {
> -    QemuOpts *machine_opts;
>       const char *kernel_filename;
>       const char *kernel_cmdline;
>       const char *dtb_arg;
>       char *filename = NULL;
>   
> -    machine_opts = qemu_get_machine_opts();
> -    kernel_filename = qemu_opt_get(machine_opts, "kernel");
> -    kernel_cmdline = qemu_opt_get(machine_opts, "append");
> -    dtb_arg = qemu_opt_get(machine_opts, "dtb");
> +    kernel_filename = current_machine->kernel_filename;
> +    kernel_cmdline = current_machine->kernel_cmdline;
> +    dtb_arg = current_machine->dtb;
>       /* default to pcbios dtb as passed by machine_init */
>       if (!dtb_arg) {
>           filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 6a64eb31ab..41dad2e583 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -343,9 +343,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
>               pmc->pci_pio_base >> 32, pmc->pci_pio_base,
>               0x0, 0x10000,
>           };
> -    QemuOpts *machine_opts = qemu_get_machine_opts();
> -    const char *dtb_file = qemu_opt_get(machine_opts, "dtb");
> -    const char *toplevel_compat = qemu_opt_get(machine_opts, "dt_compatible");
> +    const char *dtb_file = machine->dtb;
> +    const char *toplevel_compat = machine->dt_compatible;
>   
>       if (dtb_file) {
>           char *filename;
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..84715a4d78 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>   {
>       const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>       const MachineState *ms = MACHINE(hotplug_dev);
> -    const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
>       g_autofree char *uuidstr = NULL;
>       QemuUUID uuid;
>       int ret;
> @@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>        * ensure that, if the user sets nvdimm=off, we error out
>        * regardless of being 5.1 or newer.
>        */
> -    if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
> +    if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
>           error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
>           return false;
>       }
> +    ms->nvdimms_state->is_enabled = true;
>   
>       if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
>                                   &error_abort) == 0) {
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index 7f1bca928c..07fe49da0d 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -152,7 +152,7 @@ static int xilinx_load_device_tree(hwaddr addr,
>       int r;
>       const char *dtb_filename;
>   
> -    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> +    dtb_filename = current_machine->dtb;
>       if (dtb_filename) {
>           fdt = load_device_tree(dtb_filename, &fdt_size);
>           if (!fdt) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 2f19a9cda2..e7f6dc5fb3 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -100,14 +100,12 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
>       int cpu;
>       uint32_t *cells;
>       char *nodename;
> -    const char *dtb_filename;
>       char ethclk_names[] = "pclk\0hclk";
>       uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1;
>       uint32_t hfclk_phandle, rtcclk_phandle, phy_phandle;
>   
> -    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> -    if (dtb_filename) {
> -        fdt = s->fdt = load_device_tree(dtb_filename, &s->fdt_size);
> +    if (ms->dtb) {
> +        fdt = s->fdt = load_device_tree(ms->dtb, &s->fdt_size);
>           if (!fdt) {
>               error_report("load_device_tree() failed");
>               exit(1);
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 25cea7aa67..3cc18a76e7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -181,7 +181,6 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>   {
>       void *fdt;
>       int i, cpu, socket;
> -    const char *dtb_filename;
>       MachineState *mc = MACHINE(s);
>       uint64_t addr, size;
>       uint32_t *clint_cells, *plic_cells;
> @@ -195,9 +194,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>       hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
>       hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
>   
> -    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> -    if (dtb_filename) {
> -        fdt = s->fdt = load_device_tree(dtb_filename, &s->fdt_size);
> +    if (mc->dtb) {
> +        fdt = s->fdt = load_device_tree(mc->dtb, &s->fdt_size);
>           if (!fdt) {
>               error_report("load_device_tree() failed");
>               exit(1);
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index b1470b88e6..7be53f1895 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -233,11 +233,10 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>       qemu_irq *extints;
>       DriveInfo *dinfo;
>       PFlashCFI01 *flash = NULL;
> -    QemuOpts *machine_opts = qemu_get_machine_opts();
> -    const char *kernel_filename = qemu_opt_get(machine_opts, "kernel");
> -    const char *kernel_cmdline = qemu_opt_get(machine_opts, "append");
> -    const char *dtb_filename = qemu_opt_get(machine_opts, "dtb");
> -    const char *initrd_filename = qemu_opt_get(machine_opts, "initrd");
> +    const char *kernel_filename = machine->kernel_filename;
> +    const char *kernel_cmdline = machine->kernel_cmdline;
> +    const char *dtb_filename = machine->dtb;
> +    const char *initrd_filename = machine->initrd_filename;
>       const unsigned system_io_size = 224 * MiB;
>       uint32_t freq = 10000000;
>       int n;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 9b47cdca55..e8f463ff30 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -104,8 +104,6 @@ typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
>   void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
>   void qemu_boot_set(const char *boot_order, Error **errp);
>   
> -QemuOpts *qemu_get_machine_opts(void);
> -
>   bool defaults_enabled(void);
>   
>   void qemu_init(int argc, char **argv, char **envp);
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index b335dae707..b9a3ddc518 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -526,7 +526,7 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>   
>   void qemu_fdt_dumpdtb(void *fdt, int size)
>   {
> -    const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
> +    const char *dumpdtb = current_machine->dumpdtb;
>   
>       if (dumpdtb) {
>           /* Dump the dtb to a file and quit */
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 4fece1b9db..0f7222af31 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -481,7 +481,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
>    *
>    * Returns: machine options (never null).
>    */
> -QemuOpts *qemu_get_machine_opts(void)
> +static QemuOpts *qemu_get_machine_opts(void)
>   {
>       return qemu_find_opts_singleton("machine");
>   }
>

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

Posted by Paolo Bonzini 5 years, 2 months ago

On 08/12/20 03:16, Daniel Henrique Barboza wrote:
> 2) find a way to make object_property_get_str() to return kvm_type =
>  NULL if the 'kvm_type' option is absent  and keep spapr code
> untouched. I don't have the knowledge to assess how hard this would
> be.
> 
> 3) I can change the pseries logic to add an explicit default value
> for kvm_type=NULL or kvm_type='' (i.e. no user input is given). We're
> already doing that in a sense, but it's not exposed to the user. I
> would call it 'auto' and expose it to the user as default value if no
> kvm_type is explictly set. This would enhance user experience a bit
> and avoid having to deal with a scenario where kvm_type=(blank) is a
> valid input.
> 
> 
> David, if you think (3) is tolerable let me know and I can send a
> patch. IMO this change adds a bit of value regardless of Paolo's
> change.

Yes, I agree that (3) is a good idea.  If you send a patch I can 
integrate it in the series.

Paolo