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

From: Igor Mammedov <imammedo@redhat.com>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, david@gibson.dropbear.id.au
Subject: Re: [PATCH 10/15] vl: make qemu_get_machine_opts static
Date: Tue, 8 Dec 2020 11:55:04 +0100	[thread overview]
Message-ID: <20201208115504.12df6180@redhat.com> (raw)
In-Reply-To: <44bc2a0b-ff1e-d9d5-772c-a5cbe23da94a@gmail.com>

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