Received-SPF: pass (google.com: domain of linux-kernel+bounces-163533-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240430014505.2102631-1-maobibo@loongson.cn> <20240430014505.2102631-3-maobibo@loongson.cn>
In-Reply-To: <20240430014505.2102631-3-maobibo@loongson.cn>
From: Huacai Chen <chenhuacai@kernel.org>
Date: Tue, 30 Apr 2024 16:23:42 +0800
Message-ID: <CAAhV-H4xN9rdBkSA4PJafr5MNqEiCbNHTgim_bj89YNaB4PFjw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] LoongArch: Add steal time support in guest side
To: Bibo Mao <maobibo@loongson.cn>
Cc: Tianrui Zhao <zhaotianrui@loongson.cn>, Juergen Gross <jgross@suse.com>, kvm@vger.kernel.org, 
	loongarch@lists.linux.dev, linux-kernel@vger.kernel.org, x86@kernel.org, 
	virtualization@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi, Bibo,

On Tue, Apr 30, 2024 at 9:45=E2=80=AFAM Bibo Mao <maobibo@loongson.cn> wrot=
e:
>
> Percpu struct kvm_steal_time is added here, its size is 64 bytes and
> also defined as 64 bytes, so that the whole structure is in one physical
> page.
>
> When vcpu is onlined, function pv_enable_steal_time() is called. This
> function will pass guest physical address of struct kvm_steal_time and
> tells hypervisor to enable steal time. When vcpu is offline, physical
> address is set as 0 and tells hypervisor to disable steal time.
>
> Here is output of vmstat on guest when there is workload on both host
> and guest. It includes steal time stat information.
>
> procs -----------memory---------- -----io---- -system-- ------cpu-----
>  r  b   swpd   free  inact active   bi    bo   in   cs us sy id wa st
> 15  1      0 7583616 184112  72208    20    0  162   52 31  6 43  0 20
> 17  0      0 7583616 184704  72192    0     0 6318 6885  5 60  8  5 22
> 16  0      0 7583616 185392  72144    0     0 1766 1081  0 49  0  1 50
> 16  0      0 7583616 184816  72304    0     0 6300 6166  4 62 12  2 20
> 18  0      0 7583632 184480  72240    0     0 2814 1754  2 58  4  1 35
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/Kconfig                |  11 +++
>  arch/loongarch/include/asm/paravirt.h |   5 +
>  arch/loongarch/kernel/paravirt.c      | 131 ++++++++++++++++++++++++++
>  arch/loongarch/kernel/time.c          |   2 +
>  4 files changed, 149 insertions(+)
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index 0a1540a8853e..f3a03c33a052 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -592,6 +592,17 @@ config PARAVIRT
>           over full virtualization.  However, when run without a hypervis=
or
>           the kernel is theoretically slower and slightly larger.
>
> +config PARAVIRT_TIME_ACCOUNTING
> +       bool "Paravirtual steal time accounting"
> +       select PARAVIRT
> +       help
> +         Select this option to enable fine granularity task steal time
> +         accounting. Time spent executing other tasks in parallel with
> +         the current vCPU is discounted from the vCPU power. To account =
for
> +         that, there can be a small performance impact.
> +
> +         If in doubt, say N here.
> +
Can we use a hidden selection manner, which means:

config PARAVIRT_TIME_ACCOUNTING
       def_bool PARAVIRT

Because I think we needn't give too many choices to users (and bring
much more effort to test).

PowerPC even hide all the PARAVIRT config options...
see arch/powerpc/platforms/pseries/Kconfig

Huacai

