| 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: <47c9a8f1-0098-4543-ac98-e210ca6b0d34@intel.com>
Date: Mon, 30 Oct 2023 13:47:21 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Sean Christopherson <seanjc@...gle.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Jonathan Corbet <corbet@....net>,
Wanpeng Li <wanpengli@...cent.com>, x86@...nel.org,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] x86/kvm/async_pf: Use separate percpu variable to
track the enabling of asyncpf
On 10/25/2023 10:22 PM, Sean Christopherson wrote:
> On Wed, Oct 25, 2023, Vitaly Kuznetsov wrote:
>> Xiaoyao Li <xiaoyao.li@...el.com> writes:
>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>> index b8ab9ee5896c..388a3fdd3cad 100644
>>> --- a/arch/x86/kernel/kvm.c
>>> +++ b/arch/x86/kernel/kvm.c
>>> @@ -65,6 +65,7 @@ static int __init parse_no_stealacc(char *arg)
>>>
>>> early_param("no-steal-acc", parse_no_stealacc);
>>>
>>> +static DEFINE_PER_CPU_READ_MOSTLY(bool, async_pf_enabled);
>>
>> Would it make a difference is we replace this with a cpumask? I realize
>> that we need to access it on all CPUs from hotpaths but this mask will
>> rarely change so maybe there's no real perfomance hit?
>
> FWIW, I personally prefer per-CPU booleans from a readability perspective. I
> doubt there is a meaningful performance difference for a bitmap vs. individual
> booleans, the check is already gated by a static key, i.e. kernels that are NOT
> running as KVM guests don't care.
I agree with it.
> Actually, if there's performance gains to be had, optimizing kvm_read_and_reset_apf_flags()
> to read the "enabled" flag if and only if it's necessary is a more likely candidate.
> Assuming the host isn't being malicious/stupid, then apf_reason.flags will be '0'
> if PV async #PFs are disabled. The only question is whether or not apf_reason.flags
> is predictable enough for the CPU.
>
> Aha! In practice, the CPU already needs to resolve a branch based on apf_reason.flags,
> it's just "hidden" up in __kvm_handle_async_pf().
>
> If we really want to micro-optimize, provide an __always_inline inner helper so
> that __kvm_handle_async_pf() doesn't need to make a CALL just to read the flags.
> Then in the common case where a #PF isn't due to the host swapping out a page,
> the paravirt happy path doesn't need a taken branch and never reads the enabled
> variable. E.g. the below generates:
If this is wanted. It can be a separate patch, irrelevant with this
series, I think.
> 0xffffffff81939ed0 <+0>: 41 54 push %r12
> 0xffffffff81939ed2 <+2>: 31 c0 xor %eax,%eax
> 0xffffffff81939ed4 <+4>: 55 push %rbp
> 0xffffffff81939ed5 <+5>: 53 push %rbx
> 0xffffffff81939ed6 <+6>: 48 83 ec 08 sub $0x8,%rsp
> 0xffffffff81939eda <+10>: 65 8b 2d df 81 6f 7e mov %gs:0x7e6f81df(%rip),%ebp # 0x320c0 <apf_reason>
> 0xffffffff81939ee1 <+17>: 85 ed test %ebp,%ebp
> 0xffffffff81939ee3 <+19>: 75 09 jne 0xffffffff81939eee <__kvm_handle_async_pf+30>
> 0xffffffff81939ee5 <+21>: 48 83 c4 08 add $0x8,%rsp
> 0xffffffff81939ee9 <+25>: 5b pop %rbx
> 0xffffffff81939eea <+26>: 5d pop %rbp
> 0xffffffff81939eeb <+27>: 41 5c pop %r12
> 0xffffffff81939eed <+29>: c3 ret
>
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index b8ab9ee5896c..b24133dc0731 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -240,22 +240,29 @@ void kvm_async_pf_task_wake(u32 token)
> }
> EXPORT_SYMBOL_GPL(kvm_async_pf_task_wake);
>
> -noinstr u32 kvm_read_and_reset_apf_flags(void)
> +static __always_inline u32 __kvm_read_and_reset_apf_flags(void)
> {
> - u32 flags = 0;
> + u32 flags = __this_cpu_read(apf_reason.flags);
>
> - if (__this_cpu_read(apf_reason.enabled)) {
> - flags = __this_cpu_read(apf_reason.flags);
> - __this_cpu_write(apf_reason.flags, 0);
> + if (unlikely(flags)) {
> + if (likely(__this_cpu_read(apf_reason.enabled)))
> + __this_cpu_write(apf_reason.flags, 0);
> + else
> + flags = 0;
> }
>
> return flags;
> }
> +
> +u32 kvm_read_and_reset_apf_flags(void)
> +{
> + return __kvm_read_and_reset_apf_flags();
> +}
> EXPORT_SYMBOL_GPL(kvm_read_and_reset_apf_flags);
>
> noinstr bool __kvm_handle_async_pf(struct pt_regs *regs, u32 token)
> {
> - u32 flags = kvm_read_and_reset_apf_flags();
> + u32 flags = __kvm_read_and_reset_apf_flags();
> irqentry_state_t state;
>
> if (!flags)
>
>>> static DEFINE_PER_CPU_DECRYPTED(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
>>> DEFINE_PER_CPU_DECRYPTED(struct kvm_steal_time, steal_time) __aligned(64) __visible;
>>> static int has_steal_clock = 0;
>>> @@ -244,7 +245,7 @@ noinstr u32 kvm_read_and_reset_apf_flags(void)
>>> {
>>> u32 flags = 0;
>>>
>>> - if (__this_cpu_read(apf_reason.enabled)) {
>>> + if (__this_cpu_read(async_pf_enabled)) {
>>> flags = __this_cpu_read(apf_reason.flags);
>>> __this_cpu_write(apf_reason.flags, 0);
>>> }
>>> @@ -295,7 +296,7 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_asyncpf_interrupt)
>>>
>>> inc_irq_stat(irq_hv_callback_count);
>>>
>>> - if (__this_cpu_read(apf_reason.enabled)) {
>>> + if (__this_cpu_read(async_pf_enabled)) {
>>> token = __this_cpu_read(apf_reason.token);
>>> kvm_async_pf_task_wake(token);
>>> __this_cpu_write(apf_reason.token, 0);
>>> @@ -362,7 +363,7 @@ static void kvm_guest_cpu_init(void)
>>> wrmsrl(MSR_KVM_ASYNC_PF_INT, HYPERVISOR_CALLBACK_VECTOR);
>>>
>>> wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
>>> - __this_cpu_write(apf_reason.enabled, 1);
>>> + __this_cpu_write(async_pf_enabled, 1);
>>
>> As 'async_pf_enabled' is bool, it would probably be more natural to
>> write
>>
>> __this_cpu_write(async_pf_enabled, true);
>
> +1000
Powered by blists - more mailing lists