| Message ID | 20230801222629.210929-1-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RISC-V: KVM: change get_reg/set_reg error codes | expand |
On Tue, Aug 01, 2023 at 07:26:20PM -0300, Daniel Henrique Barboza wrote: > Hi, > > In this new version 3 new patches (6, 7, 8) were added by Andrew's > request during the v1 review. > > We're now avoiding throwing an -EBUSY error if a reg write is done after > the vcpu started spinning if the value being written is the same as KVM > already uses. This follows the design choice made in patch 3, allowing > for userspace 'lazy write' of registers. > > I decided to add 3 patches instead of one because the no-op check made > in patches 6 and 8 aren't just a matter of doing reg_val = host_val. > They can be squashed in a single patch if required. > > Please check the version 1 cover-letter [1] for the motivation behind > this work. Patches were based on top of riscv_kvm_queue. > > Changes from v1: > - patches 6,7, 8 (new): > - make reg writes a no-op, regardless of vcpu->arch.ran_atleast_once > state, if the value being written is the same as the host > - v1 link: https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ > > [1] https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ > I found three missing conversions, which are in the diff below. Also, I saw that the vector registers were lacking good error returns, so I reworked that and attached a completely (not even compile) tested patch for them. Please work that patch into this series. Thanks, drew diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index c88b0c7f7f01..6ca90c04ba61 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -506,7 +506,7 @@ static int riscv_vcpu_get_isa_ext_multi(struct kvm_vcpu *vcpu, unsigned long i, ext_id, ext_val; if (reg_num > KVM_REG_RISCV_ISA_MULTI_REG_LAST) - return -EINVAL; + return -ENOENT; for (i = 0; i < BITS_PER_LONG; i++) { ext_id = i + reg_num * BITS_PER_LONG; @@ -529,7 +529,7 @@ static int riscv_vcpu_set_isa_ext_multi(struct kvm_vcpu *vcpu, unsigned long i, ext_id; if (reg_num > KVM_REG_RISCV_ISA_MULTI_REG_LAST) - return -EINVAL; + return -ENOENT; for_each_set_bit(i, ®_val, BITS_PER_LONG) { ext_id = i + reg_num * BITS_PER_LONG; @@ -644,7 +644,7 @@ int kvm_riscv_vcpu_set_reg(struct kvm_vcpu *vcpu, break; } - return -EINVAL; + return -ENOENT; } int kvm_riscv_vcpu_get_reg(struct kvm_vcpu *vcpu, From b1472501677ea6bd73689e0f947149f5f86da9cc Mon Sep 17 00:00:00 2001 From: Andrew Jones <ajones@ventanamicro.com> Date: Wed, 2 Aug 2023 11:52:04 +0300 Subject: [PATCH] riscv: KVM: Improve vector save/restore errors Content-type: text/plain --- arch/riscv/kvm/vcpu_vector.c | 60 ++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c index edd2eecbddc2..39c5bceb4d1b 100644 --- a/arch/riscv/kvm/vcpu_vector.c +++ b/arch/riscv/kvm/vcpu_vector.c @@ -91,44 +91,44 @@ void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu) } #endif -static void *kvm_riscv_vcpu_vreg_addr(struct kvm_vcpu *vcpu, +static int kvm_riscv_vcpu_vreg_addr(struct kvm_vcpu *vcpu, unsigned long reg_num, - size_t reg_size) + size_t reg_size, + void **reg_val) { struct kvm_cpu_context *cntx = &vcpu->arch.guest_context; - void *reg_val; size_t vlenb = riscv_v_vsize / 32; if (reg_num < KVM_REG_RISCV_VECTOR_REG(0)) { if (reg_size != sizeof(unsigned long)) - return NULL; + return -EINVAL; switch (reg_num) { case KVM_REG_RISCV_VECTOR_CSR_REG(vstart): - reg_val = &cntx->vector.vstart; + *reg_val = &cntx->vector.vstart; break; case KVM_REG_RISCV_VECTOR_CSR_REG(vl): - reg_val = &cntx->vector.vl; + *reg_val = &cntx->vector.vl; break; case KVM_REG_RISCV_VECTOR_CSR_REG(vtype): - reg_val = &cntx->vector.vtype; + *reg_val = &cntx->vector.vtype; break; case KVM_REG_RISCV_VECTOR_CSR_REG(vcsr): - reg_val = &cntx->vector.vcsr; + *reg_val = &cntx->vector.vcsr; break; case KVM_REG_RISCV_VECTOR_CSR_REG(datap): default: - return NULL; + return -ENOENT; } } else if (reg_num <= KVM_REG_RISCV_VECTOR_REG(31)) { if (reg_size != vlenb) - return NULL; - reg_val = cntx->vector.datap + return -EINVAL; + *reg_val = cntx->vector.datap + (reg_num - KVM_REG_RISCV_VECTOR_REG(0)) * vlenb; } else { - return NULL; + return -ENOENT; } - return reg_val; + return 0; } int kvm_riscv_vcpu_get_reg_vector(struct kvm_vcpu *vcpu, @@ -141,17 +141,20 @@ int kvm_riscv_vcpu_get_reg_vector(struct kvm_vcpu *vcpu, unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | rtype); - void *reg_val = NULL; size_t reg_size = KVM_REG_SIZE(reg->id); + void *reg_val; + int rc; - if (rtype == KVM_REG_RISCV_VECTOR && - riscv_isa_extension_available(isa, v)) { - reg_val = kvm_riscv_vcpu_vreg_addr(vcpu, reg_num, reg_size); - } - - if (!reg_val) + if (rtype != KVM_REG_RISCV_VECTOR) return -EINVAL; + if (!riscv_isa_extension_available(isa, v)) + return -ENOENT; + + rc = kvm_riscv_vcpu_vreg_addr(vcpu, reg_num, reg_size, ®_val); + if (rc) + return rc; + if (copy_to_user(uaddr, reg_val, reg_size)) return -EFAULT; @@ -168,17 +171,20 @@ int kvm_riscv_vcpu_set_reg_vector(struct kvm_vcpu *vcpu, unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | rtype); - void *reg_val = NULL; size_t reg_size = KVM_REG_SIZE(reg->id); + void *reg_val; + int rc; - if (rtype == KVM_REG_RISCV_VECTOR && - riscv_isa_extension_available(isa, v)) { - reg_val = kvm_riscv_vcpu_vreg_addr(vcpu, reg_num, reg_size); - } - - if (!reg_val) + if (rtype != KVM_REG_RISCV_VECTOR) return -EINVAL; + if (!riscv_isa_extension_available(isa, v)) + return -ENOENT; + + rc = kvm_riscv_vcpu_vreg_addr(vcpu, reg_num, reg_size, ®_val); + if (rc) + return rc; + if (copy_from_user(reg_val, uaddr, reg_size)) return -EFAULT;
On Wed, Aug 02, 2023 at 12:04:25PM +0300, Andrew Jones wrote: > On Tue, Aug 01, 2023 at 07:26:20PM -0300, Daniel Henrique Barboza wrote: > > Hi, > > > > In this new version 3 new patches (6, 7, 8) were added by Andrew's > > request during the v1 review. > > > > We're now avoiding throwing an -EBUSY error if a reg write is done after > > the vcpu started spinning if the value being written is the same as KVM > > already uses. This follows the design choice made in patch 3, allowing > > for userspace 'lazy write' of registers. > > > > I decided to add 3 patches instead of one because the no-op check made > > in patches 6 and 8 aren't just a matter of doing reg_val = host_val. > > They can be squashed in a single patch if required. > > > > Please check the version 1 cover-letter [1] for the motivation behind > > this work. Patches were based on top of riscv_kvm_queue. > > > > Changes from v1: > > - patches 6,7, 8 (new): > > - make reg writes a no-op, regardless of vcpu->arch.ran_atleast_once > > state, if the value being written is the same as the host > > - v1 link: https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ > > > > [1] https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ > > > > I found three missing conversions, which are in the diff below. Also, I > saw that the vector registers were lacking good error returns, so I reworked > that and attached a completely (not even compile) tested patch for them. ^un
Hi, In this new version 3 new patches (6, 7, 8) were added by Andrew's request during the v1 review. We're now avoiding throwing an -EBUSY error if a reg write is done after the vcpu started spinning if the value being written is the same as KVM already uses. This follows the design choice made in patch 3, allowing for userspace 'lazy write' of registers. I decided to add 3 patches instead of one because the no-op check made in patches 6 and 8 aren't just a matter of doing reg_val = host_val. They can be squashed in a single patch if required. Please check the version 1 cover-letter [1] for the motivation behind this work. Patches were based on top of riscv_kvm_queue. Changes from v1: - patches 6,7, 8 (new): - make reg writes a no-op, regardless of vcpu->arch.ran_atleast_once state, if the value being written is the same as the host - v1 link: https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ [1] https://lore.kernel.org/kvm/20230731120420.91007-1-dbarboza@ventanamicro.com/ Daniel Henrique Barboza (9): RISC-V: KVM: return ENOENT in *_one_reg() when reg is unknown RISC-V: KVM: use ENOENT in *_one_reg() when extension is unavailable RISC-V: KVM: do not EOPNOTSUPP in set_one_reg() zicbo(m|z) RISC-V: KVM: do not EOPNOTSUPP in set KVM_REG_RISCV_TIMER_REG RISC-V: KVM: use EBUSY when !vcpu->arch.ran_atleast_once RISC-V: KVM: avoid EBUSY when writing same ISA val RISC-V: KVM: avoid EBUSY when writing the same machine ID val RISC-V: KVM: avoid EBUSY when writing the same isa_ext val docs: kvm: riscv: document EBUSY in KVM_SET_ONE_REG Documentation/virt/kvm/api.rst | 2 + arch/riscv/kvm/aia.c | 4 +- arch/riscv/kvm/vcpu_fp.c | 12 +++--- arch/riscv/kvm/vcpu_onereg.c | 68 +++++++++++++++++++++++----------- arch/riscv/kvm/vcpu_sbi.c | 16 ++++---- arch/riscv/kvm/vcpu_timer.c | 11 +++--- 6 files changed, 71 insertions(+), 42 deletions(-)