>  config ARCH_SUPPORTS_KEXEC
>         def_bool y
>
> diff --git a/arch/loongarch/include/asm/paravirt.h b/arch/loongarch/inclu=
de/asm/paravirt.h
> index 58f7b7b89f2c..fe27fb5e82b8 100644
> --- a/arch/loongarch/include/asm/paravirt.h
> +++ b/arch/loongarch/include/asm/paravirt.h
> @@ -17,11 +17,16 @@ static inline u64 paravirt_steal_clock(int cpu)
>  }
>
>  int pv_ipi_init(void);
> +int __init pv_time_init(void);
>  #else
>  static inline int pv_ipi_init(void)
>  {
>         return 0;
>  }
>
> +static inline int pv_time_init(void)
> +{
> +       return 0;
> +}
>  #endif // CONFIG_PARAVIRT
>  #endif
> diff --git a/arch/loongarch/kernel/paravirt.c b/arch/loongarch/kernel/par=
avirt.c
> index 9044ed62045c..3f83afc7e2d2 100644
> --- a/arch/loongarch/kernel/paravirt.c
> +++ b/arch/loongarch/kernel/paravirt.c
> @@ -5,10 +5,13 @@
>  #include <linux/jump_label.h>
>  #include <linux/kvm_para.h>
>  #include <asm/paravirt.h>
> +#include <linux/reboot.h>
>  #include <linux/static_call.h>
>
>  struct static_key paravirt_steal_enabled;
>  struct static_key paravirt_steal_rq_enabled;
> +static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
> +static int has_steal_clock;
>
>  static u64 native_steal_clock(int cpu)
>  {
> @@ -17,6 +20,57 @@ static u64 native_steal_clock(int cpu)
>
>  DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock);
>
> +static bool steal_acc =3D true;
> +static int __init parse_no_stealacc(char *arg)
> +{
> +       steal_acc =3D false;
> +       return 0;
> +}
> +early_param("no-steal-acc", parse_no_stealacc);
> +
> +static u64 para_steal_clock(int cpu)
> +{
> +       u64 steal;
> +       struct kvm_steal_time *src;
> +       int version;
> +
> +       src =3D &per_cpu(steal_time, cpu);
> +       do {
> +
> +               version =3D src->version;
> +               /* Make sure that the version is read before the steal */
> +               virt_rmb();
> +               steal =3D src->steal;
> +               /* Make sure that the steal is read before the next versi=
on */
> +               virt_rmb();
> +
> +       } while ((version & 1) || (version !=3D src->version));
> +       return steal;
> +}
> +
> +static int pv_enable_steal_time(void)
> +{
> +       int cpu =3D smp_processor_id();
> +       struct kvm_steal_time *st;
> +       unsigned long addr;
> +
> +       if (!has_steal_clock)
> +               return -EPERM;
> +
> +       st =3D &per_cpu(steal_time, cpu);
> +       addr =3D per_cpu_ptr_to_phys(st);
> +
> +       /* The whole structure kvm_steal_time should be one page */
> +       if (PFN_DOWN(addr) !=3D PFN_DOWN(addr + sizeof(*st))) {
> +               pr_warn("Illegal PV steal time addr %lx\n", addr);
> +               return -EFAULT;
> +       }
> +
> +       addr |=3D KVM_STEAL_PHYS_VALID;
> +       kvm_hypercall2(KVM_HCALL_FUNC_NOTIFY, KVM_FEATURE_STEAL_TIME, add=
r);
> +       return 0;
> +}
> +
>  #ifdef CONFIG_SMP
>  static void pv_send_ipi_single(int cpu, unsigned int action)
>  {
> @@ -110,6 +164,32 @@ static void pv_init_ipi(void)
>         if (r < 0)
>                 panic("SWI0 IRQ request failed\n");
>  }
> +
> +static void pv_disable_steal_time(void)
> +{
> +       if (has_steal_clock)
> +               kvm_hypercall2(KVM_HCALL_FUNC_NOTIFY, KVM_FEATURE_STEAL_T=
IME, 0);
> +}
> +
> +static int pv_time_cpu_online(unsigned int cpu)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +       pv_enable_steal_time();
> +       local_irq_restore(flags);
> +       return 0;
> +}
> +
> +static int pv_time_cpu_down_prepare(unsigned int cpu)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +       pv_disable_steal_time();
> +       local_irq_restore(flags);
> +       return 0;
> +}
>  #endif
>
>  static bool kvm_para_available(void)
> @@ -149,3 +229,54 @@ int __init pv_ipi_init(void)
>
>         return 1;
>  }
> +
> +static void pv_cpu_reboot(void *unused)
> +{
> +       pv_disable_steal_time();
> +}
> +
> +static int pv_reboot_notify(struct notifier_block *nb, unsigned long cod=
e,
> +               void *unused)
> +{
> +       on_each_cpu(pv_cpu_reboot, NULL, 1);
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pv_reboot_nb =3D {
> +       .notifier_call  =3D pv_reboot_notify,
> +};
> +
> +int __init pv_time_init(void)
> +{
> +       int feature;
> +
> +       if (!cpu_has_hypervisor)
> +               return 0;
> +       if (!kvm_para_available())
> +               return 0;
> +
> +       feature =3D read_cpucfg(CPUCFG_KVM_FEATURE);
> +       if (!(feature & KVM_FEATURE_STEAL_TIME))
> +               return 0;
> +
> +       has_steal_clock =3D 1;
> +       if (pv_enable_steal_time()) {
> +               has_steal_clock =3D 0;
> +               return 0;
> +       }
> +
> +       register_reboot_notifier(&pv_reboot_nb);
> +       static_call_update(pv_steal_clock, para_steal_clock);
> +       static_key_slow_inc(&paravirt_steal_enabled);
> +       if (steal_acc)
> +               static_key_slow_inc(&paravirt_steal_rq_enabled);
> +
> +#ifdef CONFIG_SMP
> +       if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +                       "loongarch/pvi_time:online",
> +                       pv_time_cpu_online, pv_time_cpu_down_prepare) < 0=
)
> +               pr_err("Failed to install cpu hotplug callbacks\n");
> +#endif
> +       pr_info("Using stolen time PV\n");
> +       return 0;
> +}
> diff --git a/arch/loongarch/kernel/time.c b/arch/loongarch/kernel/time.c
> index fd5354f9be7c..46d7d40c87e3 100644
> --- a/arch/loongarch/kernel/time.c
> +++ b/arch/loongarch/kernel/time.c
> @@ -15,6 +15,7 @@
>
>  #include <asm/cpu-features.h>
>  #include <asm/loongarch.h>
> +#include <asm/paravirt.h>
>  #include <asm/time.h>
>
>  u64 cpu_clock_freq;
> @@ -214,4 +215,5 @@ void __init time_init(void)
>
>         constant_clockevent_init();
>         constant_clocksource_init();
> +       pv_time_init();
>  }
> --
> 2.39.3
>
>