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 <danielhb...@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 <danielhb...@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);
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
