Re: [PATCH 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL

[Daniel Henrique Barboza]

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