From: Andre Przywara <andre.przywara@arm.com>
To: Pavel Fedin <p.fedin@samsung.com>
Cc: Marc Zyngier <Marc.Zyngier@arm.com>,
"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
Date: Fri, 4 Sep 2015 16:13:44 +0100 [thread overview]
Message-ID: <55E9B528.50106@arm.com> (raw)
In-Reply-To: <20be17e4e8aebcd7f1a52a634f449955bef99996.1441370053.git.p.fedin@samsung.com>
Hi Pavel,
On 04/09/15 13:40, Pavel Fedin wrote:
> The access is done similar to GICv2, using KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> group, however attribute ID encodes corresponding system register. Access
> size is always 64 bits.
Why is this? Actually all registers in the CPU interface (except the w/o
SGI registers) are 32 bits and in the pending 32-bit GICv3 support
series[1] this is exploited by using MRC/MCR accesses.
The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
Aarch64, which always takes a x<nn> register.
So can you model the register size according to the spec and allow
32-bit accesses from userland?
> Since CPU interface state actually affects only a single vCPU, no vGIC
> locking is done. Just made sure that the vCPU is not running.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
> Documentation/virtual/kvm/devices/arm-vgic.txt | 38 +++-
> arch/arm64/include/uapi/asm/kvm.h | 7 +
> include/linux/irqchip/arm-gic-v3.h | 18 +-
> virt/kvm/arm/vgic-v3-emul.c | 244 +++++++++++++++++++++++++
> 4 files changed, 303 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index 03f640f..518b634 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -88,7 +88,7 @@ Groups:
> -ENODEV: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
>
> - KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> + KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv2)
> Attributes:
> The attr field of kvm_device_attr encodes two values:
> bits: | 63 .... 52 | 51 .. 32 | 31 .... 0 |
> @@ -116,11 +116,45 @@ Groups:
>
> Limitations:
> - Priorities are not implemented, and registers are RAZ/WI
> - - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> Errors:
> -ENODEV: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
>
> + KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv3)
> + Attributes:
> + The attr field of kvm_device_attr encodes the following values:
> + bits: | 63 .. 56 | 55 .. 48 | 47 ... 40 | 39 .. 32 | 31 .. 0 |
> + values: | arch | size | reserved | cpu id | reg id |
> +
> + All CPU interface regs are (rw, 64-bit). The only supported size value is
> + KVM_REG_SIZE_U64.
> +
> + Arch, size and reg id fields actually encode system register to be
> + accessed. Normally these values are obtained using ARM64_SYS_REG() macro.
> + Getting or setting such a register has the same effect as reading or
> + writing the register on the actual hardware.
> +
> + The Active Priorities Registers AP0Rn and AP1Rn are implementation defined,
> + so we set a fixed format for our implementation that fits with the model of
> + a "GICv3 implementation without the security extensions" which we present
> + to the guest. This interface always exposes four register APR[0-3]
> + describing the maximum possible 128 preemption levels. The semantics of the
> + register indicates if any interrupts in a given preemption level are in the
> + active state by setting the corresponding bit.
> +
> + Thus, preemption level X has one or more active interrupts if and only if:
> +
> + APRn[X mod 32] == 0b1, where n = X / 32
> +
> + Bits for undefined preemption levels are RAZ/WI.
> +
> + Limitations:
> + - Priorities are not implemented, and registers are RAZ/WI
> + Errors:
> + -ENODEV: Getting or setting this register is not yet supported
The code uses -ENXIO.
> + -EBUSY: One or more VCPUs are running
> +
> +
> KVM_DEV_ARM_VGIC_GRP_NR_IRQS
> Attributes:
> A value describing the number of interrupts (SGI, PPI and SPI) for
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 249954f..7d37ccd 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -201,6 +201,13 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_CPUID_MASK (0xfffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> #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_REG_MASK (KVM_REG_SIZE_MASK | \
> + KVM_REG_ARM64_SYSREG_OP0_MASK | \
> + KVM_REG_ARM64_SYSREG_OP1_MASK | \
> + KVM_REG_ARM64_SYSREG_CRN_MASK | \
> + KVM_REG_ARM64_SYSREG_CRM_MASK | \
> + KVM_REG_ARM64_SYSREG_OP2_MASK)
> +
> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 9eeeb95..dbc5c49 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -259,8 +259,14 @@
> /*
> * CPU interface registers
> */
> -#define ICC_CTLR_EL1_EOImode_drop_dir (0U << 1)
> -#define ICC_CTLR_EL1_EOImode_drop (1U << 1)
> +#define ICC_CTLR_EL1_CBPR_SHIFT 0
> +#define ICC_CTLR_EL1_EOImode_SHIFT 1
> +#define ICC_CTLR_EL1_EOImode_drop_dir (0U << ICC_CTLR_EL1_EOImode_SHIFT)
> +#define ICC_CTLR_EL1_EOImode_drop (1U << ICC_CTLR_EL1_EOImode_SHIFT)
> +#define ICC_CTLR_EL1_PRIbits_MASK (7U << 8)
> +#define ICC_CTLR_EL1_IDbits_MASK (7U << 11)
> +#define ICC_CTLR_EL1_SEIS (1U << 14)
> +#define ICC_CTLR_EL1_A3V (1U << 15)
> #define ICC_SRE_EL1_SRE (1U << 0)
>
> /*
> @@ -285,6 +291,14 @@
>
> #define ICH_VMCR_CTLR_SHIFT 0
> #define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT)
> +#define ICH_VMCR_ENG0_SHIFT 0
> +#define ICH_VMCR_ENG0 (1 << ICH_VMCR_ENG0_SHIFT)
> +#define ICH_VMCR_ENG1_SHIFT 1
> +#define ICH_VMCR_ENG1 (1 << ICH_VMCR_ENG1_SHIFT)
> +#define ICH_VMCR_CBPR_SHIFT 4
> +#define ICH_VMCR_CBPR (1 << ICH_VMCR_CBPR_SHIFT)
> +#define ICH_VMCR_EOIM_SHIFT 9
> +#define ICH_VMCR_EOIM (1 << ICH_VMCR_EOIM_SHIFT)
> #define ICH_VMCR_BPR1_SHIFT 18
> #define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)
> #define ICH_VMCR_BPR0_SHIFT 21
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> index 8bda714..2f1c27b 100644
> --- a/virt/kvm/arm/vgic-v3-emul.c
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -48,6 +48,7 @@
> #include <asm/kvm_arm.h>
> #include <asm/kvm_mmu.h>
>
> +#include "sys_regs.h"
> #include "vgic.h"
>
> static bool handle_mmio_rao_wi(struct kvm_vcpu *vcpu,
> @@ -991,6 +992,247 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> vgic_kick_vcpus(vcpu->kvm);
> }
>
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + u64 val;
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + val = *p->val;
> +
> + vgicv3->vgic_vmcr &= ~(ICH_VMCR_CBPR|ICH_VMCR_EOIM);
> + vgicv3->vgic_vmcr |= (val << (ICH_VMCR_CBPR_SHIFT -
> + ICC_CTLR_EL1_CBPR_SHIFT)) &
> + ICH_VMCR_CBPR;
> + vgicv3->vgic_vmcr |= (val << (ICH_VMCR_EOIM_SHIFT -
> + ICC_CTLR_EL1_EOImode_SHIFT)) &
> + ICH_VMCR_EOIM;
> + } else {
> + asm volatile("mrs_s %0," __stringify(ICC_IAR1_EL1)
> + : "=r" (val));
> + val &= (ICC_CTLR_EL1_A3V | ICC_CTLR_EL1_SEIS |
> + ICC_CTLR_EL1_IDbits_MASK | ICC_CTLR_EL1_PRIbits_MASK);
> + val |= (vgicv3->vgic_vmcr & ICH_VMCR_CBPR) >>
> + (ICH_VMCR_CBPR_SHIFT - ICC_CTLR_EL1_CBPR_SHIFT);
> + val |= (vgicv3->vgic_vmcr & ICH_VMCR_EOIM) >>
> + (ICH_VMCR_EOIM_SHIFT - ICC_CTLR_EL1_EOImode_SHIFT);
> +
> + *p->val = val;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_pmr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_PMR_SHIFT) &
> + ICH_VMCR_PMR_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
> + ICH_VMCR_PMR_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR0_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR0_SHIFT) &
> + ICH_VMCR_BPR0_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
> + ICH_VMCR_BPR0_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR1_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR1_SHIFT) &
> + ICH_VMCR_BPR1_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
> + ICH_VMCR_BPR1_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_ENG0_SHIFT) &
> + ICH_VMCR_ENG0;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> + ICH_VMCR_ENG0_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_ENG1_SHIFT) &
> + ICH_VMCR_ENG1;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> + ICH_VMCR_ENG1_SHIFT;
> + }
> +
> + return true;
> +}
I wonder if the 5 functions above could be merged to have only one
implementation and 5 wrappers, since they are so similar.
Cheers,
Andre.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/327034.html
> +
> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap0r[idx] = *p->val;
> + else
> + *p->val = vgicv3->vgic_ap0r[idx];
> +
> + return true;
> +}
> +
> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu,
> + const struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap1r[idx] = *p->val;
> + else
> + *p->val = vgicv3->vgic_ap1r[idx];
> +
> + return true;
> +}
> +
> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
> + /* ICC_PMR_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
> + access_gic_pmr },
> + /* ICC_BPR0_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
> + access_gic_bpr0 },
> + /* ICC_AP0R0_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b100),
> + access_gic_ap0r },
> + /* ICC_AP0R1_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b101),
> + access_gic_ap0r },
> + /* ICC_AP0R2_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b110),
> + access_gic_ap0r },
> + /* ICC_AP0R3_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b111),
> + access_gic_ap0r },
> + /* ICC_AP1R0_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b000),
> + access_gic_ap1r },
> + /* ICC_AP1R1_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b001),
> + access_gic_ap1r },
> + /* ICC_AP1R2_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b010),
> + access_gic_ap1r },
> + /* ICC_AP1R3_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b011),
> + access_gic_ap1r },
> + /* ICC_BPR1_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b011),
> + access_gic_bpr1 },
> + /* ICC_CTLR_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b100),
> + access_gic_ctlr },
> + /* ICC_IGRPEN0_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b110),
> + access_gic_grpen0 },
> + /* ICC_GRPEN1_EL1 */
> + { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b111),
> + access_gic_grpen1 },
> +};
> +
> +static int vgic_v3_cpu_regs_access(struct kvm_device *dev,
> + struct kvm_device_attr *attr,
> + bool is_write, int cpuid)
> +{
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 id = attr->attr & KVM_DEV_ARM_VGIC_REG_MASK;
> + struct kvm_vcpu *vcpu;
> + struct sys_reg_params params;
> + u_long reg;
> + const struct sys_reg_desc *r;
> +
> + params.val = ®
> + params.is_write = is_write;
> + params.is_aarch32 = false;
> + params.is_32bit = false;
> +
> + r = find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
> + ARRAY_SIZE(gic_v3_icc_reg_descs));
> + if (!r)
> + return -ENXIO;
> +
> + if (is_write) {
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> + }
> +
> + if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
> + return -EINVAL;
> +
> + vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> + /* Ensure that VCPU is not running */
> + if (unlikely(vcpu->cpu != -1))
> + return -EBUSY;
> +
> + if (!r->access(vcpu, ¶ms, r))
> + return -EINVAL;
> +
> + if (!is_write)
> + return put_user(reg, uaddr);
> +
> + return 0;
> +}
> +
> static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> bool is_write)
> @@ -1015,6 +1257,8 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> mmio.phys_addr = vgic->vgic_redist_base;
> ranges = vgic_redist_ranges;
> break;
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + return vgic_v3_cpu_regs_access(dev, attr, is_write, cpuid);
> default:
> return -ENXIO;
> }
> --
> 2.4.4
>
next prev parent reply other threads:[~2015-09-04 15:12 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 1/5] KVM: arm/arm64: Refactor vGIC attributes handling code Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace Pavel Fedin
2015-09-04 13:53 ` Andre Przywara
2015-09-04 15:22 ` Pavel Fedin
2015-09-07 7:56 ` Pavel Fedin
2015-09-09 11:28 ` Pavel Fedin
2015-09-07 8:50 ` Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 3/5] KVM: arm64: Refactor system register handlers Pavel Fedin
2015-09-04 15:11 ` Andre Przywara
2015-09-04 15:32 ` Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id() Pavel Fedin
2015-09-04 15:12 ` Andre Przywara
2015-09-04 12:40 ` [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access Pavel Fedin
2015-09-04 15:13 ` Andre Przywara [this message]
2015-09-04 15:40 ` Pavel Fedin
2015-09-24 12:08 ` Pavel Fedin
2015-09-25 22:27 ` Andre Przywara
2015-09-25 22:33 ` Peter Maydell
2015-09-25 23:00 ` Marc Zyngier
2015-09-28 15:29 ` Pavel Fedin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55E9B528.50106@arm.com \
--to=andre.przywara@arm.com \
--cc=Marc.Zyngier@arm.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=p.fedin@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.