Received-SPF: pass (google.com: domain of linux-kernel+bounces-65249-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240214101557.2900512-1-kernel@xen0n.name> <20240214101557.2900512-6-kernel@xen0n.name>
In-Reply-To: <20240214101557.2900512-6-kernel@xen0n.name>
From: Huacai Chen <chenhuacai@kernel.org>
Date: Wed, 14 Feb 2024 21:08:44 +0800
Message-ID: <CAAhV-H43mQy34FWqRCA4h9YxsFai3AsALvqNP82OZhYdAw5TbA@mail.gmail.com>
Subject: Re: [PATCH for-6.8 5/5] KVM: LoongArch: Clean up comments of
 _kvm_get_cpucfg_mask and kvm_check_cpucfg
To: WANG Xuerui <kernel@xen0n.name>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Tianrui Zhao <zhaotianrui@loongson.cn>, 
	Bibo Mao <maobibo@loongson.cn>, kvm@vger.kernel.org, loongarch@lists.linux.dev, 
	linux-kernel@vger.kernel.org, WANG Xuerui <git@xen0n.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi, Xuerui,

On Wed, Feb 14, 2024 at 6:16=E2=80=AFPM WANG Xuerui <kernel@xen0n.name> wro=
te:
>
> From: WANG Xuerui <git@xen0n.name>
>
> Remove comments that are merely restatement of the code nearby, and
> paraphrase the rest so they read more natural for English speakers (that
> lack understanding of Chinese grammar). No functional changes.
>
> Signed-off-by: WANG Xuerui <git@xen0n.name>
> ---
>  arch/loongarch/kvm/vcpu.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 9e108ffaba30..ff51d6ba59aa 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -302,20 +302,14 @@ static int _kvm_get_cpucfg_mask(int id, u64 *v)
>  {
>         switch (id) {
>         case 2:
> -               /* Return CPUCFG2 features which have been supported by K=
VM */
> +               /* CPUCFG2 features unconditionally supported by KVM */
>                 *v =3D CPUCFG2_FP     | CPUCFG2_FPSP  | CPUCFG2_FPDP     =
|
>                      CPUCFG2_FPVERS | CPUCFG2_LLFTP | CPUCFG2_LLFTPREV |
>                      CPUCFG2_LAM;
> -               /*
> -                * If LSX is supported by CPU, it is also supported by KV=
M,
> -                * as we implement it.
> -                */
> +               /* If LSX is supported by the host, then it is also suppo=
rted by KVM */
>                 if (cpu_has_lsx)
>                         *v |=3D CPUCFG2_LSX;
> -               /*
> -                * if LASX is supported by CPU, it is also supported by K=
VM,
> -                * as we implement it.
> -                */
> +               /* Same with LASX */
Consider a full description "If LASX is supported by the host, then it
is also supported by KVM"?

>                 if (cpu_has_lasx)
>                         *v |=3D CPUCFG2_LASX;
>
> @@ -336,21 +330,23 @@ static int kvm_check_cpucfg(int id, u64 val)
>
>         switch (id) {
>         case 2:
> -               /* CPUCFG2 features checking */
>                 if (val & ~mask)
> -                       /* The unsupported features should not be set */
> +                       /* Unsupported features should not be set */
>                         return -EINVAL;
>                 if (!(val & CPUCFG2_LLFTP))
> -                       /* The LLFTP must be set, as guest must has a con=
stant timer */
> +                       /* Guests must have a constant timer */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val=
 & CPUCFG2_FPDP)))
> -                       /* Single and double float point must both be set=
 when enable FP */
> +                       /* Single and double float point must both be set=
 when FP is enabled */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
> -                       /* FP should be set when enable LSX */
> +                       /* LSX is architecturally defined to imply FP */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
> -                       /* LSX, FP should be set when enable LASX, and FP=
 has been checked before. */
> +                       /*
> +                        * LASX is architecturally defined to imply LSX a=
nd FP
> +                        * FP is checked just above
I think "LASX is architecturally defined to imply LSX and FP" is enough her=
e.

> +                        */
>                         return -EINVAL;
>                 return 0;
>         default:
And I prefer to squash the last two patches together.

Huacai

> --
> 2.43.0
>
>