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

[David Woodhouse]

On Tue, 2021-11-30 at 10:00 +0100, Claudio Fontana wrote:
> I tend to agree that what we want if kvm_enable_x2apic fails is to abort QEMU 
> if we have been requesting smp > 255,
> and if we did not request smp > 255 cpus, we want to not advertise the 
> feature.
> 
> This is not accomplished, neither by my snippet above, not by the existing 
> code at any point in git history, and not by yours in itself.
> 
> Your change seems to accomplish the call to kvm_enable_x2apic, and abort if 
> requested smp > 255,
> but it does not stop advertising X2APIC and ext-dest-id on kvm_enable_x2apic 
> failure for the case < 255, so we'd need to add that.

We don't need kvm_enable_x2apic() at all if all APIC IDs are <255.

The kvm_enable_x2apic() call sets two flags. The first (USE_32BIT_IDS)
makes KVM take the high bits of the target APIC ID from the high bits
of the MSI address. So is only relevant if we ever want to deliver
interrupts to CPUs above 255.

The second (DISABLE_BROADCAST_QUIRK) prevents APIC ID 255 from being
interpreted as a broadcast. This is relevant if you ever want to
deliver interrupts to APIC #255.

So we need kvm_enable_x2apic() *if* we have APICs >= 255, but we don't
care about it if we have fewer CPUs.

Note the condition is '>= 255'. Not '> 255'.

With APIC IDs < 256 it also doesn't matter whether we advertise the
ext-dest-id feature to the guest or not, since that only tells them
where to put the upper bits... and there aren't any upper bits.

> Do I understand it right? Do we need to wrap all of this logic in a if 
> (kvm_enabled()) ?

For Xen because we aren't really emulating CPUs or APICs at all, it
doesn't matter and you can have as many CPUs as you like.

For TCG or (KVM && !kvm_irqchip_in_kernel()) we are limited to 254
because our userspace lapic doesn't emulate x2apic at all. But in the
TCG case, even if KVM isn't *built*, kvm_irqchip_in_kernel() will
return false. So all we need to check is kvm_irqchip_in_kernel().

I think the correct check is what I had in pc_machine_done() with the
off-by-one error fixed:

    if (x86ms->apic_id_limit >= 255 && !xen_enabled() &&
        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {

But that doesn't help microvm unless we copy it there too. 

Let's take a look at the code we were already looking at in
kvm_cpu_instance_init():

        /* only applies to builtin_x86_defs cpus */
        if (!kvm_irqchip_in_kernel()) {
            x86_cpu_change_kvm_default("x2apic", "off");

That part is purely cosmetic because kvm_arch_get_supported_cpuid() is
*already* going to mask out the X2APIC bit if(!kvm_irqchip_in_kernel())
anyway.

        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
                x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
        }

That part makes sense but there's a check missing here because even
*without* ext-dest-id we can't support APIC ID 255 without using
kvm_enable_x2apic().

And by the time we're in kvm_cpu_instance_init() for the CPU with APIC
ID #255, it's too late to disable x2apic support for all the previous
CPUs.

So... we either do the check in pc_machine_done() and also a similar
check for microvm, or we actually abort in kvm_cpu_instance_init() if
this CPU's APIC ID is >=255 and kvm_enable_x2apic() has failed.

Either way, the call in intel_iommu can die.




> > 
> > 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()
> 
> right, it makes absolutely no difference, and we cannot use 
> kvm_default_props, as they are for something else entirely.
> 
> > or making the same change by some other means.
> 
> right, we need to change it by other means.
> 
> It is still unclear to me for which cpu classes and versioned models we 
> should behave like this. Thoughts?
> 
> "max"? "base"? versioned models: depending on the model features?
> 
> > 
> > 
> > > > 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.
> 
> Understand. We also need though:
> 
> 3. Not call kvm_enable_x2apic() when it should not be called (non-KVM 
> accelerator, which cpu classes and models)
> 4. Not stop advertising X2APIC and ext-dest-id when we shouldn't stop 
> advertising it.
> 
> > > > 
> > > > 
> > > > 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.
> 
> see comments above, and should we limit that code to when kvm is enabled?
> 
> > > > 
> > > 
> > > 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.
> 
> I think it is good to abort early if we figure out that the user request of 
> APIC ID >= 255 cannot be satisfied. 
> 
> > 
> > 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?
> > 
> 
> Well the harm in my mind is, do we need to handle the error condition 
> correctly at each single place? 
> Do we risk slightly different behavior and advertised features depending on 
> where the call happens?
> 
> Seems that we can reduce the complexity and long term risk by handling things 
> in one single place, if we definitely find what that place should be.
> 
> Thanks,
> 
> Claudio
>

smime.p7s
Description: S/MIME cryptographic signature