* [PATCH v2 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL @ 2020-12-10 13:07 Daniel Henrique Barboza 2020-12-10 13:07 ` [PATCH v2 1/1] " Daniel Henrique Barboza 0 siblings, 1 reply; 5+ messages in thread From: Daniel Henrique Barboza @ 2020-12-10 13:07 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, Daniel Henrique Barboza, qemu-ppc, groug, david changes from v1, all proposed by Greg: * Removed comment at the start of spapr_get_kvm_type() * Added a one liner return in spapr_get_kvm_type() * use g_ascii_strcasecmp() in all str comparisons in spapr_kvm_type() * Exposed the 'auto' default kvm-type mode in the docs * changed HV and PR to lower-case in the docs v1 link: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01677.html This patch addresses an issue that happens with the pseries machine after testing Paolo's patch [1]: $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm qemu-system-ppc64: Unknown kvm-type specified '' The reason lies on how qemu_opt_get() and object_property_get_str() works when there is no 'kvm-type' specified. We were conting on receiving NULL for kvm-type, but the latter will use a blank string "". Instead on relying on NULL, let's expose the already existing 'auto' kvm-type mode to the users and use that as default. [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html Daniel Henrique Barboza (1): spapr.c: set a 'kvm-type' default value instead of relying on NULL hw/ppc/spapr.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL 2020-12-10 13:07 [PATCH v2 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL Daniel Henrique Barboza @ 2020-12-10 13:07 ` Daniel Henrique Barboza 2020-12-10 14:01 ` Greg Kurz 0 siblings, 1 reply; 5+ messages in thread From: Daniel Henrique Barboza @ 2020-12-10 13:07 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, Daniel Henrique Barboza, qemu-ppc, groug, david spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where the function returns 0. This is relying on the current QEMU machine options handling logic, where the absence of the 'kvm-type' option will be reflected as 'vm_type=NULL' in this function. This is not robust, and will break if QEMU options code decides to propagate something else in the case mentioned above (e.g. an empty string instead of NULL). Let's avoid this entirely by setting a non-NULL default value in case of no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed values of kvm-type: "auto", "hv", and "pr", with "auto" being the default if no kvm-type was set by the user. This allows us to always be predictable regardless of any enhancements/changes made in QEMU options mechanics. While we're at it, let's also document in 'kvm-type' description the already existing default mode, now named 'auto'. The information provided about it is based on how the pseries kernel handles the KVM_CREATE_VM ioctl(), where the default value '0' makes the kernel choose an available KVM module to use, giving precedence to kvm_hv. This logic is described in the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b7e0894019..f097f4ea30 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3021,17 +3021,18 @@ static void spapr_machine_init(MachineState *machine) qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); } +#define DEFAULT_KVM_TYPE "auto" static int spapr_kvm_type(MachineState *machine, const char *vm_type) { - if (!vm_type) { + if (!g_ascii_strcasecmp(vm_type, DEFAULT_KVM_TYPE)) { return 0; } - if (!strcmp(vm_type, "HV")) { + if (!g_ascii_strcasecmp(vm_type, "hv")) { return 1; } - if (!strcmp(vm_type, "PR")) { + if (!g_ascii_strcasecmp(vm_type, "pr")) { return 2; } @@ -3131,7 +3132,7 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) { SpaprMachineState *spapr = SPAPR_MACHINE(obj); - return g_strdup(spapr->kvm_type); + return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); } static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp) @@ -3273,7 +3274,11 @@ static void spapr_instance_init(Object *obj) object_property_add_str(obj, "kvm-type", spapr_get_kvm_type, spapr_set_kvm_type); object_property_set_description(obj, "kvm-type", - "Specifies the KVM virtualization mode (HV, PR)"); + "Specifies the KVM virtualization mode (auto," + " hv, pr). Defaults to 'auto'. This mode will use" + " any available KVM module loaded in the host," + " where kvm_hv takes precedence if both kvm_hv and" + " kvm_pr are loaded."); object_property_add_bool(obj, "modern-hotplug-events", spapr_get_modern_hotplug_events, spapr_set_modern_hotplug_events); -- 2.26.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL 2020-12-10 13:07 ` [PATCH v2 1/1] " Daniel Henrique Barboza @ 2020-12-10 14:01 ` Greg Kurz 2020-12-10 14:14 ` Greg Kurz 0 siblings, 1 reply; 5+ messages in thread From: Greg Kurz @ 2020-12-10 14:01 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: pbonzini, qemu-ppc, qemu-devel, david On Thu, 10 Dec 2020 10:07:21 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where > the function returns 0. This is relying on the current QEMU machine > options handling logic, where the absence of the 'kvm-type' option > will be reflected as 'vm_type=NULL' in this function. > > This is not robust, and will break if QEMU options code decides to propagate > something else in the case mentioned above (e.g. an empty string instead > of NULL). > > Let's avoid this entirely by setting a non-NULL default value in case of > no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed > values of kvm-type: "auto", "hv", and "pr", with "auto" being the default > if no kvm-type was set by the user. This allows us to always be predictable > regardless of any enhancements/changes made in QEMU options mechanics. > > While we're at it, let's also document in 'kvm-type' description the > already existing default mode, now named 'auto'. The information provided > about it is based on how the pseries kernel handles the KVM_CREATE_VM > ioctl(), where the default value '0' makes the kernel choose an available > KVM module to use, giving precedence to kvm_hv. This logic is described in > the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm(). > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b7e0894019..f097f4ea30 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3021,17 +3021,18 @@ static void spapr_machine_init(MachineState *machine) > qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); > } > > +#define DEFAULT_KVM_TYPE "auto" > static int spapr_kvm_type(MachineState *machine, const char *vm_type) > { > - if (!vm_type) { > + if (!g_ascii_strcasecmp(vm_type, DEFAULT_KVM_TYPE)) { strcmp() would be better here : we don't want to support an already existing "AUTO" value like for the other ones. And, of course, add a comment to explain why the two other ones are handled differently. > return 0; > } > > - if (!strcmp(vm_type, "HV")) { > + if (!g_ascii_strcasecmp(vm_type, "hv")) { > return 1; > } > > - if (!strcmp(vm_type, "PR")) { > + if (!g_ascii_strcasecmp(vm_type, "pr")) { > return 2; > } > > @@ -3131,7 +3132,7 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) > { > SpaprMachineState *spapr = SPAPR_MACHINE(obj); > > - return g_strdup(spapr->kvm_type); > + return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); As in Paolo's diff, it seems better to simply initialize spapr->kvm_type ... > } > > static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp) > @@ -3273,7 +3274,11 @@ static void spapr_instance_init(Object *obj) ... here. spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE); > object_property_add_str(obj, "kvm-type", > spapr_get_kvm_type, spapr_set_kvm_type); > object_property_set_description(obj, "kvm-type", > - "Specifies the KVM virtualization mode (HV, PR)"); > + "Specifies the KVM virtualization mode (auto," > + " hv, pr). Defaults to 'auto'. This mode will use" > + " any available KVM module loaded in the host," > + " where kvm_hv takes precedence if both kvm_hv and" > + " kvm_pr are loaded."); > object_property_add_bool(obj, "modern-hotplug-events", > spapr_get_modern_hotplug_events, > spapr_set_modern_hotplug_events); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL 2020-12-10 14:01 ` Greg Kurz @ 2020-12-10 14:14 ` Greg Kurz 2020-12-10 14:19 ` Daniel Henrique Barboza 0 siblings, 1 reply; 5+ messages in thread From: Greg Kurz @ 2020-12-10 14:14 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: pbonzini, qemu-ppc, qemu-devel, david On Thu, 10 Dec 2020 15:01:20 +0100 Greg Kurz <groug@kaod.org> wrote: > On Thu, 10 Dec 2020 10:07:21 -0300 > Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > > > spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where > > the function returns 0. This is relying on the current QEMU machine > > options handling logic, where the absence of the 'kvm-type' option > > will be reflected as 'vm_type=NULL' in this function. > > > > This is not robust, and will break if QEMU options code decides to propagate > > something else in the case mentioned above (e.g. an empty string instead > > of NULL). > > > > Let's avoid this entirely by setting a non-NULL default value in case of > > no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed > > values of kvm-type: "auto", "hv", and "pr", with "auto" being the default > > if no kvm-type was set by the user. This allows us to always be predictable > > regardless of any enhancements/changes made in QEMU options mechanics. > > > > While we're at it, let's also document in 'kvm-type' description the > > already existing default mode, now named 'auto'. The information provided > > about it is based on how the pseries kernel handles the KVM_CREATE_VM > > ioctl(), where the default value '0' makes the kernel choose an available > > KVM module to use, giving precedence to kvm_hv. This logic is described in > > the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm(). > > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > --- > > hw/ppc/spapr.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index b7e0894019..f097f4ea30 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3021,17 +3021,18 @@ static void spapr_machine_init(MachineState *machine) > > qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); > > } > > > > +#define DEFAULT_KVM_TYPE "auto" > > static int spapr_kvm_type(MachineState *machine, const char *vm_type) > > { > > - if (!vm_type) { > > + if (!g_ascii_strcasecmp(vm_type, DEFAULT_KVM_TYPE)) { > > strcmp() would be better here : we don't want to support an > already existing "AUTO" value like for the other ones. > And you should also keep the check on vm_type since it is NULL if the kvm-type wasn't passed to the command line: qemu-system-ppc64: GLib: g_ascii_strcasecmp: assertion 's1 != NULL' failed > And, of course, add a comment to explain why the two other > ones are handled differently. > > > return 0; > > } > > > > - if (!strcmp(vm_type, "HV")) { > > + if (!g_ascii_strcasecmp(vm_type, "hv")) { > > return 1; > > } > > > > - if (!strcmp(vm_type, "PR")) { > > + if (!g_ascii_strcasecmp(vm_type, "pr")) { > > return 2; > > } > > > > @@ -3131,7 +3132,7 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) > > { > > SpaprMachineState *spapr = SPAPR_MACHINE(obj); > > > > - return g_strdup(spapr->kvm_type); > > + return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); > > As in Paolo's diff, it seems better to simply initialize spapr->kvm_type ... > > > } > > > > static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp) > > @@ -3273,7 +3274,11 @@ static void spapr_instance_init(Object *obj) > > ... here. > > spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE); > > > object_property_add_str(obj, "kvm-type", > > spapr_get_kvm_type, spapr_set_kvm_type); > > object_property_set_description(obj, "kvm-type", > > - "Specifies the KVM virtualization mode (HV, PR)"); > > + "Specifies the KVM virtualization mode (auto," > > + " hv, pr). Defaults to 'auto'. This mode will use" > > + " any available KVM module loaded in the host," > > + " where kvm_hv takes precedence if both kvm_hv and" > > + " kvm_pr are loaded."); > > object_property_add_bool(obj, "modern-hotplug-events", > > spapr_get_modern_hotplug_events, > > spapr_set_modern_hotplug_events); > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL 2020-12-10 14:14 ` Greg Kurz @ 2020-12-10 14:19 ` Daniel Henrique Barboza 0 siblings, 0 replies; 5+ messages in thread From: Daniel Henrique Barboza @ 2020-12-10 14:19 UTC (permalink / raw) To: Greg Kurz; +Cc: pbonzini, qemu-ppc, qemu-devel, david On 12/10/20 11:14 AM, Greg Kurz wrote: > On Thu, 10 Dec 2020 15:01:20 +0100 > Greg Kurz <groug@kaod.org> wrote: > >> On Thu, 10 Dec 2020 10:07:21 -0300 >> Daniel Henrique Barboza <danielhb413@gmail.com> wrote: >> >>> spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where >>> the function returns 0. This is relying on the current QEMU machine >>> options handling logic, where the absence of the 'kvm-type' option >>> will be reflected as 'vm_type=NULL' in this function. >>> >>> This is not robust, and will break if QEMU options code decides to propagate >>> something else in the case mentioned above (e.g. an empty string instead >>> of NULL). >>> >>> Let's avoid this entirely by setting a non-NULL default value in case of >>> no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed >>> values of kvm-type: "auto", "hv", and "pr", with "auto" being the default >>> if no kvm-type was set by the user. This allows us to always be predictable >>> regardless of any enhancements/changes made in QEMU options mechanics. >>> >>> While we're at it, let's also document in 'kvm-type' description the >>> already existing default mode, now named 'auto'. The information provided >>> about it is based on how the pseries kernel handles the KVM_CREATE_VM >>> ioctl(), where the default value '0' makes the kernel choose an available >>> KVM module to use, giving precedence to kvm_hv. This logic is described in >>> the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm(). >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> --- >>> hw/ppc/spapr.c | 15 ++++++++++----- >>> 1 file changed, 10 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index b7e0894019..f097f4ea30 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -3021,17 +3021,18 @@ static void spapr_machine_init(MachineState *machine) >>> qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); >>> } >>> >>> +#define DEFAULT_KVM_TYPE "auto" >>> static int spapr_kvm_type(MachineState *machine, const char *vm_type) >>> { >>> - if (!vm_type) { >>> + if (!g_ascii_strcasecmp(vm_type, DEFAULT_KVM_TYPE)) { >> >> strcmp() would be better here : we don't want to support an >> already existing "AUTO" value like for the other ones. >> > > And you should also keep the check on vm_type since it is NULL > if the kvm-type wasn't passed to the command line: > > qemu-system-ppc64: GLib: g_ascii_strcasecmp: assertion 's1 != NULL' failed Hm, I tested this case and didn't hit that. Perhaphs because I am testing this on top of Paolo's patches instead of master. I'll keep the NULL check as a fallback to 'auto' just to be safe then. DHB > >> And, of course, add a comment to explain why the two other >> ones are handled differently. >> >>> return 0; >>> } >>> >>> - if (!strcmp(vm_type, "HV")) { >>> + if (!g_ascii_strcasecmp(vm_type, "hv")) { >>> return 1; >>> } >>> >>> - if (!strcmp(vm_type, "PR")) { >>> + if (!g_ascii_strcasecmp(vm_type, "pr")) { >>> return 2; >>> } >>> >>> @@ -3131,7 +3132,7 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) >>> { >>> SpaprMachineState *spapr = SPAPR_MACHINE(obj); >>> >>> - return g_strdup(spapr->kvm_type); >>> + return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); >> >> As in Paolo's diff, it seems better to simply initialize spapr->kvm_type ... >> >>> } >>> >>> static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp) >>> @@ -3273,7 +3274,11 @@ static void spapr_instance_init(Object *obj) >> >> ... here. >> >> spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE); >> >>> object_property_add_str(obj, "kvm-type", >>> spapr_get_kvm_type, spapr_set_kvm_type); >>> object_property_set_description(obj, "kvm-type", >>> - "Specifies the KVM virtualization mode (HV, PR)"); >>> + "Specifies the KVM virtualization mode (auto," >>> + " hv, pr). Defaults to 'auto'. This mode will use" >>> + " any available KVM module loaded in the host," >>> + " where kvm_hv takes precedence if both kvm_hv and" >>> + " kvm_pr are loaded."); >>> object_property_add_bool(obj, "modern-hotplug-events", >>> spapr_get_modern_hotplug_events, >>> spapr_set_modern_hotplug_events); >> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-10 14:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-10 13:07 [PATCH v2 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL Daniel Henrique Barboza 2020-12-10 13:07 ` [PATCH v2 1/1] " Daniel Henrique Barboza 2020-12-10 14:01 ` Greg Kurz 2020-12-10 14:14 ` Greg Kurz 2020-12-10 14:19 ` Daniel Henrique Barboza
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.