Received-SPF: pass (google.com: domain of linux-kernel+bounces-71062-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Subject: Re: [PATCH v4 4/6] LoongArch: Add paravirt interface for guest kernel
To: Huacai Chen <chenhuacai@kernel.org>
Cc: Tianrui Zhao <zhaotianrui@loongson.cn>, Juergen Gross <jgross@suse.com>,
 Paolo Bonzini <pbonzini@redhat.com>, loongarch@lists.linux.dev,
 linux-kernel@vger.kernel.org, virtualization@lists.linux.dev,
 kvm@vger.kernel.org
References: <20240201031950.3225626-1-maobibo@loongson.cn>
 <20240201031950.3225626-5-maobibo@loongson.cn>
 <CAAhV-H7dXsU+WM172PWi_m8TYpYmzG_SW-vVQcdnOdETUxQ9+w@mail.gmail.com>
 <63f8bd29-c0da-167b-187d-61c56eb081a6@loongson.cn>
 <CAAhV-H6HQHyu=0zyv6FVLRJTkOcmnkLk5h361yGd2igYnuMMng@mail.gmail.com>
 <4dcbd5b2-ba69-e254-b3bb-75a75e5f9215@loongson.cn>
 <CAAhV-H7UyOX6ifKzUdTQiPuueOwsBUO2Jhtfcb832Gc0DTpQKQ@mail.gmail.com>
From: maobibo <maobibo@loongson.cn>
Message-ID: <b6482ad7-c800-beb9-ac49-989f8a044132@loongson.cn>
Date: Mon, 19 Feb 2024 17:50:01 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAAhV-H7UyOX6ifKzUdTQiPuueOwsBUO2Jhtfcb832Gc0DTpQKQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit


On 2024/2/19 下午5:38, Huacai Chen wrote:
> On Mon, Feb 19, 2024 at 5:21 PM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/2/19 下午4:48, Huacai Chen wrote:
>>> On Mon, Feb 19, 2024 at 12:11 PM maobibo <maobibo@loongson.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@loongson.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@loongson.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.
>> 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.
> Then you want to add an init function for each feature, so please
> rename to pv_ipi_init(), move to the last patch and in
> loongson_smp_setup().
Sure, will change function name with pv_ipi_init() and move sentences 
calling this function to last patch.

Regards
Bibo Mao

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