On Tue, Aug 30, 2016 at 6:01 PM, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
vgic_attr_regs_access_v3() is used for handling both GICD/GICR and
Ok. I wil fix it. It is coming from v2 code.I think this should be KVM_DEV_ARM_VGIC_V3_MPIDR_MASKOn 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)
, 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 huh? it's an MPIDR, not a cpu id.
> #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-d evice.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;
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...
no, no, please no more auto-init at userspace access time. This code
> + 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;
should instead check vgic_initialized() and return an error to userspace
if not initialized.
> +
> + if (!lock_all_vcpus(dev->kvm)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: why is this assignment not predicated on if (is_write) ?
> + tmp32 = *reg;
also, all this type conversion nonsense is probably unnecessary, see my
comment below.
oh, but wait, you already had a 32-bit value, which you convert into a
> + 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);
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.
_______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
