[PATCH v4 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
Marc Zyngier
marc.zyngier at arm.com
Mon Sep 12 02:12:54 PDT 2016
On 12/09/16 10:00, Vijay Kilari wrote:
> ()
>
> 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
Read again. You're migrating from a machine that implements 24 bits of
INTD to one that has 16 bits. Are you going to silently accept a bogus
value and leave most of the interrupts as undeliverable?
>
>>
>>> + } 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.
What does it mean? If the HW doesn't support that range of priority,
nothing will work anyway.
>
>>
>>> +
>>> + 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?
I strongly suggest that if you restore active priorities that are
outside of what the HW can represent, then the only possible value is zero.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
More information about the linux-arm-kernel
mailing list