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

[David Woodhouse]

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.

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

smime.p7s
Description: S/MIME cryptographic signature