Re: [PATCH for-6.1 v2] i386: do not call cpudef-only models functions for max, host, base

[Claudio Fontana]

On 11/29/21 6:17 PM, David Woodhouse wrote:
> On Mon, 2021-11-29 at 17:57 +0100, Claudio Fontana wrote:
>> On 11/29/21 4:11 PM, David Woodhouse wrote:
>>> On Mon, 2021-11-29 at 15:14 +0100, Claudio Fontana wrote:
>>>> On 11/29/21 12:39 PM, Woodhouse, David wrote:
>>>>> On Fri, 2021-07-23 at 13:29 +0200, Claudio Fontana wrote:
>>>>>>  static void kvm_cpu_instance_init(CPUState *cs)
>>>>>>  {
>>>>>>      X86CPU *cpu = X86_CPU(cs);
>>>>>> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>>>>>>
>>>>>>      host_cpu_instance_init(cpu);
>>>>>>
>>>>>> -    if (!kvm_irqchip_in_kernel()) {
>>>>>> -        x86_cpu_change_kvm_default("x2apic", "off");
>>>>>> -    } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
>>>>>> -        x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>>>>>> -    }
>>>>>> -
>>>>>> -    /* Special cases not set in the X86CPUDefinition structs: */
>>>>>> +    if (xcc->model) {
>>>>>> +        /* only applies to builtin_x86_defs cpus */
>>>>>> +        if (!kvm_irqchip_in_kernel()) {
>>>>>> +            x86_cpu_change_kvm_default("x2apic", "off");
>>>>>> +        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
>>>>>> +            x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>>>>>> +        }
>>>>>>
>>>>>> -    x86_cpu_apply_props(cpu, kvm_default_props);
>>>>>> +        /* Special cases not set in the X86CPUDefinition structs: */
>>>>>> +        x86_cpu_apply_props(cpu, kvm_default_props);
>>>>>> +    }
>>>>>>
>>>>>
>>>>> I think this causes a regression in x2apic and kvm-msi-ext-dest-id
>>>>> support. If you start qemu thus:
>>>>
>>>> If I recall correctly, this change just tries to restore the behavior 
>>>> prior to
>>>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448 ,
>>>>
>>>> fixing the issue introduced with the refactoring at that time.
>>>>
>>>> Can you try bisecting prior to
>>>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 , to see if the actual
>>>> breakage comes from somewhere else?
>>>
>>> Hm, so it looks like it never worked for '-cpu host' *until* commit
>>> f5cc5a5c16.
>>
>> Right, so here we are talking about properly supporting this for the first 
>> time.
>>
>> The fact that it works with f5cc5a5c16 is more an accident than anything 
>> else, that commit was clearly broken
>> (exemplified by reports of failed boots).
>>
>> So we need to find the proper solution, ie, exactly which features should be 
>> enabled for which cpu classes and models.
>>
>>>
>>> It didn't matter before c1bb5418e3 because you couldn't enable that
>>> many vCPUs without an IOMMU, and the *IOMMU* setup would call
>>> kvm_enable_x2apic().
>>>
>>> But after that, nothing ever called kvm_enable_x2apic() in the '-cpu
>>> host' case until commit f5cc5a5c16, which fixed it... until you
>>> restored the previous behaviour :)
>>>
>>> This "works" to fix this case, but presumably isn't correct:
>>
>> Right, we cannot just enable all this code, or the original refactor would 
>> have been right.
>>
>> These kvm default properties have been as far as I know intended for the cpu 
>> actual models (builtin_x86_defs),
>> and not for the special cpu classes max, host and base. This is what the 
>> revert addresses.
>>
>> I suspect what we actually need here is to review exactly in which specific 
>> cases kvm_enable_x2apic() should be called in the end.
>>
>> The code there is mixing changes to the kvm_default_props that are then 
>> applied using x86_cpu_apply_props (and that part should be only for 
>> xcc->model != NULL),
>> with the actual enablement of the kvm x2apic using kvm_vm_enable_cap(s, 
>> KVM_CAP_X2APIC_API, 0, flags) via kvm_enable_x2apic().
>>
>> One way is to ignore this detail and just move out those checks, since 
>> changes to kvm_default_props are harmless once we skip the 
>> x86_cpu_apply_props call,
>> as such: 
>>
>> ------
>>
>> static void kvm_cpu_instance_init(CPUState *cs)
>> {
>>     X86CPU *cpu = X86_CPU(cs);
>>     X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>>
>>     host_cpu_instance_init(cpu);
>>
>>     /* only applies to builtin_x86_defs cpus */
>>     if (!kvm_irqchip_in_kernel()) {
>>         x86_cpu_change_kvm_default("x2apic", "off");
>>     } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
>>         x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>>     }
>>
>>     if (xcc->model) {
>>         /* Special cases not set in the X86CPUDefinition structs: */
>>         x86_cpu_apply_props(cpu, kvm_default_props);
>>     }
>>
> 
> I don't believe that works in the case when kvm_enable_x2apic() fails
> on an older kernel. Although it sets the defaults, it still doesn't
> then *apply* them so it makes no difference.

Hmm I thought what you actually care for, for cpu "host", is just the 
kvm_enable_x2apic() call, not the kvm_default_props.

Do you also expect the kvm_default_prop "kvm-msi-ext-dest-id" to be switch to 
"on" and applied?

kvm_default_props were never applied to cpus without an x86 model definition 
(except for that brief time when I did it by mistake).


> 
> How about this:
> 
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -739,9 +739,9 @@ void pc_machine_done(Notifier *notifier, void *data)
>  
>  
>      if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> -        !kvm_irqchip_in_kernel()) {
> +        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
>          error_report("current -smp configuration requires kernel "
> -                     "irqchip support.");
> +                     "irqchip and X2APIC API support.");
>          exit(EXIT_FAILURE);
>      }
>  }
> 

Interesting. This would leave things like microvm out right? But maybe it's ok?

Ciao,

Claudio