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
