| Message ID | 20201208134536.1012045-1-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL | expand |
Hi Daniel, On Tue, 8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user, > DEFAULT_KVM_TYPE. This allows us 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 what > happens if the user does not set this option. This information 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> > --- > > The case I mentioned in the second paragraph is happening when we try to > execute a pseries guest with '--enable-kvm' using Paolo's patch > "vl: make qemu_get_machine_opts static" [1]: > > $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm > qemu-system-ppc64: Unknown kvm-type specified '' > > This happens due to the differences between how qemu_opt_get() and > object_property_get_str() works when there is no 'kvm-type' specified. > See [2] for more info about the issue found. > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html > [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html > > > hw/ppc/spapr.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b7e0894019..32d938dc6a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3021,9 +3021,10 @@ 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 (!strcmp(vm_type, DEFAULT_KVM_TYPE)) { > return 0; > } > > @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) > { > SpaprMachineState *spapr = SPAPR_MACHINE(obj); > > + /* > + * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE > + * instead of NULL. This allows us to be more predictable with what > + * is expected to happen in spapr_kvm_type(), since we can stop relying > + * on receiving a 'NULL' parameter as a valid input there. > + */ Returning what is obviously a default value is straightforward enough that is doesn't need to a comment IMHO. > + if (!spapr->kvm_type) { > + return g_strdup(DEFAULT_KVM_TYPE); > + } > + > return g_strdup(spapr->kvm_type); Alternatively you could simply do: return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); > } > > @@ -3273,7 +3284,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 (HV, PR)." Why not documenting "auto" as a valid option as well ? While here you could maybe convert HV and PR to lowercase in order to have a prettier "hv, pr, auto" set of valid values in the help string. You'd need to convert the related checks in spapr_kvm_type() to still check for the uppercase versions for compatibility, eg. by using g_ascii_strcasecmp(). > + " If not specified, defaults to any available KVM" > + " module loaded in the host. In case both kvm_hv" > + " and kvm_pr are loaded, kvm_hv takes precedence."); > + s/If not specified/If 'auto'/ and mention 'auto' as being the default value. > object_property_add_bool(obj, "modern-hotplug-events", > spapr_get_modern_hotplug_events, > spapr_set_modern_hotplug_events);
On 12/8/20 11:33 AM, Greg Kurz wrote: > Hi Daniel, > > On Tue, 8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user, >> DEFAULT_KVM_TYPE. This allows us 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 what >> happens if the user does not set this option. This information 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> >> --- >> >> The case I mentioned in the second paragraph is happening when we try to >> execute a pseries guest with '--enable-kvm' using Paolo's patch >> "vl: make qemu_get_machine_opts static" [1]: >> >> $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm >> qemu-system-ppc64: Unknown kvm-type specified '' >> >> This happens due to the differences between how qemu_opt_get() and >> object_property_get_str() works when there is no 'kvm-type' specified. >> See [2] for more info about the issue found. >> >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html >> [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html >> >> >> hw/ppc/spapr.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index b7e0894019..32d938dc6a 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -3021,9 +3021,10 @@ 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 (!strcmp(vm_type, DEFAULT_KVM_TYPE)) { >> return 0; >> } >> >> @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) >> { >> SpaprMachineState *spapr = SPAPR_MACHINE(obj); >> >> + /* >> + * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE >> + * instead of NULL. This allows us to be more predictable with what >> + * is expected to happen in spapr_kvm_type(), since we can stop relying >> + * on receiving a 'NULL' parameter as a valid input there. >> + */ > > Returning what is obviously a default value is straightforward enough > that is doesn't need to a comment IMHO. > >> + if (!spapr->kvm_type) { >> + return g_strdup(DEFAULT_KVM_TYPE); >> + } >> + >> return g_strdup(spapr->kvm_type); > > Alternatively you could simply do: > > return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); Got it. I'll update it in v2. > >> } >> >> @@ -3273,7 +3284,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 (HV, PR)." > > Why not documenting "auto" as a valid option as well ? This was my first idea but I got cold feet about it. I got afraid that exposing the default 'auto' option might misled users into believing that we're adding a new configuration option, even thought it's just the same behavior users are dealing with for 7+ years. I chose to use the 'auto' value as an internal default that isn't documented . Granted, this also means that we have now a hidden 'kvm-type=auto' setting, so yeah. If there is a consensus that exposing the "auto" default is the right thing to do in this case I'll just go ahead and expose it. > > While here you could maybe convert HV and PR to lowercase in order to > have a prettier "hv, pr, auto" set of valid values in the help string. > You'd need to convert the related checks in spapr_kvm_type() to still > check for the uppercase versions for compatibility, eg. by using > g_ascii_strcasecmp(). Roger that. > >> + " If not specified, defaults to any available KVM" >> + " module loaded in the host. In case both kvm_hv" >> + " and kvm_pr are loaded, kvm_hv takes precedence."); >> + > > s/If not specified/If 'auto'/ and mention 'auto' as being the default value. As I said above, I'll happily mention the 'auto' default if we can agree that this will not lead to user confusion thinking this is a new option and so on. Thanks, DHB > >> object_property_add_bool(obj, "modern-hotplug-events", >> spapr_get_modern_hotplug_events, >> spapr_set_modern_hotplug_events); >
On Tue, Dec 08, 2020 at 12:32:46PM -0300, Daniel Henrique Barboza wrote: > > > On 12/8/20 11:33 AM, Greg Kurz wrote: > > Hi Daniel, > > > > On Tue, 8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user, > > > DEFAULT_KVM_TYPE. This allows us 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 what > > > happens if the user does not set this option. This information 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> > > > --- > > > > > > The case I mentioned in the second paragraph is happening when we try to > > > execute a pseries guest with '--enable-kvm' using Paolo's patch > > > "vl: make qemu_get_machine_opts static" [1]: > > > > > > $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm > > > qemu-system-ppc64: Unknown kvm-type specified '' > > > > > > This happens due to the differences between how qemu_opt_get() and > > > object_property_get_str() works when there is no 'kvm-type' specified. > > > See [2] for more info about the issue found. > > > > > > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html > > > [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html > > > > > > > > > hw/ppc/spapr.c | 19 +++++++++++++++++-- > > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index b7e0894019..32d938dc6a 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -3021,9 +3021,10 @@ 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 (!strcmp(vm_type, DEFAULT_KVM_TYPE)) { > > > return 0; > > > } > > > @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) > > > { > > > SpaprMachineState *spapr = SPAPR_MACHINE(obj); > > > + /* > > > + * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE > > > + * instead of NULL. This allows us to be more predictable with what > > > + * is expected to happen in spapr_kvm_type(), since we can stop relying > > > + * on receiving a 'NULL' parameter as a valid input there. > > > + */ > > > > Returning what is obviously a default value is straightforward enough > > that is doesn't need to a comment IMHO. > > > > > + if (!spapr->kvm_type) { > > > + return g_strdup(DEFAULT_KVM_TYPE); > > > + } > > > + > > > return g_strdup(spapr->kvm_type); > > > > Alternatively you could simply do: > > > > return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE); > > Got it. I'll update it in v2. > > > > > > } > > > @@ -3273,7 +3284,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 (HV, PR)." > > > > Why not documenting "auto" as a valid option as well ? > > > This was my first idea but I got cold feet about it. I got afraid that > exposing the default 'auto' option might misled users into believing that > we're adding a new configuration option, even thought it's just the > same behavior users are dealing with for 7+ years. I chose to use the > 'auto' value as an internal default that isn't documented . Granted, this > also means that we have now a hidden 'kvm-type=auto' setting, so yeah. > > If there is a consensus that exposing the "auto" default is the right > thing to do in this case I'll just go ahead and expose it. I think it should be fine to expose it. > > While here you could maybe convert HV and PR to lowercase in order to > > have a prettier "hv, pr, auto" set of valid values in the help string. > > You'd need to convert the related checks in spapr_kvm_type() to still > > check for the uppercase versions for compatibility, eg. by using > > g_ascii_strcasecmp(). > > Roger that. > > > > > > + " If not specified, defaults to any available KVM" > > > + " module loaded in the host. In case both kvm_hv" > > > + " and kvm_pr are loaded, kvm_hv takes precedence."); > > > + > > > > s/If not specified/If 'auto'/ and mention 'auto' as being the default value. > > > As I said above, I'll happily mention the 'auto' default if we can agree > that this will not lead to user confusion thinking this is a new option and > so on. > > > Thanks, > > > DHB > > > > > > object_property_add_bool(obj, "modern-hotplug-events", > > > spapr_get_modern_hotplug_events, > > > spapr_set_modern_hotplug_events); > > >
To sum up everything:
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2d5aeeb45a..61f0963916 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine,
const char *vm_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;
}
@@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj,
Error **errp)
{
SpaprMachineState *spapr = SPAPR_MACHINE(obj);
- /*
- * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
- * instead of NULL. This allows us to be more predictable with what
- * is expected to happen in spapr_kvm_type(), since we can stop relying
- * on receiving a 'NULL' parameter as a valid input there.
- */
- if (!spapr->kvm_type) {
- return g_strdup(DEFAULT_KVM_TYPE);
- }
-
return g_strdup(spapr->kvm_type);
}
@@ -3294,11 +3284,13 @@ static void spapr_instance_init(Object *obj)
spapr->htab_fd = -1;
spapr->use_hotplug_event_source = true;
+
+ 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)."
- " If not specified, defaults to any
available KVM"
+ "Specifies the KVM virtualization
mode (hv, pr, auto)."
+ " auto is the default and allows
any available KVM"
" module loaded in the host. In
case both kvm_hv"
" and kvm_pr are loaded, kvm_hv
takes precedence.");
On Thu, 10 Dec 2020 13:34:59 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > To sum up everything: > LGTM > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 2d5aeeb45a..61f0963916 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine, > const char *vm_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; > } > > @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj, > Error **errp) > { > SpaprMachineState *spapr = SPAPR_MACHINE(obj); > > - /* > - * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE > - * instead of NULL. This allows us to be more predictable with what > - * is expected to happen in spapr_kvm_type(), since we can stop relying > - * on receiving a 'NULL' parameter as a valid input there. > - */ > - if (!spapr->kvm_type) { > - return g_strdup(DEFAULT_KVM_TYPE); > - } > - > return g_strdup(spapr->kvm_type); > } > > @@ -3294,11 +3284,13 @@ static void spapr_instance_init(Object *obj) > > spapr->htab_fd = -1; > spapr->use_hotplug_event_source = true; > + > + 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)." > - " If not specified, defaults to any > available KVM" > + "Specifies the KVM virtualization > mode (hv, pr, auto)." > + " auto is the default and allows > any available KVM" > " module loaded in the host. In > case both kvm_hv" > " and kvm_pr are loaded, kvm_hv > takes precedence."); >
On 12/10/20 9:47 AM, Greg Kurz wrote: > On Thu, 10 Dec 2020 13:34:59 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> To sum up everything: >> > > LGTM I just sent a v2 with a bit more done (e.g. added ignore case compare for 'auto'). Feel free to use that version or this one amended by this diff from Paolo. Thanks, DHB > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 2d5aeeb45a..61f0963916 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine, >> const char *vm_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; >> } >> >> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj, >> Error **errp) >> { >> SpaprMachineState *spapr = SPAPR_MACHINE(obj); >> >> - /* >> - * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE >> - * instead of NULL. This allows us to be more predictable with what >> - * is expected to happen in spapr_kvm_type(), since we can stop relying >> - * on receiving a 'NULL' parameter as a valid input there. >> - */ >> - if (!spapr->kvm_type) { >> - return g_strdup(DEFAULT_KVM_TYPE); >> - } >> - >> return g_strdup(spapr->kvm_type); >> } >> >> @@ -3294,11 +3284,13 @@ static void spapr_instance_init(Object *obj) >> >> spapr->htab_fd = -1; >> spapr->use_hotplug_event_source = true; >> + >> + 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)." >> - " If not specified, defaults to any >> available KVM" >> + "Specifies the KVM virtualization >> mode (hv, pr, auto)." >> + " auto is the default and allows >> any available KVM" >> " module loaded in the host. In >> case both kvm_hv" >> " and kvm_pr are loaded, kvm_hv >> takes precedence."); >> >
On Thu, 10 Dec 2020 10:10:21 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > > > On 12/10/20 9:47 AM, Greg Kurz wrote: > > On Thu, 10 Dec 2020 13:34:59 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> To sum up everything: > >> > > > > LGTM > > I just sent a v2 with a bit more done (e.g. added ignore case compare > for 'auto'). Feel free to use that version or this one amended by this > diff from Paolo. > The "bit more done" in your v2 is very important as it prevents spapr_kvm_type() to exit(1) if "auto" is explicitly passed on the command line. So I think we should go ahead with yours. > > Thanks, > > > DHB > > > > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 2d5aeeb45a..61f0963916 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine, > >> const char *vm_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; > >> } > >> > >> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj, > >> Error **errp) > >> { > >> SpaprMachineState *spapr = SPAPR_MACHINE(obj); > >> > >> - /* > >> - * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE > >> - * instead of NULL. This allows us to be more predictable with what > >> - * is expected to happen in spapr_kvm_type(), since we can stop relying > >> - * on receiving a 'NULL' parameter as a valid input there. > >> - */ > >> - if (!spapr->kvm_type) { > >> - return g_strdup(DEFAULT_KVM_TYPE); > >> - } > >> - > >> return g_strdup(spapr->kvm_type); > >> } > >> > >> @@ -3294,11 +3284,13 @@ static void spapr_instance_init(Object *obj) > >> > >> spapr->htab_fd = -1; > >> spapr->use_hotplug_event_source = true; > >> + > >> + 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)." > >> - " If not specified, defaults to any > >> available KVM" > >> + "Specifies the KVM virtualization > >> mode (hv, pr, auto)." > >> + " auto is the default and allows > >> any available KVM" > >> " module loaded in the host. In > >> case both kvm_hv" > >> " and kvm_pr are loaded, kvm_hv > >> takes precedence."); > >> > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b7e0894019..32d938dc6a 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3021,9 +3021,10 @@ 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 (!strcmp(vm_type, DEFAULT_KVM_TYPE)) { return 0; } @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp) { SpaprMachineState *spapr = SPAPR_MACHINE(obj); + /* + * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE + * instead of NULL. This allows us to be more predictable with what + * is expected to happen in spapr_kvm_type(), since we can stop relying + * on receiving a 'NULL' parameter as a valid input there. + */ + if (!spapr->kvm_type) { + return g_strdup(DEFAULT_KVM_TYPE); + } + return g_strdup(spapr->kvm_type); } @@ -3273,7 +3284,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 (HV, PR)." + " If not specified, defaults to any available KVM" + " module loaded in the host. In case both kvm_hv" + " and kvm_pr are loaded, kvm_hv takes precedence."); + object_property_add_bool(obj, "modern-hotplug-events", spapr_get_modern_hotplug_events, spapr_set_modern_hotplug_events);
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: "HV", "PR" and, if no kvm-type was set by the user, DEFAULT_KVM_TYPE. This allows us 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 what happens if the user does not set this option. This information 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> --- The case I mentioned in the second paragraph is happening when we try to execute a pseries guest with '--enable-kvm' using Paolo's patch "vl: make qemu_get_machine_opts static" [1]: $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm qemu-system-ppc64: Unknown kvm-type specified '' This happens due to the differences between how qemu_opt_get() and object_property_get_str() works when there is no 'kvm-type' specified. See [2] for more info about the issue found. [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html hw/ppc/spapr.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)