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

Manali Shukla
2024-06-04T12:23:29Z

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