| Message ID | 156958523534.1503771.7854438316257986828.stgit@bahia.lan |
|---|---|
| State | Accepted |
| Headers | show |
| Series | KVM: PPC: Book3S: HV: XIVE: Allocate less VPs in OPAL | expand |
On 27/09/2019 13:53, Greg Kurz wrote: > Reduce code duplication by consolidating the checking of vCPU ids and VP > ids to a common helper used by both legacy and native XIVE KVM devices. > And explain the magic with a comment. > > Signed-off-by: Greg Kurz <groug@kaod.org> Looks fine. One question below, > --- > arch/powerpc/kvm/book3s_xive.c | 42 ++++++++++++++++++++++++++------- > arch/powerpc/kvm/book3s_xive.h | 1 + > arch/powerpc/kvm/book3s_xive_native.c | 11 ++------- > 3 files changed, 36 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c > index 0b7859e40f66..d84da9f6ee88 100644 > --- a/arch/powerpc/kvm/book3s_xive.c > +++ b/arch/powerpc/kvm/book3s_xive.c > @@ -1211,6 +1211,37 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu) > vcpu->arch.xive_vcpu = NULL; > } > > +static bool kvmppc_xive_vcpu_id_valid(struct kvmppc_xive *xive, u32 cpu) > +{ > + /* We have a block of KVM_MAX_VCPUS VPs. We just need to check > + * raw vCPU ids are below the expected limit for this guest's > + * core stride ; kvmppc_pack_vcpu_id() will pack them down to an > + * index that can be safely used to compute a VP id that belongs > + * to the VP block. > + */ > + return cpu < KVM_MAX_VCPUS * xive->kvm->arch.emul_smt_mode; > +} > + > +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp) > +{ > + u32 vp_id; > + > + if (!kvmppc_xive_vcpu_id_valid(xive, cpu)) { > + pr_devel("Out of bounds !\n"); > + return -EINVAL; > + } > + > + vp_id = kvmppc_xive_vp(xive, cpu); > + if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > + pr_devel("Duplicate !\n"); > + return -EEXIST; > + } > + > + *vp = vp_id; > + > + return 0; why not return vp_id ? and test for a negative value in callers. C. > +} > + > int kvmppc_xive_connect_vcpu(struct kvm_device *dev, > struct kvm_vcpu *vcpu, u32 cpu) > { > @@ -1229,20 +1260,13 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev, > return -EPERM; > if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) > return -EBUSY; > - if (cpu >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { > - pr_devel("Out of bounds !\n"); > - return -EINVAL; > - } > > /* We need to synchronize with queue provisioning */ > mutex_lock(&xive->lock); > > - vp_id = kvmppc_xive_vp(xive, cpu); > - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > - pr_devel("Duplicate !\n"); > - r = -EEXIST; > + r = kvmppc_xive_compute_vp_id(xive, cpu, &vp_id); > + if (r) > goto bail; > - } > > xc = kzalloc(sizeof(*xc), GFP_KERNEL); > if (!xc) { > diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h > index fe3ed50e0818..90cf6ec35a68 100644 > --- a/arch/powerpc/kvm/book3s_xive.h > +++ b/arch/powerpc/kvm/book3s_xive.h > @@ -296,6 +296,7 @@ int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio, > struct kvmppc_xive *kvmppc_xive_get_device(struct kvm *kvm, u32 type); > void xive_cleanup_single_escalation(struct kvm_vcpu *vcpu, > struct kvmppc_xive_vcpu *xc, int irq); > +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp); > > #endif /* CONFIG_KVM_XICS */ > #endif /* _KVM_PPC_BOOK3S_XICS_H */ > diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c > index 43a86858390a..5bb480b2aafd 100644 > --- a/arch/powerpc/kvm/book3s_xive_native.c > +++ b/arch/powerpc/kvm/book3s_xive_native.c > @@ -118,19 +118,12 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, > return -EPERM; > if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) > return -EBUSY; > - if (server_num >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { > - pr_devel("Out of bounds !\n"); > - return -EINVAL; > - } > > mutex_lock(&xive->lock); > > - vp_id = kvmppc_xive_vp(xive, server_num); > - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > - pr_devel("Duplicate !\n"); > - rc = -EEXIST; > + rc = kvmppc_xive_compute_vp_id(xive, server_num, &vp_id); > + if (rc) > goto bail; > - } > > xc = kzalloc(sizeof(*xc), GFP_KERNEL); > if (!xc) { >
On Mon, 30 Sep 2019 14:01:56 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 27/09/2019 13:53, Greg Kurz wrote: > > Reduce code duplication by consolidating the checking of vCPU ids and VP > > ids to a common helper used by both legacy and native XIVE KVM devices. > > And explain the magic with a comment. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Looks fine. One question below, > > > --- > > arch/powerpc/kvm/book3s_xive.c | 42 ++++++++++++++++++++++++++------- > > arch/powerpc/kvm/book3s_xive.h | 1 + > > arch/powerpc/kvm/book3s_xive_native.c | 11 ++------- > > 3 files changed, 36 insertions(+), 18 deletions(-) > > > > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c > > index 0b7859e40f66..d84da9f6ee88 100644 > > --- a/arch/powerpc/kvm/book3s_xive.c > > +++ b/arch/powerpc/kvm/book3s_xive.c > > @@ -1211,6 +1211,37 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu) > > vcpu->arch.xive_vcpu = NULL; > > } > > > > +static bool kvmppc_xive_vcpu_id_valid(struct kvmppc_xive *xive, u32 cpu) > > +{ > > + /* We have a block of KVM_MAX_VCPUS VPs. We just need to check > > + * raw vCPU ids are below the expected limit for this guest's > > + * core stride ; kvmppc_pack_vcpu_id() will pack them down to an > > + * index that can be safely used to compute a VP id that belongs > > + * to the VP block. > > + */ > > + return cpu < KVM_MAX_VCPUS * xive->kvm->arch.emul_smt_mode; > > +} > > + > > +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp) > > +{ > > + u32 vp_id; > > + > > + if (!kvmppc_xive_vcpu_id_valid(xive, cpu)) { > > + pr_devel("Out of bounds !\n"); > > + return -EINVAL; > > + } > > + > > + vp_id = kvmppc_xive_vp(xive, cpu); > > + if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > > + pr_devel("Duplicate !\n"); > > + return -EEXIST; > > + } > > + > > + *vp = vp_id; > > + > > + return 0; > > why not return vp_id ? and test for a negative value in callers. > Simpler code in the callers IMHO. > > C. > > > +} > > + > > int kvmppc_xive_connect_vcpu(struct kvm_device *dev, > > struct kvm_vcpu *vcpu, u32 cpu) > > { > > @@ -1229,20 +1260,13 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev, > > return -EPERM; > > if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) > > return -EBUSY; > > - if (cpu >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { > > - pr_devel("Out of bounds !\n"); > > - return -EINVAL; > > - } > > > > /* We need to synchronize with queue provisioning */ > > mutex_lock(&xive->lock); > > > > - vp_id = kvmppc_xive_vp(xive, cpu); > > - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > > - pr_devel("Duplicate !\n"); > > - r = -EEXIST; > > + r = kvmppc_xive_compute_vp_id(xive, cpu, &vp_id); > > + if (r) > > goto bail; > > - } > > > > xc = kzalloc(sizeof(*xc), GFP_KERNEL); > > if (!xc) { > > diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h > > index fe3ed50e0818..90cf6ec35a68 100644 > > --- a/arch/powerpc/kvm/book3s_xive.h > > +++ b/arch/powerpc/kvm/book3s_xive.h > > @@ -296,6 +296,7 @@ int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio, > > struct kvmppc_xive *kvmppc_xive_get_device(struct kvm *kvm, u32 type); > > void xive_cleanup_single_escalation(struct kvm_vcpu *vcpu, > > struct kvmppc_xive_vcpu *xc, int irq); > > +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp); > > > > #endif /* CONFIG_KVM_XICS */ > > #endif /* _KVM_PPC_BOOK3S_XICS_H */ > > diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c > > index 43a86858390a..5bb480b2aafd 100644 > > --- a/arch/powerpc/kvm/book3s_xive_native.c > > +++ b/arch/powerpc/kvm/book3s_xive_native.c > > @@ -118,19 +118,12 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, > > return -EPERM; > > if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) > > return -EBUSY; > > - if (server_num >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { > > - pr_devel("Out of bounds !\n"); > > - return -EINVAL; > > - } > > > > mutex_lock(&xive->lock); > > > > - vp_id = kvmppc_xive_vp(xive, server_num); > > - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { > > - pr_devel("Duplicate !\n"); > > - rc = -EEXIST; > > + rc = kvmppc_xive_compute_vp_id(xive, server_num, &vp_id); > > + if (rc) > > goto bail; > > - } > > > > xc = kzalloc(sizeof(*xc), GFP_KERNEL); > > if (!xc) { > > >
diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c index 0b7859e40f66..d84da9f6ee88 100644 --- a/arch/powerpc/kvm/book3s_xive.c +++ b/arch/powerpc/kvm/book3s_xive.c @@ -1211,6 +1211,37 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu) vcpu->arch.xive_vcpu = NULL; } +static bool kvmppc_xive_vcpu_id_valid(struct kvmppc_xive *xive, u32 cpu) +{ + /* We have a block of KVM_MAX_VCPUS VPs. We just need to check + * raw vCPU ids are below the expected limit for this guest's + * core stride ; kvmppc_pack_vcpu_id() will pack them down to an + * index that can be safely used to compute a VP id that belongs + * to the VP block. + */ + return cpu < KVM_MAX_VCPUS * xive->kvm->arch.emul_smt_mode; +} + +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp) +{ + u32 vp_id; + + if (!kvmppc_xive_vcpu_id_valid(xive, cpu)) { + pr_devel("Out of bounds !\n"); + return -EINVAL; + } + + vp_id = kvmppc_xive_vp(xive, cpu); + if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { + pr_devel("Duplicate !\n"); + return -EEXIST; + } + + *vp = vp_id; + + return 0; +} + int kvmppc_xive_connect_vcpu(struct kvm_device *dev, struct kvm_vcpu *vcpu, u32 cpu) { @@ -1229,20 +1260,13 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev, return -EPERM; if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) return -EBUSY; - if (cpu >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { - pr_devel("Out of bounds !\n"); - return -EINVAL; - } /* We need to synchronize with queue provisioning */ mutex_lock(&xive->lock); - vp_id = kvmppc_xive_vp(xive, cpu); - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { - pr_devel("Duplicate !\n"); - r = -EEXIST; + r = kvmppc_xive_compute_vp_id(xive, cpu, &vp_id); + if (r) goto bail; - } xc = kzalloc(sizeof(*xc), GFP_KERNEL); if (!xc) { diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h index fe3ed50e0818..90cf6ec35a68 100644 --- a/arch/powerpc/kvm/book3s_xive.h +++ b/arch/powerpc/kvm/book3s_xive.h @@ -296,6 +296,7 @@ int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio, struct kvmppc_xive *kvmppc_xive_get_device(struct kvm *kvm, u32 type); void xive_cleanup_single_escalation(struct kvm_vcpu *vcpu, struct kvmppc_xive_vcpu *xc, int irq); +int kvmppc_xive_compute_vp_id(struct kvmppc_xive *xive, u32 cpu, u32 *vp); #endif /* CONFIG_KVM_XICS */ #endif /* _KVM_PPC_BOOK3S_XICS_H */ diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c index 43a86858390a..5bb480b2aafd 100644 --- a/arch/powerpc/kvm/book3s_xive_native.c +++ b/arch/powerpc/kvm/book3s_xive_native.c @@ -118,19 +118,12 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, return -EPERM; if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT) return -EBUSY; - if (server_num >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) { - pr_devel("Out of bounds !\n"); - return -EINVAL; - } mutex_lock(&xive->lock); - vp_id = kvmppc_xive_vp(xive, server_num); - if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) { - pr_devel("Duplicate !\n"); - rc = -EEXIST; + rc = kvmppc_xive_compute_vp_id(xive, server_num, &vp_id); + if (rc) goto bail; - } xc = kzalloc(sizeof(*xc), GFP_KERNEL); if (!xc) {
Reduce code duplication by consolidating the checking of vCPU ids and VP ids to a common helper used by both legacy and native XIVE KVM devices. And explain the magic with a comment. Signed-off-by: Greg Kurz <groug@kaod.org> --- arch/powerpc/kvm/book3s_xive.c | 42 ++++++++++++++++++++++++++------- arch/powerpc/kvm/book3s_xive.h | 1 + arch/powerpc/kvm/book3s_xive_native.c | 11 ++------- 3 files changed, 36 insertions(+), 18 deletions(-)