Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1683791148.git.haibo1.xu@intel.com> <921fc2e1a91887170e277acb1b52df57480a5736.1683791148.git.haibo1.xu@intel.com>
 <20230511-5e8b478be55881a754cc2002@orel>
In-Reply-To: <20230511-5e8b478be55881a754cc2002@orel>
From:   Haibo Xu <xiaobo55x@gmail.com>
Date:   Mon, 15 May 2023 14:21:17 +0800
Message-ID: <CAJve8on5CxNkzmfEPfmDSX=0quJL6iAoqMP+twBBw43QmTqdFg@mail.gmail.com>
Subject: Re: [PATCH 1/2] riscv: kvm: Add KVM_GET_REG_LIST API support
To:     Andrew Jones <ajones@ventanamicro.com>
Cc:     Haibo Xu <haibo1.xu@intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Anup Patel <anup@brainfault.org>,
        Atish Patra <atishp@atishpatra.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Shuah Khan <shuah@kernel.org>, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org,
        linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, May 11, 2023 at 6:44=E2=80=AFPM Andrew Jones <ajones@ventanamicro.c=
om> wrote:
>
> On Thu, May 11, 2023 at 05:22:48PM +0800, Haibo Xu wrote:
> > KVM_GET_REG_LIST API will return all registers that are available to
> > KVM_GET/SET_ONE_REG APIs. It's very useful to identify some platform
> > regression issue during VM migration.
> >
> > Since this API was already supported on arm64, it'd be straightforward
>
> s/it'd be/it is/
>
> > to enable it on riscv with similar code structure.
> >
> > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> > ---
> >  Documentation/virt/kvm/api.rst |   2 +-
> >  arch/riscv/kvm/vcpu.c          | 346 +++++++++++++++++++++++++++++++++
> >  2 files changed, 347 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/ap=
i.rst
> > index add067793b90..280e89abd004 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -3499,7 +3499,7 @@ VCPU matching underlying host.
> >  ---------------------
> >
> >  :Capability: basic
> > -:Architectures: arm64, mips
> > +:Architectures: arm64, mips, riscv
> >  :Type: vcpu ioctl
> >  :Parameters: struct kvm_reg_list (in/out)
> >  :Returns: 0 on success; -1 on error
> > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > index 8bd9f2a8a0b9..fb8834e4fa15 100644
> > --- a/arch/riscv/kvm/vcpu.c
> > +++ b/arch/riscv/kvm/vcpu.c
> > @@ -657,6 +657,334 @@ static int kvm_riscv_vcpu_set_reg_isa_ext(struct =
kvm_vcpu *vcpu,
> >       return 0;
> >  }
> >
> > +static inline unsigned long num_config_regs(void)
> > +{
> > +     return sizeof(struct kvm_riscv_config) / sizeof(unsigned long);
> > +}
> > +
> > +static int copy_config_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_config_regs();
> > +
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_CONFIG | i;
>                                           ^ this should be
>                                           size-ulong
>
>   u64 size =3D IS_ENABLED(CONFIG_32BIT) ? KVM_REG_SIZE_U32 : KVM_REG_SIZE=
_U64;
>   u64 reg =3D KVM_REG_RISCV | size | KVM_REG_RISCV_CONFIG | i;
>
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_core_regs(void)
> > +{
> > +     return sizeof(struct kvm_riscv_core) / sizeof(unsigned long);
> > +}
> > +
> > +static int copy_core_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_core_regs();
> > +
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_CORE | i;
>                                           ^ size-ulong
>
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_csr_regs(void)
> > +{
> > +     unsigned long n =3D sizeof(struct kvm_riscv_csr) / sizeof(unsigne=
d long);
> > +
> > +     if (kvm_riscv_aia_available())
> > +             n +=3D sizeof(struct kvm_riscv_aia_csr) / sizeof(unsigned=
 long);
> > +
> > +     return n;
> > +}
> > +
> > +static int copy_csr_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n1 =3D sizeof(struct kvm_riscv_csr) / sizeof(unsigned long);
> > +     int n2 =3D 0;
> > +
> > +     /* copy general csr regs */
> > +     for (i =3D 0; i < n1; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_CSR |
>                                           ^ size-ulong
>
> > +                               KVM_REG_RISCV_CSR_GENERAL | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     /* copy AIA csr regs */
> > +     if (kvm_riscv_aia_available()) {
> > +             n2 =3D sizeof(struct kvm_riscv_aia_csr) / sizeof(unsigned=
 long);
> > +
> > +             for (i =3D 0; i < n2; i++) {
> > +                     u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KV=
M_REG_RISCV_CSR |
>                                                   ^ size-ulong
>
> > +                                       KVM_REG_RISCV_CSR_AIA | i;
> > +
> > +                     if (uindices) {
> > +                             if (put_user(reg, uindices))
> > +                                     return -EFAULT;
> > +                             uindices++;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return n1 + n2;
> > +}
> > +
> > +static inline unsigned long num_timer_regs(void)
> > +{
> > +     return sizeof(struct kvm_riscv_timer) / sizeof(unsigned long);
> > +}
> > +
> > +static int copy_timer_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_timer_regs();
> > +
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_TIMER | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_fp_f_regs(const struct kvm_vcpu *vcpu)
> > +{
> > +     const struct kvm_cpu_context *cntx =3D &vcpu->arch.guest_context;
> > +
> > +     if (riscv_isa_extension_available(vcpu->arch.isa, f))
> > +             return sizeof(cntx->fp.f) / sizeof(u32);
> > +     else
> > +             return 0;
> > +}
> > +
> > +static int copy_fp_f_reg_indices(const struct kvm_vcpu *vcpu,
> > +                                     u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_fp_f_regs(vcpu);
> > +
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U32 | KVM_REG_RI=
SCV_FP_F | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_fp_d_regs(const struct kvm_vcpu *vcpu)
> > +{
> > +     const struct kvm_cpu_context *cntx =3D &vcpu->arch.guest_context;
> > +
> > +     if (riscv_isa_extension_available(vcpu->arch.isa, d))
> > +             return sizeof(cntx->fp.d.f) / sizeof(u64) + 1;
> > +     else
> > +             return 0;
> > +}
> > +
> > +static int copy_fp_d_reg_indices(const struct kvm_vcpu *vcpu,
> > +                                     u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_fp_d_regs(vcpu);
> > +     u64 reg;
> > +
> > +     /* copy fp.d.f indeices */
>
> indices
>
> > +     for (i =3D 0; i < n-1; i++) {
> > +             reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_=
FP_D | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     /* copy fp.d.fcsr indeices */
>
> indices
>
> > +     reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U32 | KVM_REG_RISCV_FP_D | i=
;
> > +     if (uindices) {
> > +             if (put_user(reg, uindices))
> > +                     return -EFAULT;
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_isa_ext_regs(void)
> > +{
> > +     return KVM_RISCV_ISA_EXT_MAX;
> > +}
> > +
> > +static int copy_isa_ext_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n =3D num_isa_ext_regs();
> > +
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_ISA_EXT | i;
>                                           ^ size-ulong
>
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +static inline unsigned long num_sbi_ext_regs(void)
> > +{
> > +     /* number of KVM_REG_RISCV_SBI_SINGLE +
> > +      *  2x(number of KVM_REG_RISCV_SBI_MULTI)
> > +      */
>
> Please use an opening wing '/*' on comments.
>
> > +     return KVM_RISCV_SBI_EXT_MAX + 2*(KVM_REG_RISCV_SBI_MULTI_REG_LAS=
T+1);
> > +}
> > +
> > +static int copy_sbi_ext_reg_indices(u64 __user *uindices)
> > +{
> > +     unsigned int i;
> > +     int n;
> > +
> > +     /* copy KVM_REG_RISCV_SBI_SINGLE */
> > +     n =3D KVM_RISCV_SBI_EXT_MAX;
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_SBI_EXT |
>                                           ^ size-ulong
>
> > +                               KVM_REG_RISCV_SBI_SINGLE | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     /* copy KVM_REG_RISCV_SBI_MULTI */
> > +     n =3D KVM_REG_RISCV_SBI_MULTI_REG_LAST + 1;
> > +     for (i =3D 0; i < n; i++) {
> > +             u64 reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RI=
SCV_SBI_EXT |
>                                           ^ size-ulong
>
> > +                               KVM_REG_RISCV_SBI_MULTI_EN | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +
> > +             reg =3D KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_=
SBI_EXT |
> > +                       KVM_REG_RISCV_SBI_MULTI_DIS | i;
> > +
> > +             if (uindices) {
> > +                     if (put_user(reg, uindices))
> > +                             return -EFAULT;
> > +                     uindices++;
> > +             }
> > +     }
> > +
> > +     return num_sbi_ext_regs();
> > +}
> > +
> > +/**
> > + * kvm_riscv_vcpu_num_regs - how many registers do we present via KVM_=
GET/SET_ONE_REG
> > + *
> > + * This is for all registers.
> > + */
> > +static unsigned long kvm_riscv_vcpu_num_regs(struct kvm_vcpu *vcpu)
> > +{
> > +     unsigned long res =3D 0;
> > +
> > +     res +=3D num_config_regs();
> > +     res +=3D num_core_regs();
> > +     res +=3D num_csr_regs();
> > +     res +=3D num_timer_regs();
> > +     res +=3D num_fp_f_regs(vcpu);
> > +     res +=3D num_fp_d_regs(vcpu);
> > +     res +=3D num_isa_ext_regs();
> > +     res +=3D num_sbi_ext_regs();
> > +
> > +     return res;
> > +}
> > +
> > +/**
> > + * kvm_riscv_vcpu_copy_reg_indices - get indices of all registers.
> > + */
> > +static int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu,
> > +                             u64 __user *uindices)
> > +{
> > +     int ret;
> > +
> > +     ret =3D copy_config_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_core_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_csr_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_timer_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_fp_f_reg_indices(vcpu, uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_fp_d_reg_indices(vcpu, uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_isa_ext_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +     uindices +=3D ret;
> > +
> > +     ret =3D copy_sbi_ext_reg_indices(uindices);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> >  static int kvm_riscv_vcpu_set_reg(struct kvm_vcpu *vcpu,
> >                                 const struct kvm_one_reg *reg)
> >  {
> > @@ -758,6 +1086,24 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >                       r =3D kvm_riscv_vcpu_get_reg(vcpu, &reg);
> >               break;
> >       }
> > +     case KVM_GET_REG_LIST: {
> > +             struct kvm_reg_list __user *user_list =3D argp;
> > +             struct kvm_reg_list reg_list;
> > +             unsigned int n;
> > +
> > +             r =3D -EFAULT;
> > +             if (copy_from_user(&reg_list, user_list, sizeof(reg_list)=
))
> > +                     break;
> > +             n =3D reg_list.n;
> > +             reg_list.n =3D kvm_riscv_vcpu_num_regs(vcpu);
> > +             if (copy_to_user(user_list, &reg_list, sizeof(reg_list)))
> > +                     break;
> > +             r =3D -E2BIG;
> > +             if (n < reg_list.n)
> > +                     break;
> > +             r =3D kvm_riscv_vcpu_copy_reg_indices(vcpu, user_list->re=
g);
> > +             break;
> > +     }
> >       default:
> >               break;
> >       }
> > --
> > 2.34.1
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Thanks,
> drew

Thanks for your review, Andrew! The comments will be addressed in next vers=
ion.