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 20:55 +0100, Claudio Fontana wrote:
> On 11/29/21 8:19 PM, David Woodhouse wrote:
> > On Mon, 2021-11-29 at 20:10 +0100, Claudio Fontana wrote:
> > > 
> > > 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?
> > 
> > It's already on today. It just isn't *true* because QEMU never called
> > kvm_enable_x2apic().
> 
> 
> property should be on, but not by setting in kvm_default_prop / applied via 
> kvm_default_prop, that mechanism is for the versioned cpu models,
> which use X86CPUModel / X86CPUDefinition , and "host" isn't one of them.
> 
> Out of curiosity, does my previous snippet actually work? Not that I am sure 
> it is the best solution,
> just for my understanding. It would be surprising to me that the need to 
> actually manually apply "kvm-msi-ext-dest-id" to "on" there.
> 

This one?

--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -161,14 +161,14 @@ static void kvm_cpu_instance_init(CPUState *cs)
 
     host_cpu_instance_init(cpu);
 
-    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_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);
     }

Note that in today's HEAD we already advertise X2APIC and ext-dest-id
to the '-cpu host' guest; it's just not *true* because we never call
kvm_enable_x2apic().

So yes, the above works on a modern kernel where kvm_enable_x2apic()
succeeds. But that's the easy case.

Where your snippet *won't* work is in the case of running on an old
kernel where kvm_enable_x2apic() fails.

In that case it needs to turn x2apic support *off*. But simply calling
(or not calling) x86_cpu_change_kvm_default() makes absolutely no
difference unless those defaults are *applied* by calling
x86_cpu_apply_props() or making the same change by some other means.


> > So what I care about (in case ∃ APIC IDs >= 255) is two things:
> > 
> >  1. Qemu needs to call kvm_enable_x2apic().
> >  2. If that *fails* qemu needs to *stop* advertising X2APIC and ext-dest-id.
> > 
> > 
> > That last patch snippet in pc_machine_done() should suffice to achieve
> > that, I think. Because if kvm_enable_x2apic() fails and qemu has been
> > asked for that many CPUs, it aborts completely. Which seems right.
> > 
> 
> seems right to abort if requesting > 255 APIC IDs cannot be satisfied, I 
> agree.
> 
> So I think in the end, we want to:
> 
> 1) make sure that when accel=kvm and smp > 255 for i386, using cpu "host", 
> kvm_enable_x2apic() is called and successful.
> 
> 2) in addressing requirement 1), we do not break something else (other 
> machines, other cpu classes/models, TCG, ...).
> 
> 3) as a plus we might want to cleanup and determine once and for all where 
> kvm_enable_x2apic() should be called:
>    we have calls in intel_iommu.c and in the kvm cpu class instance 
> initialization here in kvm-cpu.c today:
>    before adding a third call we should really ask ourselves where the proper 
> initialization of this should happen.
> 

I think the existing two calls to kvm_enable_x2apic() become mostly
redundant. Because in fact the vtd_decide_config() and
kvm_cpu_instance_init() callers would both by perfectly OK without
kvm_enable_x2apic() if there isn't a CPU with an APIC ID >= 255
anyway. 

And that means that with my patch, pc_machine_done() will have
*aborted* if their conditions aren't met.

But then again, if since kvm_enable_x2apic() is both the initial
initialisation *and* a cached sanity check that it has indeed been
enabled successfully, there perhaps isn't any *harm* in having them do
the check for themselves?

smime.p7s
Description: S/MIME cryptographic signature