Message-ID: <F7E458B77CD6B30A+554b04f1-7716-4a81-8755-c5a4f138ae71@uniontech.com>
Date: Wed, 28 Aug 2024 12:44:47 +0800
From: WangYuli <wangyuli@...ontech.com>
To: maobibo <maobibo@...ngson.cn>, 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,
 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

Thanks for review.

We'll get these problems sorted out as soon as possible.

On 2024/8/28 09:07, maobibo wrote:
> Zenghui,
>
> On 2024/8/27 1:00AM, 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
>>
>
>
Thanks
-- 
WangYuli

Powered by blists - more mailing lists