| 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: <4dcbd5b2-ba69-e254-b3bb-75a75e5f9215@loongson.cn>
Date: Mon, 19 Feb 2024 17:21:15 +0800
From: maobibo <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Tianrui Zhao <zhaotianrui@...ngson.cn>, Juergen Gross <jgross@...e.com>,
Paolo Bonzini <pbonzini@...hat.com>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org, virtualization@...ts.linux.dev,
kvm@...r.kernel.org
Subject: Re: [PATCH v4 4/6] LoongArch: Add paravirt interface for guest kernel
On 2024/2/19 下午4:48, Huacai Chen wrote:
> On Mon, Feb 19, 2024 at 12:11 PM maobibo <maobibo@...ngson.cn> wrote:
>>
>>
>>
>> On 2024/2/19 上午10:42, Huacai Chen wrote:
>>> Hi, Bibo,
>>>
>>> On Thu, Feb 1, 2024 at 11:19 AM Bibo Mao <maobibo@...ngson.cn> wrote:
>>>>
>>>> The patch adds paravirt interface for guest kernel, function
>>>> pv_guest_initi() firstly checks whether system runs on VM mode. If kernel
>>>> runs on VM mode, it will call function kvm_para_available() to detect
>>>> whether current VMM is KVM hypervisor. And the paravirt function can work
>>>> only if current VMM is KVM hypervisor, since there is only KVM hypervisor
>>>> supported on LoongArch now.
>>>>
>>>> This patch only adds paravirt interface for guest kernel, however there
>>>> is not effective pv functions added here.
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
>>>> ---
>>>> arch/loongarch/Kconfig | 9 ++++
>>>> arch/loongarch/include/asm/kvm_para.h | 7 ++++
>>>> arch/loongarch/include/asm/paravirt.h | 27 ++++++++++++
>>>> .../include/asm/paravirt_api_clock.h | 1 +
>>>> arch/loongarch/kernel/Makefile | 1 +
>>>> arch/loongarch/kernel/paravirt.c | 41 +++++++++++++++++++
>>>> arch/loongarch/kernel/setup.c | 2 +
>>>> 7 files changed, 88 insertions(+)
>>>> create mode 100644 arch/loongarch/include/asm/paravirt.h
>>>> create mode 100644 arch/loongarch/include/asm/paravirt_api_clock.h
>>>> create mode 100644 arch/loongarch/kernel/paravirt.c
>>>>
>>>> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
>>>> index 10959e6c3583..817a56dff80f 100644
>>>> --- a/arch/loongarch/Kconfig
>>>> +++ b/arch/loongarch/Kconfig
>>>> @@ -585,6 +585,15 @@ config CPU_HAS_PREFETCH
>>>> bool
>>>> default y
>>>>
>>>> +config PARAVIRT
>>>> + bool "Enable paravirtualization code"
>>>> + depends on AS_HAS_LVZ_EXTENSION
>>>> + help
>>>> + This changes the kernel so it can modify itself when it is run
>>>> + under a hypervisor, potentially improving performance significantly
>>>> + over full virtualization. However, when run without a hypervisor
>>>> + the kernel is theoretically slower and slightly larger.
>>>> +
>>>> config ARCH_SUPPORTS_KEXEC
>>>> def_bool y
>>>>
>>>> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
>>>> index 9425d3b7e486..41200e922a82 100644
>>>> --- a/arch/loongarch/include/asm/kvm_para.h
>>>> +++ b/arch/loongarch/include/asm/kvm_para.h
>>>> @@ -2,6 +2,13 @@
>>>> #ifndef _ASM_LOONGARCH_KVM_PARA_H
>>>> #define _ASM_LOONGARCH_KVM_PARA_H
>>>>
>>>> +/*
>>>> + * Hypcall code field
>>>> + */
>>>> +#define HYPERVISOR_KVM 1
>>>> +#define HYPERVISOR_VENDOR_SHIFT 8
>>>> +#define HYPERCALL_CODE(vendor, code) ((vendor << HYPERVISOR_VENDOR_SHIFT) + code)
>>>> +
>>>> /*
>>>> * LoongArch hypcall return code
>>>> */
>>>> diff --git a/arch/loongarch/include/asm/paravirt.h b/arch/loongarch/include/asm/paravirt.h
>>>> new file mode 100644
>>>> index 000000000000..b64813592ba0
>>>> --- /dev/null
>>>> +++ b/arch/loongarch/include/asm/paravirt.h
>>>> @@ -0,0 +1,27 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef _ASM_LOONGARCH_PARAVIRT_H
>>>> +#define _ASM_LOONGARCH_PARAVIRT_H
>>>> +
>>>> +#ifdef CONFIG_PARAVIRT
>>>> +#include <linux/static_call_types.h>
>>>> +struct static_key;
>>>> +extern struct static_key paravirt_steal_enabled;
>>>> +extern struct static_key paravirt_steal_rq_enabled;
>>>> +
>>>> +u64 dummy_steal_clock(int cpu);
>>>> +DECLARE_STATIC_CALL(pv_steal_clock, dummy_steal_clock);
>>>> +
>>>> +static inline u64 paravirt_steal_clock(int cpu)
>>>> +{
>>>> + return static_call(pv_steal_clock)(cpu);
>>>> +}
>>> The steal time code can be removed in this patch, I think.
>>>
>> Originally I want to remove this piece of code, but it fails to compile
>> if CONFIG_PARAVIRT is selected. Here is reference code, function
>> paravirt_steal_clock() must be defined if CONFIG_PARAVIRT is selected.
>>
>> static __always_inline u64 steal_account_process_time(u64 maxtime)
>> {
>> #ifdef CONFIG_PARAVIRT
>> if (static_key_false(¶virt_steal_enabled)) {
>> u64 steal;
>>
>> steal = paravirt_steal_clock(smp_processor_id());
>> steal -= this_rq()->prev_steal_time;
>> steal = min(steal, maxtime);
>> account_steal_time(steal);
>> this_rq()->prev_steal_time += steal;
>>
>> return steal;
>> }
>> #endif
>> return 0;
>> }
> OK, then keep it.
>
>>
>>>> +
>>>> +int pv_guest_init(void);
>>>> +#else
>>>> +static inline int pv_guest_init(void)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#endif // CONFIG_PARAVIRT
>>>> +#endif
>>>> diff --git a/arch/loongarch/include/asm/paravirt_api_clock.h b/arch/loongarch/include/asm/paravirt_api_clock.h
>>>> new file mode 100644
>>>> index 000000000000..65ac7cee0dad
>>>> --- /dev/null
>>>> +++ b/arch/loongarch/include/asm/paravirt_api_clock.h
>>>> @@ -0,0 +1 @@
>>>> +#include <asm/paravirt.h>
>>>> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
>>>> index 3c808c680370..662e6e9de12d 100644
>>>> --- a/arch/loongarch/kernel/Makefile
>>>> +++ b/arch/loongarch/kernel/Makefile
>>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MODULES) += module.o module-sections.o
>>>> obj-$(CONFIG_STACKTRACE) += stacktrace.o
>>>>
>>>> obj-$(CONFIG_PROC_FS) += proc.o
>>>> +obj-$(CONFIG_PARAVIRT) += paravirt.o
>>>>
>>>> obj-$(CONFIG_SMP) += smp.o
>>>>
>>>> diff --git a/arch/loongarch/kernel/paravirt.c b/arch/loongarch/kernel/paravirt.c
>>>> new file mode 100644
>>>> index 000000000000..21d01d05791a
>>>> --- /dev/null
>>>> +++ b/arch/loongarch/kernel/paravirt.c
>>>> @@ -0,0 +1,41 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +#include <linux/export.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/jump_label.h>
>>>> +#include <linux/kvm_para.h>
>>>> +#include <asm/paravirt.h>
>>>> +#include <linux/static_call.h>
>>>> +
>>>> +struct static_key paravirt_steal_enabled;
>>>> +struct static_key paravirt_steal_rq_enabled;
>>>> +
>>>> +static u64 native_steal_clock(int cpu)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock);
>>> The steal time code can be removed in this patch, I think.
>> Ditto, the same reason with above.
>>>
>>>> +
>>>> +static bool kvm_para_available(void)
>>>> +{
>>>> + static int hypervisor_type;
>>>> + int config;
>>>> +
>>>> + if (!hypervisor_type) {
>>>> + config = read_cpucfg(CPUCFG_KVM_SIG);
>>>> + if (!memcmp(&config, KVM_SIGNATURE, 4))
>>>> + hypervisor_type = HYPERVISOR_KVM;
>>>> + }
>>>> +
>>>> + return hypervisor_type == HYPERVISOR_KVM;
>>>> +}
>>>> +
>>>> +int __init pv_guest_init(void)
>>>> +{
>>>> + if (!cpu_has_hypervisor)
>>>> + return 0;
>>>> + if (!kvm_para_available())
>>>> + return 0;
>>>> +
>>>> + return 1;
>>>> +}
>>>> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
>>>> index edf2bba80130..de5c36dccc49 100644
>>>> --- a/arch/loongarch/kernel/setup.c
>>>> +++ b/arch/loongarch/kernel/setup.c
>>>> @@ -43,6 +43,7 @@
>>>> #include <asm/efi.h>
>>>> #include <asm/loongson.h>
>>>> #include <asm/numa.h>
>>>> +#include <asm/paravirt.h>
>>>> #include <asm/pgalloc.h>
>>>> #include <asm/sections.h>
>>>> #include <asm/setup.h>
>>>> @@ -367,6 +368,7 @@ void __init platform_init(void)
>>>> pr_info("The BIOS Version: %s\n", b_info.bios_version);
>>>>
>>>> efi_runtime_init();
>>>> + pv_guest_init();
>>> I prefer use CONFIG_PARAVIRT here, though you have a dummy version for
>>> !CONFIG_PARAVIRT, I think it is better to let others clearly know that
>>> PARAVIRT is an optional feature.
>> I remember that there is rule that CONFIG_xxx had better be used in
>> header files rather than c code, so that the code looks neat. Am I wrong?
> That depends on what we want, sometimes we want to hide the details,
> but sometimes we want to give others a notice.
I want to keep code clean here :)
>
> And there is another problem: if you want to centralize all pv init
> functions, it is better to use pv_features_init() rather than
> pv_guest_init(); if you want to give each feature an init function,
> then we don't need pv_guest_init here, and we can then add a
> pv_ipi_init() in the last patch.
Currently I have no idea how to add other pv features like pv
stealtimer, I will consider this when adding other pv features.
pv_ipi_init/pv_guest_init is both ok for me, pv_ipi_init is better for now.
Regards
Bibo Mao
>
> Huacai
>
>>
>> Regards
>> Bibo Mao
>>>
>>> Huacai
>>>
>>>
>>> Huacai
>>>> }
>>>>
>>>> static void __init check_kernel_sections_mem(void)
>>>> --
>>>> 2.39.3
>>>>
>>>>
>>
Powered by blists - more mailing lists