[PATCH v9 06/11] arm/arm64: vgic: Implement VGICv3 CPU interface access

On Tue, Nov 29, 2016 at 1:09 AM, Christoffer Dall
[off-list ref] wrote:

On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:

quoted

From: Vijaya Kumar K <redacted>

VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.

The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.

The CPU system register handling is spitted into two files

spitted?  Did you mean 'split into' ?

quoted

vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers

So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.

I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.

But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.

If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.

I suggest you move your:
  virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
  arch/arm64/kvm/vgic-sys-reg-v3.c

and rename
  virt/kvm/arm/vgic/vgic-sys-reg-common.c to
  virt/kvm/arm/vgic/vgic-sys-reg-v3.c

And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.

I will add arch/arm/kvm/vgic-coproc-v3.c (pls check if file name is ok or not?)
and return -ENXIO as shown below and update document accordingly.

int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
                               u64 *reg)
{
       /*
        * TODO: Implement for AArch32
        */
       return -ENXIO;
}

int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
                               u64 *reg)
{
       /*
        * TODO: Implement for AArch32
        */
       return -ENXIO;
}

quoted

for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.

Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.

The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt

Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---

[...]

quoted

+static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
+                         u8 idx, unsigned long *reg)
+{
+     struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+     /* num_pri_bits are initialized with HW supported values.
+      * We can rely safely on num_pri_bits even if VM has not
+      * restored ICC_CTLR_EL1 before restoring APnR registers.
+      */

nit: commenting style

ok

quoted

+     switch (vgic_v3_cpu->num_pri_bits) {
+     case 7:
+             vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
+             break;
+     case 6:
+             if (idx > 1)
+                     goto err;
+             vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
+             break;
+     default:
+             if (idx > 0)
+                     goto err;
+             vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
+     }

It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later.  Is that
not weird, or am I missing something?

As long as it is within HW supported priorities it is safe.

quoted

+
+     return true;
+err:
+     if (!is_write)
+             *reg = 0;
+
+     return false;
+}
+
+bool access_gic_ap0r_reg(struct kvm_vcpu *vcpu, bool is_write, u8 idx,
+                      unsigned long *reg)
+{
+     return access_gic_aprn(vcpu, is_write, 0, idx, reg);
+}
+
+bool access_gic_ap1r_reg(struct kvm_vcpu *vcpu, bool is_write, u8 idx,
+                      unsigned long *reg)
+{
+     return access_gic_aprn(vcpu, is_write, 1, idx, reg);
+}
+
+bool access_gic_sre_reg(struct kvm_vcpu *vcpu, bool is_write,
+                     unsigned long *reg)
+{
+     struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+     /* Validate SRE bit */
+     if (is_write) {
+             if (!(*reg & ICC_SRE_EL1_SRE))
+                     return false;
+     } else {
+             *reg = vgicv3->vgic_sre;
+     }
+
+     return true;
+}

diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
new file mode 100644
index 0000000..82c2f02
--- /dev/null
+++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c

