Re: [PATCH v3 0/5] Add support for the Idle HLT intercept feature

[Manali Shukla <manali.shukla@amd.com>]

Hi Paolo,

Thank you for reviewing my patches.

On 5/28/2024 3:52 PM, Paolo Bonzini wrote:
> On Tue, May 28, 2024 at 6:19 AM Manali Shukla <manali.shukla@amd.com> wrote:
>>
>> The upcoming new Idle HLT Intercept feature allows for the HLT
>> instruction execution by a vCPU to be intercepted by the hypervisor
>> only if there are no pending V_INTR and V_NMI events for the vCPU.
>> When the vCPU is expected to service the pending V_INTR and V_NMI
>> events, the Idle HLT intercept won’t trigger. The feature allows the
>> hypervisor to determine if the vCPU is actually idle and reduces
>> wasteful VMEXITs.
> 
> Does this have an effect on the number of vmexits for KVM, unless AVIC
> is enabled? Can you write a testcase for kvm-unit-tests' vmexit.flat
> that shows an improvement?

I have measured the total numbers of vmexits (using perf kvm stat report), while running the the test case I have
written for the idle halt intercept in vmexit.flat.


Without idle halt
-------------------------------------------------------------------------------------------------
|    Event name      |    Samples        |     Sample%      |      Time (ns)     |     Time%    |
-------------------------------------------------------------------------------------------------
|        msr         |      524213       |       49.00%     |      592573933     |    64.00 %   |
-------------------------------------------------------------------------------------------------
|        hlt         |      262080       |       24.00%     |      154429476     |    16.00%    |
-------------------------------------------------------------------------------------------------
|        vintr       |      262080       |       24.00%     |      163724208     |     16.00%   |
-------------------------------------------------------------------------------------------------

With idle halt

-----------------------------------------------------------------------------------------------
|    Event name      |     Samples       |     Sample%    |      Time (ns)     |     Time%    |
-----------------------------------------------------------------------------------------------  
|       msr          |      524213       |     66.00%     |      502011088     |    75.00 %   |
-----------------------------------------------------------------------------------------------
|      vintr         |      262080       |     33.00%     |      147515295     |    22.00%    |
-----------------------------------------------------------------------------------------------
|       io           |        1879       |      0.00%     |       8784729      |     1.00%    |
-----------------------------------------------------------------------------------------------


Below is data for the average of 10 runs of idle halt test case in vmexit.flat
----------------------------------------------------------------------------------
|  idle halt (on/off)  |   full test run    |   ipi test run   |  eoi test run   |
----------------------------------------------------------------------------------
|          on          |       5048 .4      |     1289.2       |    1140.6       |
----------------------------------------------------------------------------------
|          off         |       4806.1       |     1318.6       |    1165.8       |
----------------------------------------------------------------------------------

The "ipi test run" when the idle halt is enabled, takes less time (~2.28% )to finish as compared to
the  "ipi test run" when the idle halt is disabled. 

The "eoi test run" when the idle halt is enabled, takes less time (~2.20% )to finish as compared to
the  "eoi test run" when the idle halt is disabled. 

The "full test run" when the idle halt is enabled, takes more time (~5.4%) as compared to
the "full test run" when the idle halt is disabled.
(Seems a bit odd, I have not yet begun to investigate this behavior)

Snippet of the Test case:
+static void idle_hlt_test(void)
+{
+       x = 0;
+       cli();
+       apic_self_ipi(IPI_TEST_VECTOR);
+       safe_halt();
+       if (x != 1) printf("%d", x);
+}
+
 
> The reason I am wondering is because KVM does not really use V_INTR
> injection. The "idle HLT" intercept basically differs from the basic
> HLT trigger only in how it handles an STI;HLT sequence, as in that
> case the interrupt can be injected directly and the HLT vmexit is
> suppressed. But in that circumstance KVM would anyway use a V_INTR
> intercept to detect the opening of the interrupt injection window (and
> then the interrupt uses event injection rather than V_INTR). Again,
> this is only true if AVIC is disabled, but that is the default.
> 

I have taken traces to analyze it further.

With idle halt enabled
 220.696238: kvm_apic_ipi: dst b0 vec 176 (Fixed|physical|assert|edge|self)
 220.696238: kvm_apic_accept_irq: apicid 0 vec 176 (Fixed|edge)
 220.696238: kvm_apic: apic_write APIC_ICR = 0xb0000440b0
 220.696238: kvm_msr: msr_write 830 = 0xb0000440b0
 220.696238: kvm_entry: vcpu 0, rip 0x406a89
 220.696239: kvm_exit: vcpu 0 reason vintr rip 0x4004ae info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
 220.696239: kvm_inj_virq: IRQ 0xb0
 220.696240: kvm_entry: vcpu 0, rip 0x4004ae
 220.696240: kvm_exit: vcpu 0 reason msr rip 0x406a74 info1 0x0000000000000001 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
 220.696240: kvm_apic: apic_write APIC_EOI = 0x0
 220.696240: kvm_eoi: apicid 0 vector 176


