| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4a08ecd1-49e8-4fda-3614-276ab20f3b10@amperemail.onmicrosoft.com>
Date: Fri, 11 Aug 2023 15:52:40 +0800
From: Shijie Huang <shijie@...eremail.onmicrosoft.com>
To: Marc Zyngier <maz@...nel.org>
Cc: Huang Shijie <shijie@...amperecomputing.com>,
oliver.upton@...ux.dev, james.morse@....com,
suzuki.poulose@....com, yuzenghui@...wei.com,
catalin.marinas@....com, will@...nel.org,
linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.org, patches@...erecomputing.com,
zwang@...erecomputing.com, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v2] KVM/arm64: reconfigurate the event filters for guest
context
Hi Marc,
在 2023/8/11 15:42, Marc Zyngier 写道:
> On Fri, 11 Aug 2023 08:10:26 +0100,
> Shijie Huang <shijie@...eremail.onmicrosoft.com> wrote:
>> Hi Marc,
>>
>> 在 2023/8/11 14:10, Marc Zyngier 写道:
>>> On Fri, 11 Aug 2023 02:46:49 +0100,
>>> Shijie Huang <shijie@...eremail.onmicrosoft.com> wrote:
>>>> Hi Marc,
>>>>
>>>> 在 2023/8/10 23:27, Marc Zyngier 写道:
>>>>> Huang,
>>>>>
>>>>> Please make sure you add everyone who commented on v1 (I've Cc'd Mark
>>>>> so that he can shime need as needed).
>>>> thanks.
>>>>> On Thu, 10 Aug 2023 08:29:06 +0100,
>>>>> Huang Shijie <shijie@...amperecomputing.com> wrote:
>>>>>> 1.) Background.
>>>>>> 1.1) In arm64, start a guest with Qemu which is running as a VMM of KVM,
>>>>>> and bind the guest to core 33 and run program "a" in guest.
>>>>>> The code of "a" shows below:
>>>>>> ----------------------------------------------------------
>>>>>> #include <stdio.h>
>>>>>>
>>>>>> int main()
>>>>>> {
>>>>>> unsigned long i = 0;
>>>>>>
>>>>>> for (;;) {
>>>>>> i++;
>>>>>> }
>>>>>>
>>>>>> printf("i:%ld\n", i);
>>>>>> return 0;
>>>>>> }
>>>>>> ----------------------------------------------------------
>>>>>>
>>>>>> 1.2) Use the following perf command in host:
>>>>>> #perf stat -e cycles:G,cycles:H -C 33 -I 1000 sleep 1
>>>>>> # time counts unit events
>>>>>> 1.000817400 3,299,471,572 cycles:G
>>>>>> 1.000817400 3,240,586 cycles:H
>>>>>>
>>>>>> This result is correct, my cpu's frequency is 3.3G.
>>>>>>
>>>>>> 1.3) Use the following perf command in host:
>>>>>> #perf stat -e cycles:G,cycles:H -C 33 -d -d -I 1000 sleep 1
>>>>>> time counts unit events
>>>>>> 1.000831480 153,634,097 cycles:G (70.03%)
>>>>>> 1.000831480 3,147,940,599 cycles:H (70.03%)
>>>>>> 1.000831480 1,143,598,527 L1-dcache-loads (70.03%)
>>>>>> 1.000831480 9,986 L1-dcache-load-misses # 0.00% of all L1-dcache accesses (70.03%)
>>>>>> 1.000831480 <not supported> LLC-loads
>>>>>> 1.000831480 <not supported> LLC-load-misses
>>>>>> 1.000831480 580,887,696 L1-icache-loads (70.03%)
>>>>>> 1.000831480 77,855 L1-icache-load-misses # 0.01% of all L1-icache accesses (70.03%)
>>>>>> 1.000831480 6,112,224,612 dTLB-loads (70.03%)
>>>>>> 1.000831480 16,222 dTLB-load-misses # 0.00% of all dTLB cache accesses (69.94%)
>>>>>> 1.000831480 590,015,996 iTLB-loads (59.95%)
>>>>>> 1.000831480 505 iTLB-load-misses # 0.00% of all iTLB cache accesses (59.95%)
>>>>>>
>>>>>> This result is wrong. The "cycle:G" should be nearly 3.3G.
>>>>>>
>>>>>> 2.) Root cause.
>>>>>> There is only 7 counters in my arm64 platform:
>>>>>> (one cycle counter) + (6 normal counters)
>>>>>>
>>>>>> In 1.3 above, we will use 10 event counters.
>>>>>> Since we only have 7 counters, the perf core will trigger
>>>>>> multiplexing in hrtimer:
>>>>>> perf_mux_hrtimer_restart() --> perf_rotate_context().
>>>>>>
>>>>>> If the hrtimer occurs when the host is running, it's fine.
>>>>>> If the hrtimer occurs when the guest is running,
>>>>>> the perf_rotate_context() will program the PMU with filters for
>>>>>> host context. The KVM does not have a chance to restore
>>>>>> PMU registers with kvm_vcpu_pmu_restore_guest().
>>>>>> The PMU does not work correctly, so we got wrong result.
>>>>>>
>>>>>> 3.) About this patch.
>>>>>> Make a KVM_REQ_RELOAD_PMU request before reentering the
>>>>>> guest. The request will call kvm_vcpu_pmu_restore_guest()
>>>>>> to reconfigurate the filters for guest context.
>>>>>>
>>>>>> 4.) Test result of this patch:
>>>>>> #perf stat -e cycles:G,cycles:H -C 33 -d -d -I 1000 sleep 1
>>>>>> time counts unit events
>>>>>> 1.001006400 3,298,348,656 cycles:G (70.03%)
>>>>>> 1.001006400 3,144,532 cycles:H (70.03%)
>>>>>> 1.001006400 941,149 L1-dcache-loads (70.03%)
>>>>>> 1.001006400 17,937 L1-dcache-load-misses # 1.91% of all L1-dcache accesses (70.03%)
>>>>>> 1.001006400 <not supported> LLC-loads
>>>>>> 1.001006400 <not supported> LLC-load-misses
>>>>>> 1.001006400 1,101,889 L1-icache-loads (70.03%)
>>>>>> 1.001006400 121,638 L1-icache-load-misses # 11.04% of all L1-icache accesses (70.03%)
>>>>>> 1.001006400 1,031,228 dTLB-loads (70.03%)
>>>>>> 1.001006400 26,952 dTLB-load-misses # 2.61% of all dTLB cache accesses (69.93%)
>>>>>> 1.001006400 1,030,678 iTLB-loads (59.94%)
>>>>>> 1.001006400 338 iTLB-load-misses # 0.03% of all iTLB cache accesses (59.94%)
>>>>>>
>>>>>> The result is correct. The "cycle:G" is nearly 3.3G now.
>>>>>>
>>>>>> Signed-off-by: Huang Shijie <shijie@...amperecomputing.com>
>>>>>> ---
>>>>>> v1 --> v2:
>>>>>> Do not change perf/core code, only change the ARM64 kvm code.
>>>>>> v1: https://lkml.org/lkml/2023/8/8/1465
>>>>>>
>>>>>> ---
>>>>>> arch/arm64/kvm/arm.c | 11 ++++++++++-
>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>>>>> index c2c14059f6a8..475a2f0e0e40 100644
>>>>>> --- a/arch/arm64/kvm/arm.c
>>>>>> +++ b/arch/arm64/kvm/arm.c
>>>>>> @@ -919,8 +919,17 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>>>>>> if (!ret)
>>>>>> ret = 1;
>>>>>> - if (ret > 0)
>>>>>> + if (ret > 0) {
>>>>>> + /*
>>>>>> + * The perf_rotate_context() may rotate the events and
>>>>>> + * reprogram PMU with filters for host context.
>>>>>> + * So make a request before reentering the guest to
>>>>>> + * reconfigurate the event filters for guest context.
>>>>>> + */
>>>>>> + kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
>>>>>> +
>>>>>> ret = check_vcpu_requests(vcpu);
>>>>>> + }
>>>>> This looks extremely heavy handed. You're performing the reload on
>>>>> *every* entry, and I don't think this is right (exit-heavy workloads
>>>>> will suffer from it)
>>>>>
>>>>> Furthermore, you're also reloading the virtual state of the PMU
>>>>> (recreating guest events and other things), all of which looks pretty
>>>>> pointless, as all we're interested in is what is being counted on the
>>>>> *host*.
>>>> okay. What about to add a _new_ request, such as KVM_REQ_RESTROE_PMU_GUEST?
>>>>
>>>>
>>>>> Instead, we can restrict the reload of the host state (and only that)
>>>>> to situations where:
>>>>>
>>>>> - we're running on a VHE system
>>>>>
>>>>> - we have a host PMUv3 (not everybody does), as that's the only way we
>>>>> can profile a guest
>>>> okay. No problem.
>>>>
>>>>
>>>>> and ideally we would have a way to detect that a rotation happened
>>>>> (which may requires some help from the low-level PMU code).
>>>> I will check it, hope we can find a better way.
>>> I came up with the following patch, completely untested. Let me know
>>> how that fares for you.
>>>
>>> Thanks,
>>>
>>> M.
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index 93c541111dea..fb875c5c0347 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -49,6 +49,7 @@
>>> #define KVM_REQ_RELOAD_GICv4 KVM_ARCH_REQ(4)
>>> #define KVM_REQ_RELOAD_PMU KVM_ARCH_REQ(5)
>>> #define KVM_REQ_SUSPEND KVM_ARCH_REQ(6)
>>> +#define KVM_REQ_RELOAD_GUEST_PMU_EVENTS KVM_ARCH_REQ(7)
>>> #define KVM_DIRTY_LOG_MANUAL_CAPS
>>> (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
>>> KVM_DIRTY_LOG_INITIALLY_SET)
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index 8b51570a76f8..b40db24f1f0b 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -804,6 +804,9 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
>>> kvm_pmu_handle_pmcr(vcpu,
>>> __vcpu_sys_reg(vcpu, PMCR_EL0));
>>> + if (kvm_check_request(KVM_REQ_RELOAD_GUEST_PMU_EVENTS,
>>> vcpu))
>>> + kvm_vcpu_pmu_restore_guest(vcpu);
>>> +
>>> if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
>>> return kvm_vcpu_suspend(vcpu);
>>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>>> index 08b3a1bf0ef6..7012de417092 100644
>>> --- a/drivers/perf/arm_pmuv3.c
>>> +++ b/drivers/perf/arm_pmuv3.c
>>> @@ -772,6 +772,9 @@ static void armv8pmu_start(struct arm_pmu *cpu_pmu)
>>> /* Enable all counters */
>>> armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E);
>>> +
>>> + if (in_interrupt())
>>> + kvm_resync_guest_context();
>> I currently added a similiar check in armv8pmu_get_event_idx().
>>
>> The event multiplexing will call armv8pmu_get_event_idx(), and will
>> definitely fail at least one time.
>>
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -882,6 +882,8 @@ static int armv8pmu_get_event_idx(struct
>> pmu_hw_events *cpuc,
>> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>> struct hw_perf_event *hwc = &event->hw;
>> unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
>> + struct kvm_vcpu *vcpu;
>> + int index;
>> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>> struct hw_perf_event *hwc = &event->hw;
>> unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
>> + struct kvm_vcpu *vcpu;
>> + int index;
>>
>> /* Always prefer to place a cycle counter into the cycle
>> counter. */
>> if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
>> @@ -897,9 +899,22 @@ static int armv8pmu_get_event_idx(struct
>> pmu_hw_events *cpuc,
>> * Otherwise use events counters
>> */
>> if (armv8pmu_event_is_chained(event))
>> - return armv8pmu_get_chain_idx(cpuc, cpu_pmu);
>> + index = armv8pmu_get_chain_idx(cpuc, cpu_pmu);
>> else
>> - return armv8pmu_get_single_idx(cpuc, cpu_pmu);
>> + index = armv8pmu_get_single_idx(cpuc, cpu_pmu);
>> +
>> + /*
>> + * If we are in pmu multiplexing, we will definitely meet a failure.
>> + * Please see perf_rotate_context().
>> + * If we are in the guest context, we can mark it.
>> + */
>> + if (index < 0) {
>> + vcpu = kvm_get_running_vcpu();
>> + if (vcpu && in_interrupt() && !event->attr.pinned) {
>> + kvm_resync_guest_context();
xxxx.
>> + }
>> + }
>> + return index;
>> }
>>
>> IMHO, it's better to change armv8pmu_get_event_idx().
>>
>> But if you think it is also okay to change armv8pmu_start() to fix the bug,
>>
>> I am okay too.
> But that's doing work each time you rotate an event. And if you rotate
> a bunch of them, you'll hit this path multiple times, reloading the
> stuff again. What's the point?
In my code, I just put the kvm_make_request() in "xxx" above. Event
reloading it multiple times,
it just set a bit in vcpu->requests.
>
> My take is that we can hook at the point where the PMU gets
> re-enabled, and have the full context once and for all.
>
> Unless of course I miss something, which is very likely as the whole
> perf subsystem generally escapes me altogether.
>
> In any case, I'd welcome your testing the proposed patch.
No problem.
Thanks
Huang Shijie
>
> Thanks,
>
> M.
>
Powered by blists - more mailing lists