On Sun, Dec 17, 2023 at 12:40 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
>
> Some SBI extensions have state that needs to be saved / restored
> when migrating the VM. Provide a get/set-one-reg register type
> for SBI extension registers. Each SBI extension that uses this type
> will have its own subtype. There are currently no subtypes defined.
> The next patch introduces the first one.
>
> Reviewed-by: Anup Patel <anup@xxxxxxxxxxxxxx>
> Signed-off-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> ---
> arch/riscv/include/asm/kvm_vcpu_sbi.h | 4 ++
> arch/riscv/include/uapi/asm/kvm.h | 3 ++
> arch/riscv/kvm/vcpu_onereg.c | 42 +++++++++++++++++--
> arch/riscv/kvm/vcpu_sbi.c | 58 +++++++++++++++++++++++++++
> 4 files changed, 103 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 99c23bb37a37..dd60f73b5c36 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -60,6 +60,10 @@ int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
> const struct kvm_one_reg *reg);
> int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
> const struct kvm_one_reg *reg);
> +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
> + const struct kvm_one_reg *reg);
> +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
> + const struct kvm_one_reg *reg);
> const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> struct kvm_vcpu *vcpu, unsigned long extid);
> bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx);
> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index e961d79622fb..30f89a0e855f 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/kvm.h
> @@ -242,6 +242,9 @@ enum KVM_RISCV_SBI_EXT_ID {
> #define KVM_REG_RISCV_VECTOR_REG(n) \
> ((n) + sizeof(struct __riscv_v_ext_state) / sizeof(unsigned long))
>
> +/* Registers for specific SBI extensions are mapped as type 10 */
> +#define KVM_REG_RISCV_SBI (0x0a << KVM_REG_RISCV_TYPE_SHIFT)
> +
nit comment: KVM_REG_RISCV_SBI looks bit odd when we already have
KVM_REG_RISCV_SBI_EXT for
extension enabling/disabling.
How about renaming this to KVM_REG_RISCV_SBI_EXT_STATE or something
similar indicate that this
for a specific extension state ?
> /* Device Control API: RISC-V AIA */
> #define KVM_DEV_RISCV_APLIC_ALIGN 0x1000
> #define KVM_DEV_RISCV_APLIC_SIZE 0x4000
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index 11cdbf844291..901480e73817 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -961,6 +961,29 @@ static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu)
> return copy_sbi_ext_reg_indices(vcpu, NULL);
> }
>
> +static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu)
> +{
> + return 0;
> +}
> +
> +static int copy_sbi_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> + int n = num_sbi_regs(vcpu);
> +
> + for (int i = 0; i < n; i++) {
> + u64 reg = KVM_REG_RISCV | KVM_REG_SIZE_U64 |
> + KVM_REG_RISCV_SBI | i;
> +
> + if (uindices) {
> + if (put_user(reg, uindices))
> + return -EFAULT;
> + uindices++;
> + }
> + }
> +
> + return n;
> +}
> +
> static inline unsigned long num_vector_regs(const struct kvm_vcpu *vcpu)
> {
> if (!riscv_isa_extension_available(vcpu->arch.isa, v))
> @@ -1028,6 +1051,7 @@ unsigned long kvm_riscv_vcpu_num_regs(struct kvm_vcpu *vcpu)
> res += num_vector_regs(vcpu);
> res += num_isa_ext_regs(vcpu);
> res += num_sbi_ext_regs(vcpu);
> + res += num_sbi_regs(vcpu);
>
> return res;
> }
> @@ -1083,6 +1107,12 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu,
> ret = copy_sbi_ext_reg_indices(vcpu, uindices);
> if (ret < 0)
> return ret;
> + uindices += ret;
> +
> + ret = copy_sbi_reg_indices(vcpu, uindices);
> + if (ret < 0)
> + return ret;
> + uindices += ret;
>
> return 0;
> }
> @@ -1105,12 +1135,14 @@ int kvm_riscv_vcpu_set_reg(struct kvm_vcpu *vcpu,
> case KVM_REG_RISCV_FP_D:
> return kvm_riscv_vcpu_set_reg_fp(vcpu, reg,
> KVM_REG_RISCV_FP_D);
> + case KVM_REG_RISCV_VECTOR:
> + return kvm_riscv_vcpu_set_reg_vector(vcpu, reg);
> case KVM_REG_RISCV_ISA_EXT:
> return kvm_riscv_vcpu_set_reg_isa_ext(vcpu, reg);
> case KVM_REG_RISCV_SBI_EXT:
> return kvm_riscv_vcpu_set_reg_sbi_ext(vcpu, reg);
> - case KVM_REG_RISCV_VECTOR:
> - return kvm_riscv_vcpu_set_reg_vector(vcpu, reg);
> + case KVM_REG_RISCV_SBI:
> + return kvm_riscv_vcpu_set_reg_sbi(vcpu, reg);
> default:
> break;
> }
> @@ -1136,12 +1168,14 @@ int kvm_riscv_vcpu_get_reg(struct kvm_vcpu *vcpu,
> case KVM_REG_RISCV_FP_D:
> return kvm_riscv_vcpu_get_reg_fp(vcpu, reg,
> KVM_REG_RISCV_FP_D);
> + case KVM_REG_RISCV_VECTOR:
> + return kvm_riscv_vcpu_get_reg_vector(vcpu, reg);
> case KVM_REG_RISCV_ISA_EXT:
> return kvm_riscv_vcpu_get_reg_isa_ext(vcpu, reg);
> case KVM_REG_RISCV_SBI_EXT:
> return kvm_riscv_vcpu_get_reg_sbi_ext(vcpu, reg);
> - case KVM_REG_RISCV_VECTOR:
> - return kvm_riscv_vcpu_get_reg_vector(vcpu, reg);
> + case KVM_REG_RISCV_SBI:
> + return kvm_riscv_vcpu_get_reg_sbi(vcpu, reg);
> default:
> break;
> }
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index 088daaa23dd8..834176242ddf 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -325,6 +325,64 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
> + const struct kvm_one_reg *reg)
> +{
> + unsigned long __user *uaddr =
> + (unsigned long __user *)(unsigned long)reg->addr;
> + unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> + KVM_REG_SIZE_MASK |
> + KVM_REG_RISCV_SBI);
> + unsigned long reg_subtype, reg_val;
> +
> + if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
> + return -EINVAL;
> +
> + if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
> +
> + reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK;
> + reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
> +
> + switch (reg_subtype) {
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
> + const struct kvm_one_reg *reg)
> +{
> + unsigned long __user *uaddr =
> + (unsigned long __user *)(unsigned long)reg->addr;
> + unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> + KVM_REG_SIZE_MASK |
> + KVM_REG_RISCV_SBI);
> + unsigned long reg_subtype, reg_val;
> + int ret;
> +
> + if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
> + return -EINVAL;
> +
> + reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK;
> + reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
> +
> + switch (reg_subtype) {
> + default:
> + return -EINVAL;
> + }
> +
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> struct kvm_vcpu *vcpu, unsigned long extid)
> {
> --
> 2.43.0
>
Other than that, lgtm.
Reviewed-by: Atish Patra <atishp@xxxxxxxxxxxx>
--
Regards,
Atish
