Message-ID: <29999cfc-6ec1-d881-277a-19f51f5c7b96@loongson.cn>
Date: Wed, 28 Aug 2024 09:07:13 +0800
From: maobibo <maobibo@...ngson.cn>
To: Zenghui Yu <zenghui.yu@...ux.dev>,
 Dandan Zhang <zhangdandan@...ontech.com>
Cc: pbonzini@...hat.com, corbet@....net, zhaotianrui@...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,
 wangyuli@...ontech.com, baimingcong@...ontech.com,
 Xianglai Li <lixianglai@...ngson.cn>, Mingcong Bai <jeffbai@...c.io>
Subject: Re: [PATCH v2] Loongarch: KVM: Add KVM hypercalls documentation for
 LoongArch

Zenghui,

On 2024/8/27 上午1:00, Zenghui Yu wrote:
> [ Trivial comments inline.  You can feel free to ignore them since I
>    know almost nothing about loongarch. ]
Thanks for reviewing the hypercall document, we all know that you have 
strong background knowledge with both kernel and architecture.

> 
> On 2024/8/26 13:47, Dandan Zhang wrote:
>> 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>
>> Co-developed-by: Mingcong Bai <jeffbai@...c.io>
>> Signed-off-by: Mingcong Bai <jeffbai@...c.io>
>> Link: https://lore.kernel.org/all/5c338084b1bcccc1d57dce9ddb1e7081@aosc.io/
>> Signed-off-by: Dandan Zhang <zhangdandan@...ontech.com>
>> ---
>>   Documentation/virt/kvm/index.rst              |  1 +
>>   .../virt/kvm/loongarch/hypercalls.rst         | 86 +++++++++++++++++++
>>   Documentation/virt/kvm/loongarch/index.rst    | 10 +++
>>   MAINTAINERS                                   |  1 +
>>   4 files changed, 98 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..58168dc7166c
>> --- /dev/null
>> +++ b/Documentation/virt/kvm/loongarch/hypercalls.rst
>> @@ -0,0 +1,86 @@
>> +.. 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. Up to five arguments may be placed in registers a1 - a5.
>> +The return value is placed in v0 (an alias of a0).
>> +
>> +Source code for this interface can be found in arch/loongarch/kvm*.
>> +
>> +Querying for existence
>> +======================
>> +
>> +To determine if the host is running on KVM, we can utilize the cpucfg()
>> +function at index CPUCFG_KVM_BASE (0x40000000).
>> +
>> +The CPUCPU_KVM_BASE range, spanning from 0x40000000 to 0x400000FF, The
>> +CPUCPU_KVM_BASE range between 0x40000000 - 0x400000FF is marked as reserved.
> 
> What is CPUCPU_KVM_BASE? Grepping it in the code shows nothing.
> 
>> +Consequently, all current and future processors will not implement any
>> +feature within this range.
>> +
>> +On a KVM-virtualized Linux system, a read operation on cpucfg() at index
>> +CPUCFG_KVM_BASE (0x40000000) returns the magic string 'KVM\0'.
>> +
>> +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
>> +=================
>> +
>> +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 (Floating-
>> +point) and vector registers are not utilized as input registers and must
>> +remain unmodified during a hypercall.
>> +
>> +Hypercall functions can be inlined as it only uses one scratch register.
>> +
>> +The parameters are as follows:
>> +
>> +        ========	================	================
>> +	Register	IN			OUT
>> +        ========	================	================
>> +	a0		function number		Return code
>> +	a1		1st parameter		-
>> +	a2		2nd parameter		-
>> +	a3		3rd parameter		-
>> +	a4		4th parameter		-
>> +	a5		5th parameter		-
>> +        ========	================	================
> 
> Please consistently use tab.
> 
>> +
>> +The return codes may be one of the following:
>> +
>> +	====		=========================
>> +	Code		Meaning
>> +	====		=========================
>> +	0		Success
>> +	-1		Hypercall not implemented
>> +	-2		Bad Hypercall parameter
>> +	====		=========================
>> +
>> +KVM Hypercalls Documentation
>> +============================
>> +
>> +The template for each hypercall is as follows:
>> +
>> +1. Hypercall name
>> +2. Purpose
>> +
>> +1. KVM_HCALL_FUNC_PV_IPI
> 
> Is it still a work-in-progress thing? I don't see it in mainline.
It should be KVM_HCALL_FUNC_IPI here.

> 
>> +------------------------
>> +
>> +:Purpose: Send IPIs to multiple vCPUs.
>> +
>> +- a0: KVM_HCALL_FUNC_PV_IPI
>> +- a1: Lower part of the bitmap for destination physical CPUIDs
>> +- a2: Higher part of the bitmap for destination physical CPUIDs
>> +- a3: The lowest physical CPUID in the bitmap
> 
> - Is it a feature that implements IPI broadcast with a PV method?
> - Don't you need to *at least* specify which IPI to send by issuing this
>    hypercall?
Good question. It should be documented here. PV IPI on LoongArch 
includes both PV IPI multicast sending and PV IPI receiving, and SWI is 
used for PV IPI inject since there is no VM-exits accessing SWI registers.

> 
> But again, as I said I know nothing about loongarch.  I might have
> missed some obvious points.
> 
>> +
>> +The hypercall lets a guest send multiple IPIs (Inter-Process Interrupts) with
>> +at most 128 destinations per hypercall.The destinations are represented in a
>                                            ^
> Add a blank space.
> 
>> +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.
> 
> This looks really confusing.  "Bit 63 of a1 corresponds to the physical
> CPUID in a3+63 (a66)"?
The description is problematic, thanks for pointing it out.
It should be value of register a3 plus 1, rather than a4, how about 
*"the physical CPUID in a3 + 1"*  here?

Regards
Bibo Mao
> 
> Zenghui
> 

Powered by blists - more mailing lists