[PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[Dandan Zhang]

The kvm_hypercall set for LoongArch is limited to a1-a5.
The mention of a6 in the comment is undefined that needs to be rectified.

Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
---
 arch/loongarch/include/asm/kvm_para.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
index 335fb86778e2..43ec61589e6c 100644
--- a/arch/loongarch/include/asm/kvm_para.h
+++ b/arch/loongarch/include/asm/kvm_para.h
@@ -39,9 +39,9 @@ struct kvm_steal_time {
  * Hypercall interface for KVM hypervisor
  *
  * a0: function identifier
- * a1-a6: args
+ * a1-a5: args
  * Return value will be placed in a0.
- * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
+ * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
  */
 static __always_inline long kvm_hypercall0(u64 fid)
 {
-- 
2.43.4

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[maobibo]

On 2024/7/25 下午9:48, Dandan Zhang wrote:
> The kvm_hypercall set for LoongArch is limited to a1-a5.
> The mention of a6 in the comment is undefined that needs to be rectified.
> 
> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
> Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
> ---
>   arch/loongarch/include/asm/kvm_para.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
> index 335fb86778e2..43ec61589e6c 100644
> --- a/arch/loongarch/include/asm/kvm_para.h
> +++ b/arch/loongarch/include/asm/kvm_para.h
> @@ -39,9 +39,9 @@ struct kvm_steal_time {
>    * Hypercall interface for KVM hypervisor
>    *
>    * a0: function identifier
> - * a1-a6: args
> + * a1-a5: args
>    * Return value will be placed in a0.
> - * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
> + * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
>    */
>   static __always_inline long kvm_hypercall0(u64 fid)
>   {
> 

Dandan,

Nice catch. In future hypercall abi may expand such as the number of 
input register and output register, or async hypercall function if there 
is really such requirement.

Anyway the modification is deserved and it is enough to use now, thanks 
for doing it.

Reviewed-by: Bibo Mao <maobibo@loongson.cn>

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[Huacai Chen]

On Fri, Jul 26, 2024 at 9:49 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/7/25 下午9:48, Dandan Zhang wrote:
> > The kvm_hypercall set for LoongArch is limited to a1-a5.
> > The mention of a6 in the comment is undefined that needs to be rectified.
> >
> > Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
> > Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
> > ---
> >   arch/loongarch/include/asm/kvm_para.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
> > index 335fb86778e2..43ec61589e6c 100644
> > --- a/arch/loongarch/include/asm/kvm_para.h
> > +++ b/arch/loongarch/include/asm/kvm_para.h
> > @@ -39,9 +39,9 @@ struct kvm_steal_time {
> >    * Hypercall interface for KVM hypervisor
> >    *
> >    * a0: function identifier
> > - * a1-a6: args
> > + * a1-a5: args
> >    * Return value will be placed in a0.
> > - * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
> > + * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
> >    */
> >   static __always_inline long kvm_hypercall0(u64 fid)
> >   {
> >
>
> Dandan,
>
> Nice catch. In future hypercall abi may expand such as the number of
> input register and output register, or async hypercall function if there
> is really such requirement.
>
> Anyway the modification is deserved and it is enough to use now, thanks
> for doing it.
>
> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
Maybe it is better to implement kvm_hypercall6() than remove a6 now?

Huacai
>

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[WangYuli]

On 2024/7/26 10:55, Huacai Chen wrote:
> On Fri, Jul 26, 2024 at 9:49 AM maobibo <maobibo@loongson.cn> wrote:
>
> Maybe it is better to implement kvm_hypercall6() than remove a6 now?
>
It seems that The LoongArch KVM implementation does not include a 
kvm_hypercall6() function.

This might be a typo or an oversight.


Link: 
https://github.com/deepin-community/kernel/blob/linux-6.6.y/Documentation/virt/kvm/loongarch/hypercalls.rst 

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[maobibo]

On 2024/7/26 上午10:55, Huacai Chen wrote:
> On Fri, Jul 26, 2024 at 9:49 AM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/7/25 下午9:48, Dandan Zhang wrote:
>>> The kvm_hypercall set for LoongArch is limited to a1-a5.
>>> The mention of a6 in the comment is undefined that needs to be rectified.
>>>
>>> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
>>> Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
>>> ---
>>>    arch/loongarch/include/asm/kvm_para.h | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
>>> index 335fb86778e2..43ec61589e6c 100644
>>> --- a/arch/loongarch/include/asm/kvm_para.h
>>> +++ b/arch/loongarch/include/asm/kvm_para.h
>>> @@ -39,9 +39,9 @@ struct kvm_steal_time {
>>>     * Hypercall interface for KVM hypervisor
>>>     *
>>>     * a0: function identifier
>>> - * a1-a6: args
>>> + * a1-a5: args
>>>     * Return value will be placed in a0.
>>> - * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
>>> + * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
>>>     */
>>>    static __always_inline long kvm_hypercall0(u64 fid)
>>>    {
>>>
>>
>> Dandan,
>>
>> Nice catch. In future hypercall abi may expand such as the number of
>> input register and output register, or async hypercall function if there
>> is really such requirement.
>>
>> Anyway the modification is deserved and it is enough to use now, thanks
>> for doing it.
>>
>> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
> Maybe it is better to implement kvm_hypercall6() than remove a6 now?
That is one option also. The main reason is that there is no such 
requirement in near future :(, I prefer to removing the annotation and 
keeping it clean.

Regards
Bibo Mao
> 
> Huacai
>>

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[Huacai Chen]

On Fri, Jul 26, 2024 at 11:35 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/7/26 上午10:55, Huacai Chen wrote:
> > On Fri, Jul 26, 2024 at 9:49 AM maobibo <maobibo@loongson.cn> wrote:
> >>
> >>
> >>
> >> On 2024/7/25 下午9:48, Dandan Zhang wrote:
> >>> The kvm_hypercall set for LoongArch is limited to a1-a5.
> >>> The mention of a6 in the comment is undefined that needs to be rectified.
> >>>
> >>> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
> >>> Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
> >>> ---
> >>>    arch/loongarch/include/asm/kvm_para.h | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
> >>> index 335fb86778e2..43ec61589e6c 100644
> >>> --- a/arch/loongarch/include/asm/kvm_para.h
> >>> +++ b/arch/loongarch/include/asm/kvm_para.h
> >>> @@ -39,9 +39,9 @@ struct kvm_steal_time {
> >>>     * Hypercall interface for KVM hypervisor
> >>>     *
> >>>     * a0: function identifier
> >>> - * a1-a6: args
> >>> + * a1-a5: args
> >>>     * Return value will be placed in a0.
> >>> - * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
> >>> + * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
> >>>     */
> >>>    static __always_inline long kvm_hypercall0(u64 fid)
> >>>    {
> >>>
> >>
> >> Dandan,
> >>
> >> Nice catch. In future hypercall abi may expand such as the number of
> >> input register and output register, or async hypercall function if there
> >> is really such requirement.
> >>
> >> Anyway the modification is deserved and it is enough to use now, thanks
> >> for doing it.
> >>
> >> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
> > Maybe it is better to implement kvm_hypercall6() than remove a6 now?
> That is one option also. The main reason is that there is no such
> requirement in near future :(, I prefer to removing the annotation and
> keeping it clean.
I don't like removing something and then adding it back again, so if
kvm_hypercall6() is needed in future, it is better to add it now.

Huacai
>
> Regards
> Bibo Mao
> >
> > Huacai
> >>
>
>

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[maobibo]

On 2024/7/26 下午2:32, Huacai Chen wrote:
> On Fri, Jul 26, 2024 at 11:35 AM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/7/26 上午10:55, Huacai Chen wrote:
>>> On Fri, Jul 26, 2024 at 9:49 AM maobibo <maobibo@loongson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2024/7/25 下午9:48, Dandan Zhang wrote:
>>>>> The kvm_hypercall set for LoongArch is limited to a1-a5.
>>>>> The mention of a6 in the comment is undefined that needs to be rectified.
>>>>>
>>>>> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
>>>>> Signed-off-by: Dandan Zhang <zhangdandan@uniontech.com>
>>>>> ---
>>>>>     arch/loongarch/include/asm/kvm_para.h | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
>>>>> index 335fb86778e2..43ec61589e6c 100644
>>>>> --- a/arch/loongarch/include/asm/kvm_para.h
>>>>> +++ b/arch/loongarch/include/asm/kvm_para.h
>>>>> @@ -39,9 +39,9 @@ struct kvm_steal_time {
>>>>>      * Hypercall interface for KVM hypervisor
>>>>>      *
>>>>>      * a0: function identifier
>>>>> - * a1-a6: args
>>>>> + * a1-a5: args
>>>>>      * Return value will be placed in a0.
>>>>> - * Up to 6 arguments are passed in a1, a2, a3, a4, a5, a6.
>>>>> + * Up to 5 arguments are passed in a1, a2, a3, a4, a5.
>>>>>      */
>>>>>     static __always_inline long kvm_hypercall0(u64 fid)
>>>>>     {
>>>>>
>>>>
>>>> Dandan,
>>>>
>>>> Nice catch. In future hypercall abi may expand such as the number of
>>>> input register and output register, or async hypercall function if there
>>>> is really such requirement.
>>>>
>>>> Anyway the modification is deserved and it is enough to use now, thanks
>>>> for doing it.
>>>>
>>>> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
>>> Maybe it is better to implement kvm_hypercall6() than remove a6 now?
>> That is one option also. The main reason is that there is no such
>> requirement in near future :(, I prefer to removing the annotation and
>> keeping it clean.
> I don't like removing something and then adding it back again, so if
> kvm_hypercall6() is needed in future, it is better to add it now.
I do not see the requirement by now.

At the same time I just suggest you care about LoongArch kernel and 
catch up the gap, just go forward rather than go around. Is it 
responsibility of maintainer to catch future direction?

Thanks for merging LoongArch KVM code at beginning, and I think I can 
merge LoongArch kvm kernel to KVM tree directly just like KVM 
x86/ARM64/RISCV.

Regards
Bibo

> 
> Huacai
>>
>> Regards
>> Bibo Mao
>>>
>>> Huacai
>>>>
>>
>>

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[WangYuli]

Hi Bibo and Huacai,


Ah... tell me you two aren't arguing, right?


Both of you are working towards the same goal—making the upstream

code for the Loongarch architecture as clean and elegant as possible.

If it's just a disagreement about how to handle this small patch,

there's no need to make things complicated.


As a partner of yours and a community developer passionate about

Loongson CPU, I'd much rather see you two working together

harmoniously than complaining about each other's work. I have full

confidence in Bibo's judgment on the direction of KVM for Loongarch,

and I also believe that Huacai, as the Loongarch maintainer, has always

been fulfilling his responsibilities.


You are both excellent Linux developers. That's all.


To be specific about the controversy caused by this particular commit,

I think the root cause is that the KVM documentation for Loongarch

hasn't been upstreamed. In my opinion, the documentation seems

ready to be upstreamed. If you're all busy with more important work,

I can take the time to submit them and provide a Chinese translation.


If this is feasible, it would be better to merge this commit after that.


Best wishes,


--

WangYuli

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[maobibo]

On 2024/7/29 上午11:03, WangYuli wrote:
> Hi Bibo and Huacai,
> 
> 
> Ah... tell me you two aren't arguing, right?
> 
> 
> Both of you are working towards the same goal—making the upstream
> 
> code for the Loongarch architecture as clean and elegant as possible.
> 
> If it's just a disagreement about how to handle this small patch,
> 
> there's no need to make things complicated.
> 
> 
> As a partner of yours and a community developer passionate about
> 
> Loongson CPU, I'd much rather see you two working together
> 
> harmoniously than complaining about each other's work. I have full
> 
> confidence in Bibo's judgment on the direction of KVM for Loongarch,
> 
> and I also believe that Huacai, as the Loongarch maintainer, has always
> 
> been fulfilling his responsibilities.
> 
> 
> You are both excellent Linux developers. That's all.
> 
> 
> To be specific about the controversy caused by this particular commit,
> 
> I think the root cause is that the KVM documentation for Loongarch
> 
> hasn't been upstreamed. In my opinion, the documentation seems
> 
> ready to be upstreamed. If you're all busy with more important work,
> 
> I can take the time to submit them and provide a Chinese translation.
> 
> 
> If this is feasible, it would be better to merge this commit after that.
Yuli,

The argument is normal in the work and life :)

You are right, KVM document is important and any contribution is welcome.

Regards
Bibo Mao
> 
> 
> Best wishes,
> 
> 
> -- 
> 
> WangYuli
> 
> 

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[WangYuli]

Hi all,


The English documentation for LoongArch KVM has been submitted.


Link: 
https://lore.kernel.org/all/04DAF94279B88A3F+20240731055755.84082-1-wangyuli@uniontech.com/

To ensure consistency,I have observed that a complete Chinese

translation of the KVM subsystem is currently absent from the kernel.


Consequently,I will undertake the translation of the entire subsystem

prior to incorporating the LoongArch KVM-specific documentation.


This process may require some time,but I will endeavor to complete

it as expeditiously as possible.


Best wishes,

-- 
WangYuli*
*

Re: [PATCH] KVM: Loongarch: Remove undefined a6 argument comment for kvm_hypercall

[Huacai Chen]

On Mon, Jul 29, 2024 at 11:03 AM WangYuli <wangyuli@uniontech.com> wrote:
>
> Hi Bibo and Huacai,
>
>
> Ah... tell me you two aren't arguing, right?
>
>
> Both of you are working towards the same goal—making the upstream
>
> code for the Loongarch architecture as clean and elegant as possible.
>
> If it's just a disagreement about how to handle this small patch,
>
> there's no need to make things complicated.
>
>
> As a partner of yours and a community developer passionate about
>
> Loongson CPU, I'd much rather see you two working together
>
> harmoniously than complaining about each other's work. I have full
>
> confidence in Bibo's judgment on the direction of KVM for Loongarch,
>
> and I also believe that Huacai, as the Loongarch maintainer, has always
>
> been fulfilling his responsibilities.
>
>
> You are both excellent Linux developers. That's all.
Bibo has made lots of contributions on LoongArch/KVM, and he is more
professional than me. I hope we can collaborate well in the future.
For this patch itself, I've applied to loongarch-fixes, thanks.

Huacai

>
>
> To be specific about the controversy caused by this particular commit,
>
> I think the root cause is that the KVM documentation for Loongarch
>
> hasn't been upstreamed. In my opinion, the documentation seems
>
> ready to be upstreamed. If you're all busy with more important work,
>
> I can take the time to submit them and provide a Chinese translation.
>
>
> If this is feasible, it would be better to merge this commit after that.
>
>
> Best wishes,
>
>
> --
>
> WangYuli
>
>