[PATCH v4 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
Vijay Kilari
vijay.kilari at gmail.com
Mon Sep 12 02:00:28 PDT 2016
()
On Sun, Sep 11, 2016 at 1:26 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On Sat, 10 Sep 2016 17:52:17 +0530
> vijay.kilari at gmail.com wrote:
>
>> From: Vijaya Kumar K <Vijaya.Kumar at cavium.com>
>>
>> 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 version of VGIC v3 specification is define here
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>
>> Signed-off-by: Pavel Fedin <p.fedin at samsung.com>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar at cavium.com>
>> ---
>> arch/arm64/include/uapi/asm/kvm.h | 3 +
>> arch/arm64/kvm/Makefile | 1 +
>> include/linux/irqchip/arm-gic-v3.h | 32 ++++-
>> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 ++++
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 ---
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 18 +++
>> virt/kvm/arm/vgic/vgic-mmio.c | 16 +++
>> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 261 ++++++++++++++++++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic-v3.c | 4 +
>> virt/kvm/arm/vgic/vgic.h | 15 +++
>> 10 files changed, 376 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 56dc08d..91c7137 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_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_SYSREG_INSTR_MASK (0xffff)
>> #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_CPU_SYSREGS 6
>> +
>> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
>>
>> /* Device Control API on vcpu fd */
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index d50a82a..1a14e29 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 99ac022..22ec183 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -354,6 +354,24 @@
>> */
>> #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_CBPR_MASK (1 << ICC_CTLR_EL1_CBPR_SHIFT)
>> +#define ICC_CTLR_EL1_EOImode_SHIFT (1)
>
> Since you're adding this, please rewrite the two existing EOImode
> macros to use this new define.
>
>> +#define ICC_CTLR_EL1_EOImode_MASK (1 << ICC_CTLR_EL1_EOImode_SHIFT)
>> +#define ICC_CTLR_EL1_PRI_BITS_SHIFT (8)
>> +#define ICC_CTLR_EL1_PRI_BITS_MASK (0x7 << ICC_CTLR_EL1_PRI_BITS_SHIFT)
>> +#define ICC_CTLR_EL1_ID_BITS_SHIFT (11)
>> +#define ICC_CTLR_EL1_ID_BITS_MASK (0x7 << ICC_CTLR_EL1_ID_BITS_SHIFT)
>> +#define ICC_PMR_EL1_SHIFT (0)
>> +#define ICC_PMR_EL1_MASK (0xff << ICC_PMR_EL1_SHIFT)
>> +#define ICC_BPR0_EL1_SHIFT (0)
>> +#define ICC_BPR0_EL1_MASK (0x7 << ICC_PMR_EL1_SHIFT)
>> +#define ICC_BPR1_EL1_SHIFT (0)
>> +#define ICC_BPR1_EL1_MASK (0x7 << ICC_PMR_EL1_SHIFT)
>> +#define ICC_IGRPEN0_EL1_SHIFT (0)
>> +#define ICC_IGRPEN0_EL1_MASK (1 << ICC_IGRPEN0_EL1_SHIFT)
>> +#define ICC_IGRPEN1_EL1_SHIFT (0)
>> +#define ICC_IGRPEN1_EL1_MASK (1 << ICC_IGRPEN1_EL1_SHIFT)
>> #define ICC_SRE_EL1_SRE (1U << 0)
>>
>> /*
>> @@ -383,7 +401,19 @@
>> #define ICH_HCR_UIE (1 << 1)
>>
>> #define ICH_VMCR_CTLR_SHIFT 0
>> -#define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT)
>> +#define ICH_VMCR_CTLR_MASK (0x210 << ICH_VMCR_CTLR_SHIFT)
>
> Why are you dropping the four control bits? You're now only covering
> VEOIM and VCBPR. Worse, you don't even use that modified macro in this
> patch.
I modified this macro to hold only VEOIM and VCBPR fields.
For the rest of the fields VENG1 and VENG0, struct vmcr is added with
vmcr.grpen0 and vmcr.grpen1 separately.
>
>> +#define ICH_VMCR_CBPR_SHIFT 4
>> +#define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT)
>> +#define ICH_VMCR_EOIM_SHIFT 9
>> +#define ICH_VMCR_EOIM_MASK (1 << ICH_VMCR_EOIM_SHIFT)
>> +#define ICH_VMCR_ENG0_SHIFT 0
>> +#define ICH_VMCR_ENG0_MASK (1 << ICH_VMCR_ENG0_SHIFT)
>> +#define ICH_VMCR_ENG1_SHIFT 1
>> +#define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_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_BPR1_SHIFT 18
>> #define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)
>> #define ICH_VMCR_BPR0_SHIFT 21
>
> And here you're covering for all the bits. So what is now the purpose
> of ICH_VMCR_CTLR_MASK now?
OK. Can be replaced with CBPR and VEOIM macros.
>
> In general, I'd like this kind of change to be split from the rest of
> the patch so that it can be reviewed independently by the irqchip
> maintainers (tglx, Jason and myself).
>
OK. I will send separate patch for changing this macro and adding
vmcr.grpen0 and vmcr.grpen1 fields to struct vmcr
[...]
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>> @@ -0,0 +1,261 @@
>> +#include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_host.h>
>> +#include <kvm/iodev.h>
>> +#include <kvm/arm_vgic.h>
>> +#include <asm/kvm_emulate.h>
>> +#include <asm/kvm_arm.h>
>> +#include <asm/kvm_mmu.h>
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +#include "sys_regs.h"
>> +
>> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> + u64 val;
>> + u32 id_bits;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + val = p->regval;
>> + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>> + ICC_CTLR_EL1_CBPR_SHIFT) << ICH_VMCR_CBPR_SHIFT;
>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>> + ICC_CTLR_EL1_EOImode_SHIFT) << ICH_VMCR_EOIM_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>
> What if userspace writes something that is incompatible with the
> current configuration? Wrong number of ID bits, or number of priorities?
IDand PRI bits of ICC_CTLR_EL1 are read only. Not updated
>
>> + } else {
>> + val = 0;
>> + /* ICC_CTLR_EL1.A3V and ICC_CTRL_EL1.SEIS are not set */
>> + val |= VGIC_PRI_BITS << ICC_CTLR_EL1_PRI_BITS_SHIFT;
>
> Shouldn't that come from the actual HW?
Yes, want to expose only VGIC supported value instead of HW.
>
>> +
>> + if (vgic_has_its(vcpu->kvm))
>> + id_bits = INTERRUPT_ID_BITS_ITS;
>> + else
>> + id_bits = INTERRUPT_ID_BITS_SPIS;
>> +
>> + if (id_bits >= 24)
>> + val |= (1 << ICC_CTLR_EL1_ID_BITS_SHIFT);
>> + else
>> + val |= (0 << ICC_CTLR_EL1_ID_BITS_SHIFT);
>> +
>> + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
>> + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
>> + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
>> + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
>> +
>> + p->regval = val;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.pmr = (p->regval << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
>
> I don't get this. You're trying to extract a field from a register, and
> yet you're starting by shifting it *up* before masking it. This only
> works because your shift is 0. In general, I believe this should read:
>
> val = (regval & MASK) >> SHIFT;
>
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.pmr & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
>
> and this the other way around.
>
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.bpr = (p->regval << ICC_BPR0_EL1_SHIFT) &
>> + ICC_BPR0_EL1_MASK;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.bpr & ICC_BPR0_EL1_MASK) >>
>> + ICC_BPR0_EL1_SHIFT;
>> + }
>
> Same problems (and I'll stop commenting on this issue).
>
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.abpr = (p->regval << ICC_BPR1_EL1_SHIFT) &
>> + ICC_BPR1_EL1_MASK;
>
> nit: I'd prefer it if the binary points were called bpr0 and bpr1
> instead of bpr and abpr.
>
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.abpr & ICC_BPR1_EL1_MASK) >>
>> + ICC_BPR1_EL1_SHIFT;
>> + }
>
> Shouldn't this account for the ICC_CTLR_EL1.CBPR setting?
>
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.grpen0 = (p->regval << ICC_IGRPEN0_EL1_SHIFT) &
>> + ICC_IGRPEN0_EL1_MASK;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.grpen0 & ICC_IGRPEN0_EL1_MASK) >>
>> + ICC_IGRPEN0_EL1_SHIFT;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.grpen1 = (p->regval << ICC_IGRPEN1_EL1_SHIFT) &
>> + ICC_IGRPEN1_EL1_MASK;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.grpen1 & ICC_IGRPEN1_EL1_MASK) >>
>> + ICC_IGRPEN1_EL1_SHIFT;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu, 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->regval;
>
> What if some of the priority levels are not implemented? Restoring such
> an active priority will result in a VM that silently breaks.
You suggest to read vtr_to_nr_pri_bits() i.e ICH_VTR_EL2.PRIbits
and save/restore only required apr registers?
>>
>
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny.
More information about the linux-arm-kernel
mailing list