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

[Daniel Henrique Barboza]

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