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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 19 Feb 2024 16:48:38 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: maobibo <maobibo@...ngson.cn>
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 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(&paravirt_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.

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.

Huacai

>
> Regards
> Bibo Mao
> >
> > Huacai
> >
> >
> > Huacai
> >>   }
> >>
> >>   static void __init check_kernel_sections_mem(void)
> >> --
> >> 2.39.3
> >>
> >>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