Re: [Qemu-devel] [PATCH v6 wave 2 2/3] hw/isa/lpc_ich9: add broadcast SMI feature

[Laszlo Ersek]

On 01/18/17 13:38, Igor Mammedov wrote:
> On Wed, 18 Jan 2017 11:23:48 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 01/18/17 11:19, Laszlo Ersek wrote:
>>> On 01/18/17 11:03, Igor Mammedov wrote:  
>>>> On Tue, 17 Jan 2017 19:53:21 +0100
>>>> Laszlo Ersek <address@hidden> wrote:
>>>>  
>>
>> [snip]
>>
>>>>> This is the code I wrote (extracting a new ich9_apm_broadcast_smi()
>>>>> function from ich9_apm_ctrl_changed(), due to size reasons):
>>>>>  
>>>>>> static void ich9_apm_broadcast_smi(void)
>>>>>> {
>>>>>>     CPUState *cs;
>>>>>>
>>>>>>     cpu_synchronize_all_states(); /* <--------- mark this */  
>>>> it probably should be executed after pause_all_vcpus(),
>>>> maybe it will help.
>>>>  
>>>>>>     CPU_FOREACH(cs) {
>>>>>>         X86CPU *cpu = X86_CPU(cs);
>>>>>>         CPUX86State *env = &cpu->env;
>>>>>>
>>>>>>         if (env->smbase == 0x30000 && env->eip == 0xfff0) {
>>>>>>             CPUClass *k = CPU_GET_CLASS(cs);
>>>>>>             uint64_t cpu_arch_id = k->get_arch_id(cs);
>>>>>>
>>>>>>             trace_ich9_apm_broadcast_smi_skip(cpu_arch_id);
>>>>>>             continue;
>>>>>>         }
>>>>>>
>>>>>>         cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>>>>     }
>>>>>> }    
>>>>>  
>>>> [...]  
>>>>> (b) If I add the cpu_synchronize_all_states() call before the loop, then
>>>>> the incorrect filter matches go away; QEMU sees the KVM VCPU states
>>>>> correctly, and the SMI is broad-cast.
>>>>>
>>>>> However, in this case, the boot slows to a *crawl*. It's unbearable. All
>>>>> VCPUs are pegged at 100%, and you can easily follow the OVMF debug log
>>>>> with the naked eye, almost line by line.
>>>>> I didn't expect that cpu_synchronize_all_states() would be so costly,
>>>>> but it makes the idea of checking VCPU state in the APM_CNT handler a
>>>>> non-starter.  
>>>> I wonder were bottleneck is to slow down guest so much.
>>>> What is actual cost of calling cpu_synchronize_all_states()?
>>>>
>>>> Maybe calling pause_all_vcpus() before cpu_synchronize_all_states()
>>>> would help.  
>>>
>>> If I prepend just a pause_all_vcpus() function call, at the top, then
>>> the VM freezes (quite understandably) when the first SMI is raised via
>>> APM_CNT.
>>>
>>> If I, instead, bracket the function body with pause_all_vcpus() and
>>> resume_all_vcpus(), like this:
>>>
>>> static void ich9_apm_broadcast_smi(void)
>>> {
>>>     CPUState *cs;
>>>
>>>     pause_all_vcpus();
>>>     cpu_synchronize_all_states();
>>>     CPU_FOREACH(cs) {
>>>         X86CPU *cpu = X86_CPU(cs);
>>>         CPUX86State *env = &cpu->env;
>>>
>>>         if (env->smbase == 0x30000 && env->eip == 0xfff0) {
>>>             CPUClass *k = CPU_GET_CLASS(cs);
>>>             uint64_t cpu_arch_id = k->get_arch_id(cs);
>>>
>>>             trace_ich9_apm_broadcast_smi_skip(cpu_arch_id);
>>>             continue;
>>>         }
>>>
>>>         cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>     }
>>>     resume_all_vcpus();
>>> }
>>>
>>> then I see the following symptoms:
>>> - rather than 4 VCPUs being pegged at 100%, only one VCPU is pegged at
>>>   100%
>>> - the boot process, as observed from the OVMF log, is just as slow
>>>   (= crawling) as without the VCPU pausing/resuming (i.e., like with
>>>   cpu_synchronize_all_states() only); so ultimately the
>>>   pausing/resuming doesn't help.  
>>
>> I also tried this variant (bracketing only the sync call with pause/resume):
>>
>> static void ich9_apm_broadcast_smi(void)
>> {
>>     CPUState *cs;
>>
>>     pause_all_vcpus();
>>     cpu_synchronize_all_states();
>>     resume_all_vcpus();
>>     CPU_FOREACH(cs) {
>>         X86CPU *cpu = X86_CPU(cs);
>>         CPUX86State *env = &cpu->env;
>>
>>         if (env->smbase == 0x30000 && env->eip == 0xfff0) {
>>             CPUClass *k = CPU_GET_CLASS(cs);
>>             uint64_t cpu_arch_id = k->get_arch_id(cs);
>>
>>             trace_ich9_apm_broadcast_smi_skip(cpu_arch_id);
>>             continue;
>>         }
>>
>>         cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>     }
>> }
>>
>> This behaves identically to the version where pause/resume bracket the
>> entire function body (i.e., 1 VCPU pegged, super-slow boot progress).
> wrapping entire function into pause_all_vcpus()/resume_all_vcpus()
> looks better as then cpu_interrupt() would not need to send IPIs to CPUs
> since pause_all_vcpus() would have done it.

I don't understand, why would a pause operation send IPIs?

Do you mean the resume? Even in that case, why would the resume send
precisely SMIs (substituting for the specific CPU_INTERRUPT_SMI calls)
and not some other kind of interrupt?

> So the left question is what this 1 CPU does to make things so slow.
> Does it send a lot of SMIs or something else?

The firmware uses many SMIs / Trigger() calls, for example for the
implementation of UEFI variable services, and SMM lockbox operations.
However, it does not use *masses* of them. Following the OVMF log, I see
that *individual* APM_CNT writes take very long (on the order of a
second, even), which implies that a single cpu_synchronize_all_states()
function call takes very long.

I briefly checked what cpu_synchronize_all_states() does, digging down
its call stack. I don't see anything in it that we should or could
modify for the sake of this work.


I think the current line of investigation is way out of scope for this
work, and that we've lost focus. In the original "PATCH v6 wave 2 2/3",
cpu_interrupt() just works as intended.

Why are we branching out this far from that? Just to avoid the CPU
unpark feature that Paolo suggested (which would also be negotiated)?

What is so wrong with the planned CPU unpark stuff that justifies
blocking this patch?

Honestly I don't understand why we can't approach these features
*incrementally*. Let's do the negotiable SMI broadcast now, then do VCPU
hotplug later. I think the current negotiation pattern is flexible and
future-proofed enough for that. It was you who suggested, for
simplifying the last patch in the series, that we completely ignore VCPU
hotplug for now (and I whole-heartedly agreed).

Let me put it like this: what is in this patch, in your opinion, that
*irrevocably* breaks the upcoming VCPU hotplug feature, or else limits
our options in implementing it? In my opinion, we can later implement
*anything at all*, dependent on new feature bits. We can even decide to
reject guest feature requests that specify *only* bit#0; which
practically means disabling guests (using SMIs) that don't conform to
our VCPU hotlpug implementation.

Laszlo