Message-ID: <5c338084b1bcccc1d57dce9ddb1e7081@aosc.io>
Date: Wed, 31 Jul 2024 17:14:47 +0800
From: Mingcong Bai <jeffbai@...c.io>
To: WangYuli <wangyuli@...ontech.com>
Cc: pbonzini@...hat.com, corbet@....net, zhaotianrui@...ngson.cn,
 maobibo@...ngson.cn, chenhuacai@...nel.org, kernel@...0n.name,
 kvm@...r.kernel.org, loongarch@...ts.linux.dev, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, guanwentao@...ontech.com, Xianglai Li
 <lixianglai@...ngson.cn>
Subject: Re: [PATCH] Loongarch: KVM: Add KVM hypercalls documentation for
 LoongArch

Hi Yuli,

Thanks for submitting this documentation. I have just a couple 
suggestions on prose and grammar. Please see below.

在 2024-07-31 13:57，WangYuli 写道：
> From: Bibo Mao <maobibo@...ngson.cn>
> 
> Add documentation topic for using pv_virt when running as a guest
> on KVM hypervisor.
> 
> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
> Signed-off-by: Xianglai Li <lixianglai@...ngson.cn>
> Signed-off-by: WangYuli <wangyuli@...ontech.com>
> ---
>  Documentation/virt/kvm/index.rst              |  1 +
>  .../virt/kvm/loongarch/hypercalls.rst         | 79 +++++++++++++++++++
>  Documentation/virt/kvm/loongarch/index.rst    | 10 +++
>  MAINTAINERS                                   |  1 +
>  4 files changed, 91 insertions(+)
>  create mode 100644 Documentation/virt/kvm/loongarch/hypercalls.rst
>  create mode 100644 Documentation/virt/kvm/loongarch/index.rst
> 
> diff --git a/Documentation/virt/kvm/index.rst 
> b/Documentation/virt/kvm/index.rst
> index ad13ec55ddfe..9ca5a45c2140 100644
> --- a/Documentation/virt/kvm/index.rst
> +++ b/Documentation/virt/kvm/index.rst
> @@ -14,6 +14,7 @@ KVM
>     s390/index
>     ppc-pv
>     x86/index
> +   loongarch/index
> 
>     locking
>     vcpu-requests
> diff --git a/Documentation/virt/kvm/loongarch/hypercalls.rst 
> b/Documentation/virt/kvm/loongarch/hypercalls.rst
> new file mode 100644
> index 000000000000..1679e48d67d2
> --- /dev/null
> +++ b/Documentation/virt/kvm/loongarch/hypercalls.rst
> @@ -0,0 +1,79 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===================================
> +The LoongArch paravirtual interface
> +===================================
> +
> +KVM hypercalls use the HVCL instruction with code 0x100, and the 
> hypercall
> +number is put in a0 and up to five arguments may be placed in a1-a5, 
> the
> +return value is placed in v0 (alias with a0).

The original paragraph can be split into a few sentences for better 
readability:

"KVM hypercalls use the HVCL instruction with code 0x100 and the 
hypercall number is put in a0. Up to five arguments may be placed in 
registers a1 - a5. The return value is placed in v0 (an alias of a0)."

