On Mon, 30 Sep 2019 14:01:56 +0200
Cédric Le Goater <clg@xxxxxxxx> 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@xxxxxxxx>
>
> 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) {
> >
>
