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

[Igor Mammedov]

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) {
[...]