Re: [PATCH 4/5] LoongArch: Add paravirt interface for guest kernel

[maobibo <maobibo@xxxxxxxxxxx>]

On 2024/1/3 下午3:40, Jürgen Groß wrote:
> On 03.01.24 08:16, Bibo Mao wrote:
> > The patch add paravirt interface for guest kernel, it checks whether
> > system runs on VM mode. If it is, it will detect hypervisor type. And
> > returns true it is KVM hypervisor, else return false. Currently only
> > KVM hypervisor is supported, so there is only hypervisor detection
> > for KVM type.
>
> I guess you are talking of pv_guest_init() here? Or do you mean
> kvm_para_available()?
yes, it is pv_guest_init. It will be better if all hypervisor detection
is called in function pv_guest_init. Currently there is only kvm 
hypervisor, kvm_para_available is hard-coded in pv_guest_init here.

I can split file paravirt.c into paravirt.c and kvm.c, paravirt.c is 
used for hypervisor detection, and move code relative with pv_ipi into kvm.c

Regards
Bibo Mao
> > Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
> > ---
> >   arch/loongarch/Kconfig                        |  8 ++++
> >   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, 87 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 ee123820a476..940e5960d297 100644
> > --- a/arch/loongarch/Kconfig
> > +++ b/arch/loongarch/Kconfig
> > @@ -564,6 +564,14 @@ config CPU_HAS_PREFETCH
> >       bool
> >       default y
> > +config PARAVIRT
> > +    bool "Enable paravirtualization code"
> > +    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);
> > +}
> > +
> > +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);
>
> This is the 4th arch with the same definition of native_steal_clock() and
> pv_steal_clock. I think we should add a common file kernel/paravirt.c and
> move the related functions from the archs into the new file.
>
> If you don't want to do that I can prepare a series.
>
> > +
> > +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 d183a745fb85..fa680bdd0bd1 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>
> > @@ -376,6 +377,7 @@ void __init platform_init(void)
> >       pr_info("The BIOS Version: %s\n", b_info.bios_version);
> >       efi_runtime_init();
> > +    pv_guest_init();
>
> Any reason pv_guest_init() needs to return a value at all, seeing that 
> you don't
> use the returned value?
>
>
> Juergen