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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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