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@xxxxxxxxx> wrote: > > > > > > On Tue, Aug 30, 2016 at 6:01 PM, Christoffer Dall > > <christoffer.dall@xxxxxxxxxx> wrote: > >> > >> On Wed, Aug 24, 2016 at 04:50:06PM +0530, vijay.kilari@xxxxxxxxx wrote: > >> > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx> > >> > > >> > 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@xxxxxxxxxx> > >> > --- > >> > 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, ®_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, ®, 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 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