Not sure if "HVCL instruction with code 0x100" is a proper expression, 
so I have left it alone (I'm a little suspicious). Others are welcome to 
comment on this.

> +
> +The code for that interface can be found in arch/loongarch/kvm/*

It would seem that this sentence (or rather the components and 
structures of this whole documentation?) was borrowed from 
Documentation/virt/kvm/ppc-pv.rst. But nevertheless:

"Source code for this interface can be found in arch/loongarch/kvm*."

> +
> +Querying for existence
> +======================
> +
> +To find out if we're running on KVM or not, cpucfg can be used with 
> index
> +CPUCFG_KVM_BASE (0x40000000), cpucfg range between 0x40000000 - 
> 0x400000FF
> +is marked as a specially reserved range. All existing and future 
> processors
> +will not implement any features in this range.

Again, please consider splitting up the sentences:

"We can use cpucfg() at index CPUCFG_KVM_BASE (0x40000000) to query 
whether the host is running on KVM. The CPUCPU_KVM_BASE range between 
0x40000000 - 0x400000FF is marked as reserved - all existing and future 
processors will not implement any features in this range."

> +
> +When Linux is running on KVM, cpucfg with index CPUCFG_KVM_BASE 
> (0x40000000)
> +returns magic string "KVM\0"

When the Linux host is running on KVM, a read on cpucfg() at index 
CPUCFG_KVM_BASE (0x40000000) returns a magic string "KVM\0".

> +
> +Once you determined you're running under a PV capable KVM, you can now 
> use
> +hypercalls as described below.

Once you have determined that your host is running on a 
paravirtualization-capable KVM, you may now use hypercalls as described 
below.

> +
> +KVM hypercall ABI
> +=================
> +
> +Hypercall ABI on KVM is simple, only one scratch register a0 (v0) and 
> at most
> +five generic registers used as input parameter. FP register and vector 
> register
> +is not used for input register and should not be modified during 
> hypercall.
> +Hypercall function can be inlined since there is only one scratch 
> register.

"The KVM hypercall ABI is simple, with one scratch register a0 (v0) and 
at most five generic registers (a1 - a5) used as input parameters. The 
FP and vector registers are not used as input registers and should not 
be modified in a hypercall. Hypercall functions can be inlined, as there 
is only one scratch register."

It is recommended to define what the "The FP and vector registers" are 
with parentheses.

> +
> +The parameters are as follows:
> +
> +        ========	================	================
> +	Register	IN			OUT
> +        ========	================	================
> +	a0		function number		Return code

Function index?

> +	a1		1st parameter		-
> +	a2		2nd parameter		-
> +	a3		3rd parameter		-
> +	a4		4th parameter		-
> +	a5		5th parameter		-
> +        ========	================	================
> +
> +Return codes can be as follows:

The return codes may be one of the following:

> +
> +	====		=========================
> +	Code		Meaning
> +	====		=========================
> +	0		Success
> +	-1		Hypercall not implemented
> +	-2		Hypercall parameter error

Bad hypercall parameter

> +	====		=========================
> +
> +KVM Hypercalls Documentation
> +============================
> +
> +The template for each hypercall is:

"The template for each hypercall is as follows:"

Also, please consider adding a blank line here (though it probably 
doesn't matter once rendered, but it would make the plain text more 
readable).

> +1. Hypercall name
> +2. Purpose
> +
> +1. KVM_HCALL_FUNC_PV_IPI
> +------------------------
> +
> +:Purpose: Send IPIs to multiple vCPUs.
> +
> +- a0: KVM_HCALL_FUNC_PV_IPI
> +- a1: lower part of the bitmap of destination physical CPUIDs

Lower part of the bitmap for destination physical CPUIDs.

> +- a2: higher part of the bitmap of destination physical CPUIDs

Ditto, please capitalize the first letter after ":" and use "for" before 
"destination physical CPUIDs".

> +- a3: the lowest physical CPUID in bitmap

The lowest physical CPUID in the bitmap.

> +
> +The hypercall lets a guest send multicast IPIs, with at most 128
> +destinations per hypercall.  The destinations are represented by a 
> bitmap
> +contained in the first two arguments (a1 and a2). Bit 0 of a1 
> corresponds
> +to the physical CPUID in the third argument (a3), bit 1 corresponds to 
> the
> +physical ID a3+1, and so on.

Cleaning up sentences and dropping inconsistent expressions (such as 
"argument registers", which was never brought up before). I would 
recommend making them all consistent throughout by classifying a0 and a1 
- a5.

The hypercall lets a guest send multiple IPIs (Inter-Process Interrupts) 
with at most 128 destinations per hypercall. The destinations are 
represented in a bitmap contained in the first two input registers (a1 
and a2). Bit 0 of a1 corresponds to the physical CPUID in the third 
input register (a3) and bit 1 corresponds to the physical CPUID in a3+1 
(a4), and so on.

> diff --git a/Documentation/virt/kvm/loongarch/index.rst 
> b/Documentation/virt/kvm/loongarch/index.rst
> new file mode 100644
> index 000000000000..83387b4c5345
> --- /dev/null
> +++ b/Documentation/virt/kvm/loongarch/index.rst
> @@ -0,0 +1,10 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +KVM for LoongArch systems
> +=========================
> +
> +.. toctree::
> +   :maxdepth: 2
> +
> +   hypercalls.rst
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 958e935449e5..8aa5d92b12ee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12073,6 +12073,7 @@ L:	kvm@...r.kernel.org
>  L:	loongarch@...ts.linux.dev
>  S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/virt/kvm/kvm.git
> +F:	Documentation/virt/kvm/loongarch/
>  F:	arch/loongarch/include/asm/kvm*
>  F:	arch/loongarch/include/uapi/asm/kvm*
>  F:	arch/loongarch/kvm/

Best Regards,
Mingcong Bai

Powered by blists - more mailing lists