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
