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

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(-)

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;
>       }
>

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;
> >       }
> >   
>