() On Sun, Sep 11, 2016 at 1:26 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > On Sat, 10 Sep 2016 17:52:17 +0530 > vijay.kilari@xxxxxxxxx wrote: > >> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx> >> >> 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@xxxxxxxxxxx> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx> >> --- >> 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. _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
