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

[Claudio Fontana]

On 11/29/21 9:29 PM, David Woodhouse wrote:
> 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().

This is clear to me. The move of the code there is simply to:

1) make sure that, for non-host, versioned cpu models, we continue to set the 
kvm_default_props and apply them in the same way.

2) for the host cpu, make sure that kvm_enable_x2apic() is called. 
x86_cpu_change_kvm_default is completely irrelevant for host cpu, as you have 
noted.

You are right that we continue to not handle the error path correctly on 
kvm_enable_x2apic failure, for both smp > 255 and <= 255.  

> 
> 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.


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.

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

> 
> 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