Message ID | 20201208164606.4109134-1-imammedo@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] ppc/spapr: cleanup -machine pseries,nvdimm=X handling | expand |
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; > > } > > >
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; }
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(-)