Re: [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access

[Christoffer Dall <christoffer.dall@linaro.org>]

On Tue, Sep 06, 2016 at 07:44:15PM +0530, Vijay Kilari wrote:
> Resending in plain text mode
> 
> On Tue, Sep 6, 2016 at 7:17 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> >
> >
> > On Tue, Aug 30, 2016 at 6:01 PM, Christoffer Dall
> > <christoffer.dall@linaro.org> wrote:
> >>
> >> On Wed, Aug 24, 2016 at 04:50:06PM +0530, vijay.kilari@gmail.com wrote:
> >> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> >
> >> > VGICv3 Distributor and Redistributor registers are accessed using
> >> > KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> >> > with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> >> > These registers are accessed as 32-bit and cpu mpidr
> >> > value passed along with register offset is used to identify the
> >> > cpu for redistributor registers access.
> >> >
> >> > The version of VGIC v3 specification is define here
> >> >
> >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >
> >> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> > ---
> >> >  arch/arm64/include/uapi/asm/kvm.h   |   3 +
> >> >  virt/kvm/arm/vgic/vgic-kvm-device.c | 127
> >> > +++++++++++++++++++++++++++++++++++-
> >> >  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 113
> >> > ++++++++++++++++++++++++++++++++
> >> >  virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
> >> >  virt/kvm/arm/vgic/vgic.h            |   8 +++
> >> >  5 files changed, 250 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/arch/arm64/include/uapi/asm/kvm.h
> >> > b/arch/arm64/include/uapi/asm/kvm.h
> >> > index 3051f86..94ea676 100644
> >> > --- a/arch/arm64/include/uapi/asm/kvm.h
> >> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> > @@ -201,10 +201,13 @@ struct kvm_arch_memory_slot {
> >> >  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS        2
> >> >  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT       32
> >> >  #define   KVM_DEV_ARM_VGIC_CPUID_MASK        (0xffULL <<
> >> > KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> >> > +#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
> >> > +                             (0xffffffffULL <<
> >> > KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> >>
> >> I think this should be KVM_DEV_ARM_VGIC_V3_MPIDR_MASK, and I would
> >> probably define a V3 of the shift as well, since we're really talking
> >> about two distinct APIs here.
> >>
> >> >  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT      0
> >> >  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK       (0xffffffffULL <<
> >> > KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >  #define KVM_DEV_ARM_VGIC_GRP_CTRL    4
> >> > +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >
> >> >  /* Device Control API on vcpu fd */
> >> > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> > b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> > index 163b057..06f0158 100644
> >> > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> > @@ -433,16 +433,136 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
> >> >
> >> >  #ifdef CONFIG_KVM_ARM_VGIC_V3
> >> >
> >> > +static int parse_vgic_v3_attr(struct kvm_device *dev,
> >> > +                           struct kvm_device_attr *attr,
> >> > +                           struct vgic_reg_attr *reg_attr)
> >> > +{
> >> > +     int cpuid;
> >> > +
> >> > +     cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> >> > +              KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> >> > +
> >> > +     if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
> >> > +             return -EINVAL;
> >>
> >> huh?  it's an MPIDR, not a cpu id.
> >>
> >> just resolve it with kvm_mpidr_to_vcpu and check its return value.
> >>
> >>
> >> > +
> >> > +     reg_attr->vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
> >>
> >> please check the return value of this function in any case...
> >>
> >> > +     reg_attr->addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> >> > +
> >> > +     return 0;
> >> > +}
> >> > +
> >> > +/**
> >> > + * vgic_attr_regs_access_v3 - allows user space to access VGIC v3 state
> >> > + *
> >> > + * @dev:      kvm device handle
> >> > + * @attr:     kvm device attribute
> >> > + * @reg:      address the value is read or written
> >> > + * @is_write: true if userspace is writing a register
> >> > + */
> >> > +static int vgic_attr_regs_access_v3(struct kvm_device *dev,
> >> > +                                 struct kvm_device_attr *attr,
> >> > +                                 u64 *reg, bool is_write)
> >> > +{
> >> > +     struct vgic_reg_attr reg_attr;
> >> > +     gpa_t addr;
> >> > +     struct kvm_vcpu *vcpu;
> >> > +     int ret;
> >> > +     u32 tmp32;
> >> > +
> >> > +     ret = parse_vgic_v3_attr(dev, attr, &reg_attr);
> >> > +     if (ret)
> >> > +             return ret;
> >> > +
> >> > +     vcpu = reg_attr.vcpu;
> >> > +     addr = reg_attr.addr;
> >> > +
> >> > +     mutex_lock(&dev->kvm->lock);
> >> > +
> >> > +     ret = vgic_init(dev->kvm);
> >> > +     if (ret)
> >> > +             goto out;
> >>
> >> no, no, please no more auto-init at userspace access time.  This code
> >> should instead check vgic_initialized() and return an error to userspace
> >> if not initialized.
> >>
> >> Ok. I wil fix it. It is coming from v2 code.

Next time, please fix your reply/indentation settings for replying to
patches.

> >
> >
> >>
> >> > +
> >> > +     if (!lock_all_vcpus(dev->kvm)) {
> >> > +             ret = -EBUSY;
> >> > +             goto out;
> >> > +     }
> >> > +
> >> > +     switch (attr->group) {
> >> > +     case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> >> > +             tmp32 = *reg;
> >>
> >> why is this assignment not predicated on if (is_write) ?
> >>
> >> also, all this type conversion nonsense is probably unnecessary, see my
> >> comment below.
> >>
> >> > +             ret = vgic_v3_dist_uaccess(vcpu, is_write, addr, &tmp32);
> >> > +             if (!is_write)
> >> > +                     *reg = tmp32;
> >> > +             break;
> >> > +     case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> >> > +             tmp32 = *reg;
> >> > +             ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
> >> > &tmp32);
> >> > +             if (!is_write)
> >> > +                     *reg = tmp32;
> >> > +             break;
> >> > +     default:
> >> > +             ret = -EINVAL;
> >> > +             break;
> >> > +     }
> >> > +
> >> > +     unlock_all_vcpus(dev->kvm);
> >> > +out:
> >> > +     mutex_unlock(&dev->kvm->lock);
> >> > +     return ret;
> >> > +}
> >> > +
> >> >  static int vgic_v3_set_attr(struct kvm_device *dev,
> >> >                           struct kvm_device_attr *attr)
> >> >  {
> >> > -     return vgic_set_common_attr(dev, attr);
> >> > +     int ret;
> >> > +
> >> > +     ret = vgic_set_common_attr(dev, attr);
> >> > +     if (ret != -ENXIO)
> >> > +             return ret;
> >> > +
> >> > +     switch (attr->group) {
> >> > +     case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> >> > +     case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> >> > +             u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> >> > +             u32 tmp32;
> >> > +             u64 reg;
> >> > +
> >> > +             if (get_user(tmp32, uaddr))
> >> > +                     return -EFAULT;
> >> > +
> >> > +             reg = tmp32;
> >> > +             return vgic_attr_regs_access_v3(dev, attr, &reg, true);
> >>
> >> oh, but wait, you already had a 32-bit value, which you convert into a
> >> 64-bit value, just to convert it back again, to do some extra data
> >> copies?
> >>
> >> I'm really confused as to why this has to be so complicated.
> >>
> >> Why not simply use u32 all the way?
> >
> >
> >     vgic_attr_regs_access_v3() is used for handling both GICD/GICR and
> > SYSREGS. For this reason we convert u32 to 64 and vice versa.
> > To avoid these conversions, I can create separate vgic_attr_regs_access_v3()
> > functions for GICD and SYSREGS.
> >
> >

Ah, I see, thanks for pointing that out.

If there's a cleaner way, perhaps by creating a wrapper, that would
be better, but we shouldn't duplicate the complicated function just to
avoid the cast.

Thanks,
-Christoffer