@@ -0,0 +1,142 @@
+/*
+ * VGIC system registers handling functions
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <asm/kvm_emulate.h>
+#include "vgic.h"
+#include "sys_regs.h"
+
+#define ACCESS_SYS_REG(REG)                                          \
+static bool access_gic_##REG##_sys_reg(struct kvm_vcpu *vcpu,                \
+                                 struct sys_reg_params *p,           \
+                                 const struct sys_reg_desc *r)       \
+{                                                                    \
+     unsigned long tmp;                                              \
+     bool ret;                                                       \
+                                                                     \
+     if (p->is_write)                                                \
+             tmp = p->regval;                                        \
+     ret = access_gic_##REG##_reg(vcpu, p->is_write, &tmp);          \
+     if (!p->is_write)                                               \
+             p->regval = tmp;                                        \
+                                                                     \
+     return ret;                                                     \
+}
+
+ACCESS_SYS_REG(ctlr)
+ACCESS_SYS_REG(pmr)
+ACCESS_SYS_REG(bpr0)
+ACCESS_SYS_REG(bpr1)
+ACCESS_SYS_REG(sre)
+ACCESS_SYS_REG(grpen0)
+ACCESS_SYS_REG(grpen1)
+
+#define ACCESS_APNR_SYS_REG(REG)                                     \
+static bool access_gic_##REG##_sys_reg(struct kvm_vcpu *vcpu,                \
+                                 struct sys_reg_params *p,           \
+                                 const struct sys_reg_desc *r)       \
+{                                                                    \
+     unsigned long tmp;                                              \
+     u8 idx = p->Op2 & 3;                                            \
+     bool ret;                                                       \
+                                                                     \
+     if (p->is_write)                                                \
+             tmp = p->regval;                                        \
+     ret = access_gic_##REG##_reg(vcpu, p->is_write, idx, &tmp);     \
+     if (!p->is_write)                                               \
+             p->regval = tmp;                                        \
+                                                                     \
+     return ret;                                                     \
+}
+
+ACCESS_APNR_SYS_REG(ap0r)
+ACCESS_APNR_SYS_REG(ap1r)

I don't get these indirections.  Why can't you call the functions
directly?

The code is same for accessing the registers hence added this indirection.

quoted

+
+static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
+     /* ICC_PMR_EL1 */
+     { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr_sys_reg },
+     /* ICC_BPR0_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0_sys_reg },
+     /* ICC_AP0R0_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r_sys_reg },
+     /* ICC_AP0R1_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r_sys_reg },
+     /* ICC_AP0R2_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r_sys_reg },
+     /* ICC_AP0R3_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r_sys_reg },
+     /* ICC_AP1R0_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r_sys_reg },
+     /* ICC_AP1R1_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r_sys_reg },
+     /* ICC_AP1R2_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r_sys_reg },
+     /* ICC_AP1R3_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r_sys_reg },
+     /* ICC_BPR1_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1_sys_reg },
+     /* ICC_CTLR_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr_sys_reg },
+     /* ICC_SRE_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(5), access_gic_sre_sys_reg },
+     /* ICC_IGRPEN0_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0_sys_reg },
+     /* ICC_GRPEN1_EL1 */
+     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1_sys_reg },
+};
+
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+                             u64 *reg)
+{
+     struct sys_reg_params params;
+     u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
+
+     params.regval = *reg;
+     params.is_write = is_write;
+     params.is_aarch32 = false;
+     params.is_32bit = false;
+
+     if (find_reg_by_id(sysreg, &params, gic_v3_icc_reg_descs,
+                           ARRAY_SIZE(gic_v3_icc_reg_descs)))
+             return 0;
+
+     return -ENXIO;
+}
+
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+                             u64 *reg)
+{
+     struct sys_reg_params params;
+     const struct sys_reg_desc *r;
+     u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
+
+     if (is_write)
+             params.regval = *reg;
+     params.is_write = is_write;
+     params.is_aarch32 = false;
+     params.is_32bit = false;
+
+     r = find_reg_by_id(sysreg, &params, gic_v3_icc_reg_descs,
+                        ARRAY_SIZE(gic_v3_icc_reg_descs));
+     if (!r)
+             return -ENXIO;
+
+     if (!r->access(vcpu, &params, r))
+             return -EINVAL;
+
+     if (!is_write)
+             *reg = params.regval;
+
+     return 0;
+}

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index a3ff04b..6e7400e 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c

@@ -240,6 +240,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
              vgic_v3->vgic_sre = 0;
      }

+     vcpu->arch.vgic_cpu.num_id_bits = (kvm_vgic_global_state.ich_vtr_el2 &
+                                        ICH_VTR_ID_BITS_MASK) >>
+                                        ICH_VTR_ID_BITS_SHIFT;
+     vcpu->arch.vgic_cpu.num_pri_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
+                                         ICH_VTR_PRI_BITS_MASK) >>
+                                         ICH_VTR_PRI_BITS_SHIFT) + 1;
+
      /* Get the show on the road... */
      vgic_v3->vgic_hcr = ICH_HCR_EN;
 }

@@ -340,6 +347,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
       */
      kvm_vgic_global_state.nr_lr = (ich_vtr_el2 & 0xf) + 1;
      kvm_vgic_global_state.can_emulate_gicv2 = false;
+     kvm_vgic_global_state.ich_vtr_el2 = ich_vtr_el2;

      if (!info->vcpu.start) {
              kvm_info("GICv3: no GICV resource entry\n");

diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 9232791..af23399 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h

@@ -140,6 +140,28 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
                       int offset, u32 *val);
 int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
                       int offset, u32 *val);
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+                      u64 id, u64 *val);
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+                             u64 *reg);
+bool access_gic_ctlr_reg(struct kvm_vcpu *vcpu, bool is_write,
+                      unsigned long *reg);
+bool access_gic_pmr_reg(struct kvm_vcpu *vcpu, bool is_write,
+                     unsigned long *reg);
+bool access_gic_bpr0_reg(struct kvm_vcpu *vcpu, bool is_write,
+                      unsigned long *reg);
+bool access_gic_bpr1_reg(struct kvm_vcpu *vcpu, bool is_write,
+                      unsigned long *reg);
+bool access_gic_grpen0_reg(struct kvm_vcpu *vcpu, bool is_write,
+                        unsigned long *reg);
+bool access_gic_grpen1_reg(struct kvm_vcpu *vcpu, bool is_write,
+                        unsigned long *reg);
+bool access_gic_ap0r_reg(struct kvm_vcpu *vcpu, bool is_write,
+                      u8 idx, unsigned long *reg);
+bool access_gic_ap1r_reg(struct kvm_vcpu *vcpu, bool is_write,
+                      u8 idx, unsigned long *reg);
+bool access_gic_sre_reg(struct kvm_vcpu *vcpu, bool is_write,
+                     unsigned long *reg);

nit: since all of this is exported, I would name them vgic_access_ctlr()
and so on. The _reg postfix is probably also unnecessary for all of
these.

OK

quoted

 int kvm_register_vgic_device(unsigned long type);
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
--
1.9.1

Thanks,
-Christoffer