Without idle halt enabled
 6204.951631: kvm_apic_ipi: dst b0 vec 176 (Fixed|physical|assert|edge|self)
 6204.951631: kvm_apic_accept_irq: apicid 0 vec 176 (Fixed|edge)
 6204.951631: kvm_apic: apic_write APIC_ICR = 0xb0000440b0
 6204.951631: kvm_msr: msr_write 830 = 0xb0000440b0
 6204.951631: kvm_entry: vcpu 0, rip 0x406a89
 6204.951632: kvm_exit: vcpu 0 reason hlt rip 0x4004ad info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
 6204.951632: kvm_entry: vcpu 0, rip 0x4004ae
 6204.951632: kvm_exit: vcpu 0 reason vintr rip 0x4004ae info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
 6204.951632: kvm_inj_virq: IRQ 0xb0
 6204.951632: kvm_entry: vcpu 0, rip 0x4004ae
 6204.951633: kvm_exit: vcpu 0 reason msr rip 0x406a74 info1 0x0000000000000001 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
 6204.951633: kvm_apic: apic_write APIC_EOI = 0x0
 6204.951633: kvm_eoi: apicid 0 vector 176

From the traces, it is seen that hlt exit is avoided while the idle halt intercept feature is enabled.

> So unless I'm wrong in my analysis above, I'm not sure this series,
> albeit small, is really worth it. As things stand, it would be more
> interesting to enable this for nested VMs, especially Hyper-V which
> does use V_INTR and V_TPL; even better, _emulating_ it on older
> processors would reduce the L2->L0->L1->L0->L2 path to a
> less-expensive L2->L0->L2 vmexit.
> 

I am yet to try to test the idle halt intercept feature on nested guest.

- Manali
> 
> Paolo
> 
>> Presence of the Idle HLT Intercept feature is indicated via CPUID
>> function Fn8000_000A_EDX[30].
>>
>> Document for the Idle HLT intercept feature is available at [1].
>>
>> [1]: AMD64 Architecture Programmer's Manual Pub. 24593, April 2024,
>>      Vol 2, 15.9 Instruction Intercepts (Table 15-7: IDLE_HLT).
>>      https://bugzilla.kernel.org/attachment.cgi?id=306250
>>
>> Testing Done:
>> - Added a selftest to test the Idle HLT intercept functionality.
>> - Compile and functionality testing for the Idle HLT intercept selftest
>>   are only done for x86_64.
>> - Tested SEV and SEV-ES guest for the Idle HLT intercept functionality.
>>
>> v2 -> v3
>> - Incorporated Andrew's suggestion to structure vcpu_stat_types in
>>   a way that each architecture can share the generic types and also
>>   provide its own.
>>
>> v1 -> v2
>> - Done changes in svm_idle_hlt_test based on the review comments from Sean.
>> - Added an enum based approach to get binary stats in vcpu_get_stat() which
>>   doesn't use string to get stat data based on the comments from Sean.
>> - Added self_halt() and cli() helpers based on the comments from Sean.
>>
>> Manali Shukla (5):
>>   x86/cpufeatures: Add CPUID feature bit for Idle HLT intercept
>>   KVM: SVM: Add Idle HLT intercept support
>>   KVM: selftests: Add safe_halt() and cli() helpers to common code
>>   KVM: selftests: Add an interface to read the data of named vcpu stat
>>   KVM: selftests: KVM: SVM: Add Idle HLT intercept test
>>
>>  arch/x86/include/asm/cpufeatures.h            |  1 +
>>  arch/x86/include/asm/svm.h                    |  1 +
>>  arch/x86/include/uapi/asm/svm.h               |  2 +
>>  arch/x86/kvm/svm/svm.c                        | 11 ++-
>>  tools/testing/selftests/kvm/Makefile          |  1 +
>>  .../testing/selftests/kvm/include/kvm_util.h  | 44 +++++++++
>>  .../kvm/include/x86_64/kvm_util_arch.h        | 40 +++++++++
>>  .../selftests/kvm/include/x86_64/processor.h  | 18 ++++
>>  tools/testing/selftests/kvm/lib/kvm_util.c    | 32 +++++++
>>  .../selftests/kvm/x86_64/svm_idle_hlt_test.c  | 89 +++++++++++++++++++
>>  10 files changed, 236 insertions(+), 3 deletions(-)
>>  create mode 100644 tools/testing/selftests/kvm/x86_64/svm_idle_hlt_test.c
>>
>>
>> base-commit: d91a9cc16417b8247213a0144a1f0fd61dc855dd
>> --
>> 2.34.1
>>
>