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
