* [PATCH 0/2] KVM: arm/arm64: vgic: Workaround GICC_PMR misreporting
@ 2017-03-16 11:45 ` Marc Zyngier
0 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw)
To: Christoffer Dall, Peter Maydell, Eric Auger, Andre Przywara
Cc: linux-arm-kernel, kvmarm, kvm
We have an annoying bug in the way we deal with GICC_PMR being
reported to userspace. When emulating a GICv2, we only report the top
5 bits (which is what the HW deals with) in an 8 bit field, completely
missing the left shift that would make it look like a normal value.
VM save/restore works just fine because we have the same issue on both
sides, and fixing it without any opt-in would break that migration.
This series implements a new attribute for the GICv2 emulation that
can be probed and set by userspace if it is interested in the real PMR
value. It also implements the reverse hack when emulating GICv2 on a
GICv3 host...
As this impacts userspace, I'm quite eager to have feedback from
people dealing with that side of the world.
Marc Zyngier (2):
KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to
userspace
KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour
arch/arm/include/uapi/asm/kvm.h | 4 ++--
arch/arm64/include/uapi/asm/kvm.h | 4 ++--
include/kvm/arm_vgic.h | 5 +++++
virt/kvm/arm/vgic/vgic-kvm-device.c | 5 +++++
virt/kvm/arm/vgic/vgic-v2.c | 16 ++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++
6 files changed, 51 insertions(+), 4 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 0/2] KVM: arm/arm64: vgic: Workaround GICC_PMR misreporting @ 2017-03-16 11:45 ` Marc Zyngier 0 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw) To: linux-arm-kernel We have an annoying bug in the way we deal with GICC_PMR being reported to userspace. When emulating a GICv2, we only report the top 5 bits (which is what the HW deals with) in an 8 bit field, completely missing the left shift that would make it look like a normal value. VM save/restore works just fine because we have the same issue on both sides, and fixing it without any opt-in would break that migration. This series implements a new attribute for the GICv2 emulation that can be probed and set by userspace if it is interested in the real PMR value. It also implements the reverse hack when emulating GICv2 on a GICv3 host... As this impacts userspace, I'm quite eager to have feedback from people dealing with that side of the world. Marc Zyngier (2): KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour arch/arm/include/uapi/asm/kvm.h | 4 ++-- arch/arm64/include/uapi/asm/kvm.h | 4 ++-- include/kvm/arm_vgic.h | 5 +++++ virt/kvm/arm/vgic/vgic-kvm-device.c | 5 +++++ virt/kvm/arm/vgic/vgic-v2.c | 16 ++++++++++++++++ virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++ 6 files changed, 51 insertions(+), 4 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-16 11:45 ` Marc Zyngier @ 2017-03-16 11:45 ` Marc Zyngier -1 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw) To: Christoffer Dall, Peter Maydell, Eric Auger, Andre Przywara Cc: linux-arm-kernel, kvmarm, kvm We allow userspace to save/restore the GICC_PMR values in order to allow migration. This value is extracted from GICH_PMCR, where it occupies a 5 bit field. But the canonical PMR is an 8 bit value and we fail to shift the virtual priority, resulting in a non-sensical value being reported to userspace. Fixing it once and for all would be ideal, but that would break migration of guest from old to new kernels. We thus introduce a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) that allows userspace to register its interest for the one true representation of PMR. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/include/uapi/asm/kvm.h | 4 ++-- arch/arm64/include/uapi/asm/kvm.h | 4 ++-- include/kvm/arm_vgic.h | 5 +++++ virt/kvm/arm/vgic/vgic-kvm-device.c | 5 +++++ virt/kvm/arm/vgic/vgic-v2.c | 16 ++++++++++++++++ 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index 6ebd3e6a1fd1..2852836221c0 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -189,6 +189,8 @@ struct kvm_arch_memory_slot { #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_CTRL_INIT 0 +#define KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR 1 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 @@ -198,8 +200,6 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff #define VGIC_LEVEL_INFO_LINE_LEVEL 0 -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 - /* KVM_IRQ_LINE irq field index values */ #define KVM_ARM_IRQ_TYPE_SHIFT 24 #define KVM_ARM_IRQ_TYPE_MASK 0xff diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index c2860358ae3e..e649dc753759 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -209,6 +209,8 @@ struct kvm_arch_memory_slot { #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_CTRL_INIT 0 +#define KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR 1 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 @@ -218,8 +220,6 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff #define VGIC_LEVEL_INFO_LINE_LEVEL 0 -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 - /* Device Control API on vcpu fd */ #define KVM_ARM_VCPU_PMU_V3_CTRL 0 #define KVM_ARM_VCPU_PMU_V3_IRQ 0 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index b72dd2ad5f44..af46ec3c427d 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -170,6 +170,8 @@ struct vgic_its { struct vgic_state_iter; +#define VGIC_QUIRK_CANONICAL_PMR 0 + struct vgic_dist { bool in_kernel; bool ready; @@ -218,6 +220,9 @@ struct vgic_dist { struct list_head lpi_list_head; int lpi_list_count; + /* Quirks driven by userspace requests */ + unsigned long quirks; + /* used by vgic-debug */ struct vgic_state_iter *iter; }; diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c index d181d2baee9c..b4935cd49c24 100644 --- a/virt/kvm/arm/vgic/vgic-kvm-device.c +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c @@ -160,6 +160,10 @@ static int vgic_set_common_attr(struct kvm_device *dev, r = vgic_init(dev->kvm); mutex_unlock(&dev->kvm->lock); return r; + case KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR: + set_bit(VGIC_QUIRK_CANONICAL_PMR, + &dev->kvm->arch.vgic.quirks); + break; } break; } @@ -408,6 +412,7 @@ static int vgic_v2_has_attr(struct kvm_device *dev, case KVM_DEV_ARM_VGIC_GRP_CTRL: switch (attr->attr) { case KVM_DEV_ARM_VGIC_CTRL_INIT: + case KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR: return 0; } } diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecdf3225..87f9dd1eaf1c 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,6 +191,15 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; + + /* + * If userspace is happy to deal with the normal PMR range (8 + * bits), we need to strip the 3 lowest bits so that we fit + * into the 5 bits that GICv2 gives us on the virtual side. + */ + if (test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr >>= 3; + vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & GICH_VMCR_PRIMASK_MASK; @@ -209,6 +218,13 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_BINPOINT_SHIFT; vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> GICH_VMCR_PRIMASK_SHIFT; + /* + * If userspace is happy to deal with the normal PMR range (8 + * bits), we need to shift the reduced range (5 bits) to + * expose it as if it was a normal value. + */ + if (test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr <<= 3; } void vgic_v2_enable(struct kvm_vcpu *vcpu) -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-16 11:45 ` Marc Zyngier 0 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw) To: linux-arm-kernel We allow userspace to save/restore the GICC_PMR values in order to allow migration. This value is extracted from GICH_PMCR, where it occupies a 5 bit field. But the canonical PMR is an 8 bit value and we fail to shift the virtual priority, resulting in a non-sensical value being reported to userspace. Fixing it once and for all would be ideal, but that would break migration of guest from old to new kernels. We thus introduce a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) that allows userspace to register its interest for the one true representation of PMR. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/include/uapi/asm/kvm.h | 4 ++-- arch/arm64/include/uapi/asm/kvm.h | 4 ++-- include/kvm/arm_vgic.h | 5 +++++ virt/kvm/arm/vgic/vgic-kvm-device.c | 5 +++++ virt/kvm/arm/vgic/vgic-v2.c | 16 ++++++++++++++++ 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index 6ebd3e6a1fd1..2852836221c0 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -189,6 +189,8 @@ struct kvm_arch_memory_slot { #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_CTRL_INIT 0 +#define KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR 1 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 @@ -198,8 +200,6 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff #define VGIC_LEVEL_INFO_LINE_LEVEL 0 -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 - /* KVM_IRQ_LINE irq field index values */ #define KVM_ARM_IRQ_TYPE_SHIFT 24 #define KVM_ARM_IRQ_TYPE_MASK 0xff diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index c2860358ae3e..e649dc753759 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -209,6 +209,8 @@ struct kvm_arch_memory_slot { #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_CTRL_INIT 0 +#define KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR 1 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 @@ -218,8 +220,6 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff #define VGIC_LEVEL_INFO_LINE_LEVEL 0 -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 - /* Device Control API on vcpu fd */ #define KVM_ARM_VCPU_PMU_V3_CTRL 0 #define KVM_ARM_VCPU_PMU_V3_IRQ 0 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index b72dd2ad5f44..af46ec3c427d 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -170,6 +170,8 @@ struct vgic_its { struct vgic_state_iter; +#define VGIC_QUIRK_CANONICAL_PMR 0 + struct vgic_dist { bool in_kernel; bool ready; @@ -218,6 +220,9 @@ struct vgic_dist { struct list_head lpi_list_head; int lpi_list_count; + /* Quirks driven by userspace requests */ + unsigned long quirks; + /* used by vgic-debug */ struct vgic_state_iter *iter; }; diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c index d181d2baee9c..b4935cd49c24 100644 --- a/virt/kvm/arm/vgic/vgic-kvm-device.c +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c @@ -160,6 +160,10 @@ static int vgic_set_common_attr(struct kvm_device *dev, r = vgic_init(dev->kvm); mutex_unlock(&dev->kvm->lock); return r; + case KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR: + set_bit(VGIC_QUIRK_CANONICAL_PMR, + &dev->kvm->arch.vgic.quirks); + break; } break; } @@ -408,6 +412,7 @@ static int vgic_v2_has_attr(struct kvm_device *dev, case KVM_DEV_ARM_VGIC_GRP_CTRL: switch (attr->attr) { case KVM_DEV_ARM_VGIC_CTRL_INIT: + case KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR: return 0; } } diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecdf3225..87f9dd1eaf1c 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,6 +191,15 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; + + /* + * If userspace is happy to deal with the normal PMR range (8 + * bits), we need to strip the 3 lowest bits so that we fit + * into the 5 bits that GICv2 gives us on the virtual side. + */ + if (test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr >>= 3; + vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & GICH_VMCR_PRIMASK_MASK; @@ -209,6 +218,13 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_BINPOINT_SHIFT; vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> GICH_VMCR_PRIMASK_SHIFT; + /* + * If userspace is happy to deal with the normal PMR range (8 + * bits), we need to shift the reduced range (5 bits) to + * expose it as if it was a normal value. + */ + if (test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr <<= 3; } void vgic_v2_enable(struct kvm_vcpu *vcpu) -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-16 11:45 ` Marc Zyngier @ 2017-03-20 14:24 ` Christoffer Dall -1 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 14:24 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > We allow userspace to save/restore the GICC_PMR values in order > to allow migration. This value is extracted from GICH_PMCR, where > it occupies a 5 bit field. But the canonical PMR is an 8 bit > value and we fail to shift the virtual priority, resulting in > a non-sensical value being reported to userspace. > > Fixing it once and for all would be ideal, but that would break > migration of guest from old to new kernels. We thus introduce > a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > that allows userspace to register its interest for the one true > representation of PMR. Thinking about this some more, I think we should just leave the ABI as is without introducing the flag, because we do not loose any information by doing so, and we can completely leave it up to userspace to work around our funny ABI. In the end, considering my comments on the next patch, the result would be amusing, and look something like this patch instead: diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 76e61c8..b2f60ca 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -83,6 +83,12 @@ Groups: Bits for undefined preemption levels are RAZ/WI. + For historical reasons and to provide ABI compatibility with userspace we + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask + field in the lower 5 bits of a word, meaning that userspace must always + use the lower 5 bits to communicate with the KVM device and must shift the + value left by 3 places to obtain the actual priority mask level. + Limitations: - Priorities are not implemented, and registers are RAZ/WI - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index a3ad7ff..7b7ecac 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, val = vmcr.ctlr; break; case GIC_CPU_PRIMASK: - val = vmcr.pmr; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * unsigned long. + */ + val = vmcr.pmr >> 3; break; case GIC_CPU_BINPOINT: val = vmcr.bpr; @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, vmcr.ctlr = val; break; case GIC_CPU_PRIMASK: - vmcr.pmr = val; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * unsigned long. + */ + vmcr.pmr = val << 3; break; case GIC_CPU_BINPOINT: vmcr.bpr = val; diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecd..95739cc 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & GICH_VMCR_PRIMASK_MASK; vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> GICH_VMCR_BINPOINT_SHIFT; - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> - GICH_VMCR_PRIMASK_SHIFT; + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> + GICH_VMCR_PRIMASK_SHIFT) << 3; } void vgic_v2_enable(struct kvm_vcpu *vcpu) diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index db28f7c..64b70b4 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -85,7 +85,8 @@ struct vgic_vmcr { u32 ctlr; u32 abpr; u32 bpr; - u32 pmr; + u32 pmr; /* Priority mask field in the GICC_PMR and + * ICC_PMR_EL1 priority field format */ /* Below member variable are valid only for GICv3 */ u32 grpen0; u32 grpen1; Let me know what you think. Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-20 14:24 ` Christoffer Dall 0 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 14:24 UTC (permalink / raw) To: linux-arm-kernel On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > We allow userspace to save/restore the GICC_PMR values in order > to allow migration. This value is extracted from GICH_PMCR, where > it occupies a 5 bit field. But the canonical PMR is an 8 bit > value and we fail to shift the virtual priority, resulting in > a non-sensical value being reported to userspace. > > Fixing it once and for all would be ideal, but that would break > migration of guest from old to new kernels. We thus introduce > a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > that allows userspace to register its interest for the one true > representation of PMR. Thinking about this some more, I think we should just leave the ABI as is without introducing the flag, because we do not loose any information by doing so, and we can completely leave it up to userspace to work around our funny ABI. In the end, considering my comments on the next patch, the result would be amusing, and look something like this patch instead: diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 76e61c8..b2f60ca 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -83,6 +83,12 @@ Groups: Bits for undefined preemption levels are RAZ/WI. + For historical reasons and to provide ABI compatibility with userspace we + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask + field in the lower 5 bits of a word, meaning that userspace must always + use the lower 5 bits to communicate with the KVM device and must shift the + value left by 3 places to obtain the actual priority mask level. + Limitations: - Priorities are not implemented, and registers are RAZ/WI - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index a3ad7ff..7b7ecac 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, val = vmcr.ctlr; break; case GIC_CPU_PRIMASK: - val = vmcr.pmr; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * unsigned long. + */ + val = vmcr.pmr >> 3; break; case GIC_CPU_BINPOINT: val = vmcr.bpr; @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, vmcr.ctlr = val; break; case GIC_CPU_PRIMASK: - vmcr.pmr = val; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * unsigned long. + */ + vmcr.pmr = val << 3; break; case GIC_CPU_BINPOINT: vmcr.bpr = val; diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecd..95739cc 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & GICH_VMCR_PRIMASK_MASK; vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> GICH_VMCR_BINPOINT_SHIFT; - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> - GICH_VMCR_PRIMASK_SHIFT; + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> + GICH_VMCR_PRIMASK_SHIFT) << 3; } void vgic_v2_enable(struct kvm_vcpu *vcpu) diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index db28f7c..64b70b4 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -85,7 +85,8 @@ struct vgic_vmcr { u32 ctlr; u32 abpr; u32 bpr; - u32 pmr; + u32 pmr; /* Priority mask field in the GICC_PMR and + * ICC_PMR_EL1 priority field format */ /* Below member variable are valid only for GICv3 */ u32 grpen0; u32 grpen1; Let me know what you think. Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-20 14:24 ` Christoffer Dall @ 2017-03-20 15:12 ` Marc Zyngier -1 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-20 15:12 UTC (permalink / raw) To: Christoffer Dall; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel On 20/03/17 14:24, Christoffer Dall wrote: > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: >> We allow userspace to save/restore the GICC_PMR values in order >> to allow migration. This value is extracted from GICH_PMCR, where >> it occupies a 5 bit field. But the canonical PMR is an 8 bit >> value and we fail to shift the virtual priority, resulting in >> a non-sensical value being reported to userspace. >> >> Fixing it once and for all would be ideal, but that would break >> migration of guest from old to new kernels. We thus introduce >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) >> that allows userspace to register its interest for the one true >> representation of PMR. > > Thinking about this some more, I think we should just leave the ABI as > is without introducing the flag, because we do not loose any information > by doing so, and we can completely leave it up to userspace to work > around our funny ABI. Right. That's the other option. Do we have any use case where we'd like to expose the real thing to userspace? This would impact migration from KVM to "something else", but I'm not sure we ever want to consider this seriously. > In the end, considering my comments on the next patch, the result would > be amusing, and look something like this patch instead: > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > index 76e61c8..b2f60ca 100644 > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > @@ -83,6 +83,12 @@ Groups: > > Bits for undefined preemption levels are RAZ/WI. > > + For historical reasons and to provide ABI compatibility with userspace we > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > + field in the lower 5 bits of a word, meaning that userspace must always > + use the lower 5 bits to communicate with the KVM device and must shift the > + value left by 3 places to obtain the actual priority mask level. > + > Limitations: > - Priorities are not implemented, and registers are RAZ/WI > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index a3ad7ff..7b7ecac 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > val = vmcr.ctlr; > break; > case GIC_CPU_PRIMASK: > - val = vmcr.pmr; > + /* > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > + * the PMR field as GICH_VMCR.VMPriMask rather than > + * GICC_PMR.Priority, so we expose the upper five bits of > + * priority mask to userspace using the lower bits in the > + * unsigned long. > + */ > + val = vmcr.pmr >> 3; > break; > case GIC_CPU_BINPOINT: > val = vmcr.bpr; > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > vmcr.ctlr = val; > break; > case GIC_CPU_PRIMASK: > - vmcr.pmr = val; > + /* > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > + * the PMR field as GICH_VMCR.VMPriMask rather than > + * GICC_PMR.Priority, so we expose the upper five bits of > + * priority mask to userspace using the lower bits in the > + * unsigned long. > + */ > + vmcr.pmr = val << 3; By just looking at the code, I understand that you have struct vmcr carrying PMR using its architectural representation? That's cunning indeed. > break; > case GIC_CPU_BINPOINT: > vmcr.bpr = val; > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index b834ecd..95739cc 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > GICH_VMCR_ALIAS_BINPOINT_MASK; > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > GICH_VMCR_BINPOINT_MASK; > - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & > + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & > GICH_VMCR_PRIMASK_MASK; > > vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; > @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > GICH_VMCR_BINPOINT_SHIFT; > - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> > - GICH_VMCR_PRIMASK_SHIFT; > + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> > + GICH_VMCR_PRIMASK_SHIFT) << 3; > } > > void vgic_v2_enable(struct kvm_vcpu *vcpu) > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index db28f7c..64b70b4 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -85,7 +85,8 @@ struct vgic_vmcr { > u32 ctlr; > u32 abpr; > u32 bpr; > - u32 pmr; > + u32 pmr; /* Priority mask field in the GICC_PMR and > + * ICC_PMR_EL1 priority field format */ > /* Below member variable are valid only for GICv3 */ > u32 grpen0; > u32 grpen1; > > > Let me know what you think. If everybody is happy with it, then so am I. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-20 15:12 ` Marc Zyngier 0 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-20 15:12 UTC (permalink / raw) To: linux-arm-kernel On 20/03/17 14:24, Christoffer Dall wrote: > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: >> We allow userspace to save/restore the GICC_PMR values in order >> to allow migration. This value is extracted from GICH_PMCR, where >> it occupies a 5 bit field. But the canonical PMR is an 8 bit >> value and we fail to shift the virtual priority, resulting in >> a non-sensical value being reported to userspace. >> >> Fixing it once and for all would be ideal, but that would break >> migration of guest from old to new kernels. We thus introduce >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) >> that allows userspace to register its interest for the one true >> representation of PMR. > > Thinking about this some more, I think we should just leave the ABI as > is without introducing the flag, because we do not loose any information > by doing so, and we can completely leave it up to userspace to work > around our funny ABI. Right. That's the other option. Do we have any use case where we'd like to expose the real thing to userspace? This would impact migration from KVM to "something else", but I'm not sure we ever want to consider this seriously. > In the end, considering my comments on the next patch, the result would > be amusing, and look something like this patch instead: > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > index 76e61c8..b2f60ca 100644 > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > @@ -83,6 +83,12 @@ Groups: > > Bits for undefined preemption levels are RAZ/WI. > > + For historical reasons and to provide ABI compatibility with userspace we > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > + field in the lower 5 bits of a word, meaning that userspace must always > + use the lower 5 bits to communicate with the KVM device and must shift the > + value left by 3 places to obtain the actual priority mask level. > + > Limitations: > - Priorities are not implemented, and registers are RAZ/WI > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index a3ad7ff..7b7ecac 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > val = vmcr.ctlr; > break; > case GIC_CPU_PRIMASK: > - val = vmcr.pmr; > + /* > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > + * the PMR field as GICH_VMCR.VMPriMask rather than > + * GICC_PMR.Priority, so we expose the upper five bits of > + * priority mask to userspace using the lower bits in the > + * unsigned long. > + */ > + val = vmcr.pmr >> 3; > break; > case GIC_CPU_BINPOINT: > val = vmcr.bpr; > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > vmcr.ctlr = val; > break; > case GIC_CPU_PRIMASK: > - vmcr.pmr = val; > + /* > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > + * the PMR field as GICH_VMCR.VMPriMask rather than > + * GICC_PMR.Priority, so we expose the upper five bits of > + * priority mask to userspace using the lower bits in the > + * unsigned long. > + */ > + vmcr.pmr = val << 3; By just looking@the code, I understand that you have struct vmcr carrying PMR using its architectural representation? That's cunning indeed. > break; > case GIC_CPU_BINPOINT: > vmcr.bpr = val; > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index b834ecd..95739cc 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > GICH_VMCR_ALIAS_BINPOINT_MASK; > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > GICH_VMCR_BINPOINT_MASK; > - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & > + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & > GICH_VMCR_PRIMASK_MASK; > > vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; > @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > GICH_VMCR_BINPOINT_SHIFT; > - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> > - GICH_VMCR_PRIMASK_SHIFT; > + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> > + GICH_VMCR_PRIMASK_SHIFT) << 3; > } > > void vgic_v2_enable(struct kvm_vcpu *vcpu) > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index db28f7c..64b70b4 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -85,7 +85,8 @@ struct vgic_vmcr { > u32 ctlr; > u32 abpr; > u32 bpr; > - u32 pmr; > + u32 pmr; /* Priority mask field in the GICC_PMR and > + * ICC_PMR_EL1 priority field format */ > /* Below member variable are valid only for GICv3 */ > u32 grpen0; > u32 grpen1; > > > Let me know what you think. If everybody is happy with it, then so am I. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-20 15:12 ` Marc Zyngier @ 2017-03-20 18:31 ` Christoffer Dall -1 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 18:31 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: > On 20/03/17 14:24, Christoffer Dall wrote: > > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > >> We allow userspace to save/restore the GICC_PMR values in order > >> to allow migration. This value is extracted from GICH_PMCR, where > >> it occupies a 5 bit field. But the canonical PMR is an 8 bit > >> value and we fail to shift the virtual priority, resulting in > >> a non-sensical value being reported to userspace. > >> > >> Fixing it once and for all would be ideal, but that would break > >> migration of guest from old to new kernels. We thus introduce > >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > >> that allows userspace to register its interest for the one true > >> representation of PMR. > > > > Thinking about this some more, I think we should just leave the ABI as > > is without introducing the flag, because we do not loose any information > > by doing so, and we can completely leave it up to userspace to work > > around our funny ABI. > > Right. That's the other option. Do we have any use case where we'd like > to expose the real thing to userspace? My stand here is that we *are* exposing the real thing - we just decided to use a funny format. If anything relied on the format being exported as reading the GICC_PMR directly, then their code would be already broken, and I don't think we care about supporting an already-broken non-functional userspace. The ABI is already what it is - not beautiful - but functionally just fine. > This would impact migration from > KVM to "something else", but I'm not sure we ever want to consider this > seriously. > I don't think it really impacts anything. For example, KVM to TCG will still work, it just requires userspace to do the wrangling of shifting the PMR 3 bits left and right, but it knows all about the versions it's dealing with etc. so that can be solved in userspace as well. And also, you're right, nobody is doing anything like this in userspace in the moment, so let's just clarify our bad ABI and declare success ;) > > In the end, considering my comments on the next patch, the result would > > be amusing, and look something like this patch instead: > > > > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > > index 76e61c8..b2f60ca 100644 > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > > @@ -83,6 +83,12 @@ Groups: > > > > Bits for undefined preemption levels are RAZ/WI. > > > > + For historical reasons and to provide ABI compatibility with userspace we > > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > > + field in the lower 5 bits of a word, meaning that userspace must always > > + use the lower 5 bits to communicate with the KVM device and must shift the > > + value left by 3 places to obtain the actual priority mask level. > > + > > Limitations: > > - Priorities are not implemented, and registers are RAZ/WI > > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > index a3ad7ff..7b7ecac 100644 > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > val = vmcr.ctlr; > > break; > > case GIC_CPU_PRIMASK: > > - val = vmcr.pmr; > > + /* > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > + * GICC_PMR.Priority, so we expose the upper five bits of > > + * priority mask to userspace using the lower bits in the > > + * unsigned long. > > + */ > > + val = vmcr.pmr >> 3; > > break; > > case GIC_CPU_BINPOINT: > > val = vmcr.bpr; > > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > vmcr.ctlr = val; > > break; > > case GIC_CPU_PRIMASK: > > - vmcr.pmr = val; > > + /* > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > + * GICC_PMR.Priority, so we expose the upper five bits of > > + * priority mask to userspace using the lower bits in the > > + * unsigned long. > > + */ > > + vmcr.pmr = val << 3; > > By just looking at the code, I understand that you have struct vmcr > carrying PMR using its architectural representation? That's cunning indeed. > Yeah, so that's the idea. My thought is that we either (a) don't use the intermediate struct vmcr representation for PMR at all, or (b) clearly define why we need to intermediate data structure and which format it should be in (the architectural one). If there's a better case for (a), we can do that too, but I found this one easily explainable with the comments I suggested. > > break; > > case GIC_CPU_BINPOINT: > > vmcr.bpr = val; > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > > index b834ecd..95739cc 100644 > > --- a/virt/kvm/arm/vgic/vgic-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-v2.c > > @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > GICH_VMCR_ALIAS_BINPOINT_MASK; > > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > > GICH_VMCR_BINPOINT_MASK; > > - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & > > + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & > > GICH_VMCR_PRIMASK_MASK; > > > > vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; > > @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > > GICH_VMCR_BINPOINT_SHIFT; > > - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> > > - GICH_VMCR_PRIMASK_SHIFT; > > + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> > > + GICH_VMCR_PRIMASK_SHIFT) << 3; > > } > > > > void vgic_v2_enable(struct kvm_vcpu *vcpu) > > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > > index db28f7c..64b70b4 100644 > > --- a/virt/kvm/arm/vgic/vgic.h > > +++ b/virt/kvm/arm/vgic/vgic.h > > @@ -85,7 +85,8 @@ struct vgic_vmcr { > > u32 ctlr; > > u32 abpr; > > u32 bpr; > > - u32 pmr; > > + u32 pmr; /* Priority mask field in the GICC_PMR and > > + * ICC_PMR_EL1 priority field format */ > > /* Below member variable are valid only for GICv3 */ > > u32 grpen0; > > u32 grpen1; > > > > > > Let me know what you think. > > If everybody is happy with it, then so am I. > Cool. Would you like me to send a proper patch, or do you prefer to take care of this one on your end? Thanks, -Christoffer ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-20 18:31 ` Christoffer Dall 0 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 18:31 UTC (permalink / raw) To: linux-arm-kernel On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: > On 20/03/17 14:24, Christoffer Dall wrote: > > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > >> We allow userspace to save/restore the GICC_PMR values in order > >> to allow migration. This value is extracted from GICH_PMCR, where > >> it occupies a 5 bit field. But the canonical PMR is an 8 bit > >> value and we fail to shift the virtual priority, resulting in > >> a non-sensical value being reported to userspace. > >> > >> Fixing it once and for all would be ideal, but that would break > >> migration of guest from old to new kernels. We thus introduce > >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > >> that allows userspace to register its interest for the one true > >> representation of PMR. > > > > Thinking about this some more, I think we should just leave the ABI as > > is without introducing the flag, because we do not loose any information > > by doing so, and we can completely leave it up to userspace to work > > around our funny ABI. > > Right. That's the other option. Do we have any use case where we'd like > to expose the real thing to userspace? My stand here is that we *are* exposing the real thing - we just decided to use a funny format. If anything relied on the format being exported as reading the GICC_PMR directly, then their code would be already broken, and I don't think we care about supporting an already-broken non-functional userspace. The ABI is already what it is - not beautiful - but functionally just fine. > This would impact migration from > KVM to "something else", but I'm not sure we ever want to consider this > seriously. > I don't think it really impacts anything. For example, KVM to TCG will still work, it just requires userspace to do the wrangling of shifting the PMR 3 bits left and right, but it knows all about the versions it's dealing with etc. so that can be solved in userspace as well. And also, you're right, nobody is doing anything like this in userspace in the moment, so let's just clarify our bad ABI and declare success ;) > > In the end, considering my comments on the next patch, the result would > > be amusing, and look something like this patch instead: > > > > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > > index 76e61c8..b2f60ca 100644 > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > > @@ -83,6 +83,12 @@ Groups: > > > > Bits for undefined preemption levels are RAZ/WI. > > > > + For historical reasons and to provide ABI compatibility with userspace we > > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > > + field in the lower 5 bits of a word, meaning that userspace must always > > + use the lower 5 bits to communicate with the KVM device and must shift the > > + value left by 3 places to obtain the actual priority mask level. > > + > > Limitations: > > - Priorities are not implemented, and registers are RAZ/WI > > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > index a3ad7ff..7b7ecac 100644 > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > val = vmcr.ctlr; > > break; > > case GIC_CPU_PRIMASK: > > - val = vmcr.pmr; > > + /* > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > + * GICC_PMR.Priority, so we expose the upper five bits of > > + * priority mask to userspace using the lower bits in the > > + * unsigned long. > > + */ > > + val = vmcr.pmr >> 3; > > break; > > case GIC_CPU_BINPOINT: > > val = vmcr.bpr; > > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > vmcr.ctlr = val; > > break; > > case GIC_CPU_PRIMASK: > > - vmcr.pmr = val; > > + /* > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > + * GICC_PMR.Priority, so we expose the upper five bits of > > + * priority mask to userspace using the lower bits in the > > + * unsigned long. > > + */ > > + vmcr.pmr = val << 3; > > By just looking at the code, I understand that you have struct vmcr > carrying PMR using its architectural representation? That's cunning indeed. > Yeah, so that's the idea. My thought is that we either (a) don't use the intermediate struct vmcr representation for PMR at all, or (b) clearly define why we need to intermediate data structure and which format it should be in (the architectural one). If there's a better case for (a), we can do that too, but I found this one easily explainable with the comments I suggested. > > break; > > case GIC_CPU_BINPOINT: > > vmcr.bpr = val; > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > > index b834ecd..95739cc 100644 > > --- a/virt/kvm/arm/vgic/vgic-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-v2.c > > @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > GICH_VMCR_ALIAS_BINPOINT_MASK; > > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > > GICH_VMCR_BINPOINT_MASK; > > - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & > > + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & > > GICH_VMCR_PRIMASK_MASK; > > > > vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; > > @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > > GICH_VMCR_BINPOINT_SHIFT; > > - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> > > - GICH_VMCR_PRIMASK_SHIFT; > > + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> > > + GICH_VMCR_PRIMASK_SHIFT) << 3; > > } > > > > void vgic_v2_enable(struct kvm_vcpu *vcpu) > > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > > index db28f7c..64b70b4 100644 > > --- a/virt/kvm/arm/vgic/vgic.h > > +++ b/virt/kvm/arm/vgic/vgic.h > > @@ -85,7 +85,8 @@ struct vgic_vmcr { > > u32 ctlr; > > u32 abpr; > > u32 bpr; > > - u32 pmr; > > + u32 pmr; /* Priority mask field in the GICC_PMR and > > + * ICC_PMR_EL1 priority field format */ > > /* Below member variable are valid only for GICv3 */ > > u32 grpen0; > > u32 grpen1; > > > > > > Let me know what you think. > > If everybody is happy with it, then so am I. > Cool. Would you like me to send a proper patch, or do you prefer to take care of this one on your end? Thanks, -Christoffer ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-20 18:31 ` Christoffer Dall @ 2017-03-20 18:55 ` Christoffer Dall -1 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 18:55 UTC (permalink / raw) To: Marc Zyngier Cc: Christoffer Dall, Peter Maydell, Eric Auger, Andre Przywara, linux-arm-kernel, kvmarm, kvm On Mon, Mar 20, 2017 at 07:31:29PM +0100, Christoffer Dall wrote: > On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: > > On 20/03/17 14:24, Christoffer Dall wrote: > > > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > > >> We allow userspace to save/restore the GICC_PMR values in order > > >> to allow migration. This value is extracted from GICH_PMCR, where > > >> it occupies a 5 bit field. But the canonical PMR is an 8 bit > > >> value and we fail to shift the virtual priority, resulting in > > >> a non-sensical value being reported to userspace. > > >> > > >> Fixing it once and for all would be ideal, but that would break > > >> migration of guest from old to new kernels. We thus introduce > > >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > > >> that allows userspace to register its interest for the one true > > >> representation of PMR. > > > > > > Thinking about this some more, I think we should just leave the ABI as > > > is without introducing the flag, because we do not loose any information > > > by doing so, and we can completely leave it up to userspace to work > > > around our funny ABI. > > > > Right. That's the other option. Do we have any use case where we'd like > > to expose the real thing to userspace? > > My stand here is that we *are* exposing the real thing - we just decided > to use a funny format. If anything relied on the format being exported > as reading the GICC_PMR directly, then their code would be already > broken, and I don't think we care about supporting an already-broken > non-functional userspace. The ABI is already what it is - not > beautiful - but functionally just fine. > > > > This would impact migration from > > KVM to "something else", but I'm not sure we ever want to consider this > > seriously. > > > > I don't think it really impacts anything. For example, KVM to TCG will > still work, it just requires userspace to do the wrangling of shifting > the PMR 3 bits left and right, but it knows all about the versions it's > dealing with etc. so that can be solved in userspace as well. > > And also, you're right, nobody is doing anything like this in userspace > in the moment, so let's just clarify our bad ABI and declare success ;) > > > > > > In the end, considering my comments on the next patch, the result would > > > be amusing, and look something like this patch instead: > > > > > > > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > index 76e61c8..b2f60ca 100644 > > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > @@ -83,6 +83,12 @@ Groups: > > > > > > Bits for undefined preemption levels are RAZ/WI. > > > > > > + For historical reasons and to provide ABI compatibility with userspace we > > > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > > > + field in the lower 5 bits of a word, meaning that userspace must always > > > + use the lower 5 bits to communicate with the KVM device and must shift the > > > + value left by 3 places to obtain the actual priority mask level. > > > + > > > Limitations: > > > - Priorities are not implemented, and registers are RAZ/WI > > > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > index a3ad7ff..7b7ecac 100644 > > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > > val = vmcr.ctlr; > > > break; > > > case GIC_CPU_PRIMASK: > > > - val = vmcr.pmr; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + val = vmcr.pmr >> 3; > > > break; > > > case GIC_CPU_BINPOINT: > > > val = vmcr.bpr; > > > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > > vmcr.ctlr = val; > > > break; > > > case GIC_CPU_PRIMASK: > > > - vmcr.pmr = val; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + vmcr.pmr = val << 3; > > > > By just looking at the code, I understand that you have struct vmcr > > carrying PMR using its architectural representation? That's cunning indeed. > > > > Yeah, so that's the idea. My thought is that we either (a) don't use > the intermediate struct vmcr representation for PMR at all, or (b) > clearly define why we need to intermediate data structure and which > format it should be in (the architectural one). > > If there's a better case for (a), we can do that too, but I found this > one easily explainable with the comments I suggested. > Just for reference, this is what the other solution looks like (untested, of course). I prefer my first version, but I don't feel strongly about it: diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 76e61c8..b2f60ca 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -83,6 +83,12 @@ Groups: Bits for undefined preemption levels are RAZ/WI. + For historical reasons and to provide ABI compatibility with userspace we + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask + field in the lower 5 bits of a word, meaning that userspace must always + use the lower 5 bits to communicate with the KVM device and must shift the + value left by 3 places to obtain the actual priority mask level. + Limitations: - Priorities are not implemented, and registers are RAZ/WI - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c index 79f37e3..5befc53 100644 --- a/arch/arm64/kvm/vgic-sys-reg-v3.c +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c @@ -95,14 +95,18 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - struct vgic_vmcr vmcr; + struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3; + u32 pmr; - vgic_get_vmcr(vcpu, &vmcr); if (p->is_write) { - vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; - vgic_set_vmcr(vcpu, &vmcr); + pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; + vgic_v3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK; + vgic_v3->vgic_vmcr |= (pmr << ICH_VMCR_PMR_SHIFT) + & ICH_VMCR_PMR_MASK; } else { - p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; + pmr = (vgic_v3->vgic_vmcr & ICH_VMCR_PMR_MASK) >> + ICH_VMCR_PMR_SHIFT; + p->regval = (pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; } return true; diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index a3ad7ff..41c413b 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -219,6 +219,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; u32 val; @@ -229,7 +230,15 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, val = vmcr.ctlr; break; case GIC_CPU_PRIMASK: - val = vmcr.pmr; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + val = (cpuif->vgic_vmcr & GICH_VMCR_PRIMASK_MASK) >> + GICH_VMCR_PRIMASK_SHIFT; break; case GIC_CPU_BINPOINT: val = vmcr.bpr; @@ -253,6 +262,7 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len, unsigned long val) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; vgic_get_vmcr(vcpu, &vmcr); @@ -262,7 +272,16 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, vmcr.ctlr = val; break; case GIC_CPU_PRIMASK: - vmcr.pmr = val; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + cpuif->vgic_vmcr &= ~GICH_VMCR_PRIMASK_MASK; + cpuif->vgic_vmcr |= (val << GICH_VMCR_PRIMASK_SHIFT) & + GICH_VMCR_PRIMASK_MASK; break; case GIC_CPU_BINPOINT: vmcr.bpr = val; diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecd..cc85bbf 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,8 +191,6 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & - GICH_VMCR_PRIMASK_MASK; vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; } @@ -207,8 +205,6 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> GICH_VMCR_BINPOINT_SHIFT; - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> - GICH_VMCR_PRIMASK_SHIFT; } void vgic_v2_enable(struct kvm_vcpu *vcpu) diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index edc6ee2..bcc5434 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -184,7 +184,6 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; - vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; @@ -204,7 +203,6 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; - vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index db28f7c..9886be3 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -85,7 +85,6 @@ struct vgic_vmcr { u32 ctlr; u32 abpr; u32 bpr; - u32 pmr; /* Below member variable are valid only for GICv3 */ u32 grpen0; u32 grpen1; Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-20 18:55 ` Christoffer Dall 0 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 18:55 UTC (permalink / raw) To: linux-arm-kernel On Mon, Mar 20, 2017 at 07:31:29PM +0100, Christoffer Dall wrote: > On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: > > On 20/03/17 14:24, Christoffer Dall wrote: > > > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > > >> We allow userspace to save/restore the GICC_PMR values in order > > >> to allow migration. This value is extracted from GICH_PMCR, where > > >> it occupies a 5 bit field. But the canonical PMR is an 8 bit > > >> value and we fail to shift the virtual priority, resulting in > > >> a non-sensical value being reported to userspace. > > >> > > >> Fixing it once and for all would be ideal, but that would break > > >> migration of guest from old to new kernels. We thus introduce > > >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > > >> that allows userspace to register its interest for the one true > > >> representation of PMR. > > > > > > Thinking about this some more, I think we should just leave the ABI as > > > is without introducing the flag, because we do not loose any information > > > by doing so, and we can completely leave it up to userspace to work > > > around our funny ABI. > > > > Right. That's the other option. Do we have any use case where we'd like > > to expose the real thing to userspace? > > My stand here is that we *are* exposing the real thing - we just decided > to use a funny format. If anything relied on the format being exported > as reading the GICC_PMR directly, then their code would be already > broken, and I don't think we care about supporting an already-broken > non-functional userspace. The ABI is already what it is - not > beautiful - but functionally just fine. > > > > This would impact migration from > > KVM to "something else", but I'm not sure we ever want to consider this > > seriously. > > > > I don't think it really impacts anything. For example, KVM to TCG will > still work, it just requires userspace to do the wrangling of shifting > the PMR 3 bits left and right, but it knows all about the versions it's > dealing with etc. so that can be solved in userspace as well. > > And also, you're right, nobody is doing anything like this in userspace > in the moment, so let's just clarify our bad ABI and declare success ;) > > > > > > In the end, considering my comments on the next patch, the result would > > > be amusing, and look something like this patch instead: > > > > > > > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > index 76e61c8..b2f60ca 100644 > > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > @@ -83,6 +83,12 @@ Groups: > > > > > > Bits for undefined preemption levels are RAZ/WI. > > > > > > + For historical reasons and to provide ABI compatibility with userspace we > > > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > > > + field in the lower 5 bits of a word, meaning that userspace must always > > > + use the lower 5 bits to communicate with the KVM device and must shift the > > > + value left by 3 places to obtain the actual priority mask level. > > > + > > > Limitations: > > > - Priorities are not implemented, and registers are RAZ/WI > > > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > index a3ad7ff..7b7ecac 100644 > > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > > val = vmcr.ctlr; > > > break; > > > case GIC_CPU_PRIMASK: > > > - val = vmcr.pmr; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + val = vmcr.pmr >> 3; > > > break; > > > case GIC_CPU_BINPOINT: > > > val = vmcr.bpr; > > > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > > vmcr.ctlr = val; > > > break; > > > case GIC_CPU_PRIMASK: > > > - vmcr.pmr = val; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + vmcr.pmr = val << 3; > > > > By just looking at the code, I understand that you have struct vmcr > > carrying PMR using its architectural representation? That's cunning indeed. > > > > Yeah, so that's the idea. My thought is that we either (a) don't use > the intermediate struct vmcr representation for PMR at all, or (b) > clearly define why we need to intermediate data structure and which > format it should be in (the architectural one). > > If there's a better case for (a), we can do that too, but I found this > one easily explainable with the comments I suggested. > Just for reference, this is what the other solution looks like (untested, of course). I prefer my first version, but I don't feel strongly about it: diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 76e61c8..b2f60ca 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -83,6 +83,12 @@ Groups: Bits for undefined preemption levels are RAZ/WI. + For historical reasons and to provide ABI compatibility with userspace we + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask + field in the lower 5 bits of a word, meaning that userspace must always + use the lower 5 bits to communicate with the KVM device and must shift the + value left by 3 places to obtain the actual priority mask level. + Limitations: - Priorities are not implemented, and registers are RAZ/WI - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c index 79f37e3..5befc53 100644 --- a/arch/arm64/kvm/vgic-sys-reg-v3.c +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c @@ -95,14 +95,18 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - struct vgic_vmcr vmcr; + struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3; + u32 pmr; - vgic_get_vmcr(vcpu, &vmcr); if (p->is_write) { - vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; - vgic_set_vmcr(vcpu, &vmcr); + pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; + vgic_v3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK; + vgic_v3->vgic_vmcr |= (pmr << ICH_VMCR_PMR_SHIFT) + & ICH_VMCR_PMR_MASK; } else { - p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; + pmr = (vgic_v3->vgic_vmcr & ICH_VMCR_PMR_MASK) >> + ICH_VMCR_PMR_SHIFT; + p->regval = (pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; } return true; diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index a3ad7ff..41c413b 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -219,6 +219,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; u32 val; @@ -229,7 +230,15 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, val = vmcr.ctlr; break; case GIC_CPU_PRIMASK: - val = vmcr.pmr; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + val = (cpuif->vgic_vmcr & GICH_VMCR_PRIMASK_MASK) >> + GICH_VMCR_PRIMASK_SHIFT; break; case GIC_CPU_BINPOINT: val = vmcr.bpr; @@ -253,6 +262,7 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len, unsigned long val) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; vgic_get_vmcr(vcpu, &vmcr); @@ -262,7 +272,16 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, vmcr.ctlr = val; break; case GIC_CPU_PRIMASK: - vmcr.pmr = val; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + cpuif->vgic_vmcr &= ~GICH_VMCR_PRIMASK_MASK; + cpuif->vgic_vmcr |= (val << GICH_VMCR_PRIMASK_SHIFT) & + GICH_VMCR_PRIMASK_MASK; break; case GIC_CPU_BINPOINT: vmcr.bpr = val; diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecd..cc85bbf 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,8 +191,6 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & - GICH_VMCR_PRIMASK_MASK; vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; } @@ -207,8 +205,6 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> GICH_VMCR_BINPOINT_SHIFT; - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> - GICH_VMCR_PRIMASK_SHIFT; } void vgic_v2_enable(struct kvm_vcpu *vcpu) diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index edc6ee2..bcc5434 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -184,7 +184,6 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; - vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; @@ -204,7 +203,6 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; - vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index db28f7c..9886be3 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -85,7 +85,6 @@ struct vgic_vmcr { u32 ctlr; u32 abpr; u32 bpr; - u32 pmr; /* Below member variable are valid only for GICv3 */ u32 grpen0; u32 grpen1; Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace 2017-03-20 18:31 ` Christoffer Dall @ 2017-03-20 19:03 ` Marc Zyngier -1 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-20 19:03 UTC (permalink / raw) To: Christoffer Dall; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel On 20/03/17 18:31, Christoffer Dall wrote: > On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: >> On 20/03/17 14:24, Christoffer Dall wrote: >>> On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: >>>> We allow userspace to save/restore the GICC_PMR values in order >>>> to allow migration. This value is extracted from GICH_PMCR, where >>>> it occupies a 5 bit field. But the canonical PMR is an 8 bit >>>> value and we fail to shift the virtual priority, resulting in >>>> a non-sensical value being reported to userspace. >>>> >>>> Fixing it once and for all would be ideal, but that would break >>>> migration of guest from old to new kernels. We thus introduce >>>> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) >>>> that allows userspace to register its interest for the one true >>>> representation of PMR. >>> >>> Thinking about this some more, I think we should just leave the ABI as >>> is without introducing the flag, because we do not loose any information >>> by doing so, and we can completely leave it up to userspace to work >>> around our funny ABI. >> >> Right. That's the other option. Do we have any use case where we'd like >> to expose the real thing to userspace? > > My stand here is that we *are* exposing the real thing - we just decided > to use a funny format. If anything relied on the format being exported > as reading the GICC_PMR directly, then their code would be already > broken, and I don't think we care about supporting an already-broken > non-functional userspace. The ABI is already what it is - not > beautiful - but functionally just fine. > > >> This would impact migration from >> KVM to "something else", but I'm not sure we ever want to consider this >> seriously. >> > > I don't think it really impacts anything. For example, KVM to TCG will > still work, it just requires userspace to do the wrangling of shifting > the PMR 3 bits left and right, but it knows all about the versions it's > dealing with etc. so that can be solved in userspace as well. > > And also, you're right, nobody is doing anything like this in userspace > in the moment, so let's just clarify our bad ABI and declare success ;) > > > >>> In the end, considering my comments on the next patch, the result would >>> be amusing, and look something like this patch instead: >>> >>> >>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt >>> index 76e61c8..b2f60ca 100644 >>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt >>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt >>> @@ -83,6 +83,12 @@ Groups: >>> >>> Bits for undefined preemption levels are RAZ/WI. >>> >>> + For historical reasons and to provide ABI compatibility with userspace we >>> + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask >>> + field in the lower 5 bits of a word, meaning that userspace must always >>> + use the lower 5 bits to communicate with the KVM device and must shift the >>> + value left by 3 places to obtain the actual priority mask level. >>> + >>> Limitations: >>> - Priorities are not implemented, and registers are RAZ/WI >>> - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> index a3ad7ff..7b7ecac 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >>> val = vmcr.ctlr; >>> break; >>> case GIC_CPU_PRIMASK: >>> - val = vmcr.pmr; >>> + /* >>> + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the >>> + * the PMR field as GICH_VMCR.VMPriMask rather than >>> + * GICC_PMR.Priority, so we expose the upper five bits of >>> + * priority mask to userspace using the lower bits in the >>> + * unsigned long. >>> + */ >>> + val = vmcr.pmr >> 3; >>> break; >>> case GIC_CPU_BINPOINT: >>> val = vmcr.bpr; >>> @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, >>> vmcr.ctlr = val; >>> break; >>> case GIC_CPU_PRIMASK: >>> - vmcr.pmr = val; >>> + /* >>> + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the >>> + * the PMR field as GICH_VMCR.VMPriMask rather than >>> + * GICC_PMR.Priority, so we expose the upper five bits of >>> + * priority mask to userspace using the lower bits in the >>> + * unsigned long. >>> + */ >>> + vmcr.pmr = val << 3; >> >> By just looking at the code, I understand that you have struct vmcr >> carrying PMR using its architectural representation? That's cunning indeed. >> > > Yeah, so that's the idea. My thought is that we either (a) don't use > the intermediate struct vmcr representation for PMR at all, or (b) > clearly define why we need to intermediate data structure and which > format it should be in (the architectural one). > > If there's a better case for (a), we can do that too, but I found this > one easily explainable with the comments I suggested. > >>> break; >>> case GIC_CPU_BINPOINT: >>> vmcr.bpr = val; >>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c >>> index b834ecd..95739cc 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-v2.c >>> @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> GICH_VMCR_ALIAS_BINPOINT_MASK; >>> vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & >>> GICH_VMCR_BINPOINT_MASK; >>> - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & >>> + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & >>> GICH_VMCR_PRIMASK_MASK; >>> >>> vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; >>> @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> GICH_VMCR_ALIAS_BINPOINT_SHIFT; >>> vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> >>> GICH_VMCR_BINPOINT_SHIFT; >>> - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> >>> - GICH_VMCR_PRIMASK_SHIFT; >>> + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> >>> + GICH_VMCR_PRIMASK_SHIFT) << 3; >>> } >>> >>> void vgic_v2_enable(struct kvm_vcpu *vcpu) >>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>> index db28f7c..64b70b4 100644 >>> --- a/virt/kvm/arm/vgic/vgic.h >>> +++ b/virt/kvm/arm/vgic/vgic.h >>> @@ -85,7 +85,8 @@ struct vgic_vmcr { >>> u32 ctlr; >>> u32 abpr; >>> u32 bpr; >>> - u32 pmr; >>> + u32 pmr; /* Priority mask field in the GICC_PMR and >>> + * ICC_PMR_EL1 priority field format */ >>> /* Below member variable are valid only for GICv3 */ >>> u32 grpen0; >>> u32 grpen1; >>> >>> >>> Let me know what you think. >> >> If everybody is happy with it, then so am I. >> > > > Cool. Would you like me to send a proper patch, or do you prefer to > take care of this one on your end? Please do send a proper patch (rather this one than your second version, which I found rather hard to read), and I'll apply it on top of the current set of fixes. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace @ 2017-03-20 19:03 ` Marc Zyngier 0 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-20 19:03 UTC (permalink / raw) To: linux-arm-kernel On 20/03/17 18:31, Christoffer Dall wrote: > On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: >> On 20/03/17 14:24, Christoffer Dall wrote: >>> On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: >>>> We allow userspace to save/restore the GICC_PMR values in order >>>> to allow migration. This value is extracted from GICH_PMCR, where >>>> it occupies a 5 bit field. But the canonical PMR is an 8 bit >>>> value and we fail to shift the virtual priority, resulting in >>>> a non-sensical value being reported to userspace. >>>> >>>> Fixing it once and for all would be ideal, but that would break >>>> migration of guest from old to new kernels. We thus introduce >>>> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) >>>> that allows userspace to register its interest for the one true >>>> representation of PMR. >>> >>> Thinking about this some more, I think we should just leave the ABI as >>> is without introducing the flag, because we do not loose any information >>> by doing so, and we can completely leave it up to userspace to work >>> around our funny ABI. >> >> Right. That's the other option. Do we have any use case where we'd like >> to expose the real thing to userspace? > > My stand here is that we *are* exposing the real thing - we just decided > to use a funny format. If anything relied on the format being exported > as reading the GICC_PMR directly, then their code would be already > broken, and I don't think we care about supporting an already-broken > non-functional userspace. The ABI is already what it is - not > beautiful - but functionally just fine. > > >> This would impact migration from >> KVM to "something else", but I'm not sure we ever want to consider this >> seriously. >> > > I don't think it really impacts anything. For example, KVM to TCG will > still work, it just requires userspace to do the wrangling of shifting > the PMR 3 bits left and right, but it knows all about the versions it's > dealing with etc. so that can be solved in userspace as well. > > And also, you're right, nobody is doing anything like this in userspace > in the moment, so let's just clarify our bad ABI and declare success ;) > > > >>> In the end, considering my comments on the next patch, the result would >>> be amusing, and look something like this patch instead: >>> >>> >>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt >>> index 76e61c8..b2f60ca 100644 >>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt >>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt >>> @@ -83,6 +83,12 @@ Groups: >>> >>> Bits for undefined preemption levels are RAZ/WI. >>> >>> + For historical reasons and to provide ABI compatibility with userspace we >>> + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask >>> + field in the lower 5 bits of a word, meaning that userspace must always >>> + use the lower 5 bits to communicate with the KVM device and must shift the >>> + value left by 3 places to obtain the actual priority mask level. >>> + >>> Limitations: >>> - Priorities are not implemented, and registers are RAZ/WI >>> - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> index a3ad7ff..7b7ecac 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >>> val = vmcr.ctlr; >>> break; >>> case GIC_CPU_PRIMASK: >>> - val = vmcr.pmr; >>> + /* >>> + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the >>> + * the PMR field as GICH_VMCR.VMPriMask rather than >>> + * GICC_PMR.Priority, so we expose the upper five bits of >>> + * priority mask to userspace using the lower bits in the >>> + * unsigned long. >>> + */ >>> + val = vmcr.pmr >> 3; >>> break; >>> case GIC_CPU_BINPOINT: >>> val = vmcr.bpr; >>> @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, >>> vmcr.ctlr = val; >>> break; >>> case GIC_CPU_PRIMASK: >>> - vmcr.pmr = val; >>> + /* >>> + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the >>> + * the PMR field as GICH_VMCR.VMPriMask rather than >>> + * GICC_PMR.Priority, so we expose the upper five bits of >>> + * priority mask to userspace using the lower bits in the >>> + * unsigned long. >>> + */ >>> + vmcr.pmr = val << 3; >> >> By just looking at the code, I understand that you have struct vmcr >> carrying PMR using its architectural representation? That's cunning indeed. >> > > Yeah, so that's the idea. My thought is that we either (a) don't use > the intermediate struct vmcr representation for PMR at all, or (b) > clearly define why we need to intermediate data structure and which > format it should be in (the architectural one). > > If there's a better case for (a), we can do that too, but I found this > one easily explainable with the comments I suggested. > >>> break; >>> case GIC_CPU_BINPOINT: >>> vmcr.bpr = val; >>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c >>> index b834ecd..95739cc 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-v2.c >>> @@ -191,7 +191,7 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> GICH_VMCR_ALIAS_BINPOINT_MASK; >>> vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & >>> GICH_VMCR_BINPOINT_MASK; >>> - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & >>> + vmcr |= ((vmcrp->pmr >> 3) << GICH_VMCR_PRIMASK_SHIFT) & >>> GICH_VMCR_PRIMASK_MASK; >>> >>> vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; >>> @@ -207,8 +207,8 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> GICH_VMCR_ALIAS_BINPOINT_SHIFT; >>> vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> >>> GICH_VMCR_BINPOINT_SHIFT; >>> - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> >>> - GICH_VMCR_PRIMASK_SHIFT; >>> + vmcrp->pmr = ((vmcr & GICH_VMCR_PRIMASK_MASK) >> >>> + GICH_VMCR_PRIMASK_SHIFT) << 3; >>> } >>> >>> void vgic_v2_enable(struct kvm_vcpu *vcpu) >>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>> index db28f7c..64b70b4 100644 >>> --- a/virt/kvm/arm/vgic/vgic.h >>> +++ b/virt/kvm/arm/vgic/vgic.h >>> @@ -85,7 +85,8 @@ struct vgic_vmcr { >>> u32 ctlr; >>> u32 abpr; >>> u32 bpr; >>> - u32 pmr; >>> + u32 pmr; /* Priority mask field in the GICC_PMR and >>> + * ICC_PMR_EL1 priority field format */ >>> /* Below member variable are valid only for GICv3 */ >>> u32 grpen0; >>> u32 grpen1; >>> >>> >>> Let me know what you think. >> >> If everybody is happy with it, then so am I. >> > > > Cool. Would you like me to send a proper patch, or do you prefer to > take care of this one on your end? Please do send a proper patch (rather this one than your second version, which I found rather hard to read), and I'll apply it on top of the current set of fixes. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour 2017-03-16 11:45 ` Marc Zyngier @ 2017-03-16 11:45 ` Marc Zyngier -1 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw) To: Christoffer Dall, Peter Maydell, Eric Auger, Andre Przywara Cc: linux-arm-kernel, kvmarm, kvm Similarily to the GICv2 case, we need to expose a PMR value that is either the 8bit value (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR being set) or the 5bit, truncated value otherwise. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index be0f4c3e0142..d260214a5bdb 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -184,6 +184,17 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; + + /* + * If we're emulating GICv2 and that userspace can't deal with + * the normal PMR range (8 bits), we need to make it GICv3 + * compatible by shifting the value by 3 bits in order to deal + * with the full 8bit range. + */ + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr <<= 3; + vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; @@ -205,6 +216,16 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; + + /* + * If we're emulating GICv2 and that userspace DOES NOT deal + * with the normal PMR range (8 bits), we need to reduce it to + * the GICv2 range (5 bits). + */ + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr >>= 3; + vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour @ 2017-03-16 11:45 ` Marc Zyngier 0 siblings, 0 replies; 18+ messages in thread From: Marc Zyngier @ 2017-03-16 11:45 UTC (permalink / raw) To: linux-arm-kernel Similarily to the GICv2 case, we need to expose a PMR value that is either the 8bit value (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR being set) or the 5bit, truncated value otherwise. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index be0f4c3e0142..d260214a5bdb 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -184,6 +184,17 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; + + /* + * If we're emulating GICv2 and that userspace can't deal with + * the normal PMR range (8 bits), we need to make it GICv3 + * compatible by shifting the value by 3 bits in order to deal + * with the full 8bit range. + */ + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr <<= 3; + vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; @@ -205,6 +216,16 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; + + /* + * If we're emulating GICv2 and that userspace DOES NOT deal + * with the normal PMR range (8 bits), we need to reduce it to + * the GICv2 range (5 bits). + */ + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) + vmcrp->pmr >>= 3; + vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour 2017-03-16 11:45 ` Marc Zyngier @ 2017-03-20 14:24 ` Christoffer Dall -1 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 14:24 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel On Thu, Mar 16, 2017 at 11:45:35AM +0000, Marc Zyngier wrote: > Similarily to the GICv2 case, we need to expose a PMR value that > is either the 8bit value (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR > being set) or the 5bit, truncated value otherwise. I'm a bit puzzled by this patch as well. The struct vmcr is an internal representation of the VMCR register fields without well-defined semantics of how the fields should be laid out. I think I would prefer it if we document on the vmcr struct which format the pmr field is in, and choose the actual ICC_PMR_EL1 or GICC_PMR format for this field, and then we leave the get/set_vmcr functions untouched. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index be0f4c3e0142..d260214a5bdb 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -184,6 +184,17 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; > vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; > + > + /* > + * If we're emulating GICv2 and that userspace can't deal with My comments above notwithstanding: s/that// > + * the normal PMR range (8 bits), we need to make it GICv3 > + * compatible by shifting the value by 3 bits in order to deal > + * with the full 8bit range. > + */ > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && > + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) > + vmcrp->pmr <<= 3; > + > vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; > vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; > vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; > @@ -205,6 +216,16 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; > vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; > vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; > + > + /* > + * If we're emulating GICv2 and that userspace DOES NOT deal s/that// nit: why do we write DOES NOT here i upper case (feels a bit aggressive)? > + * with the normal PMR range (8 bits), we need to reduce it to > + * the GICv2 range (5 bits). > + */ > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && > + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) > + vmcrp->pmr >>= 3; > + > vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; > vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; > } > -- > 2.11.0 > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour @ 2017-03-20 14:24 ` Christoffer Dall 0 siblings, 0 replies; 18+ messages in thread From: Christoffer Dall @ 2017-03-20 14:24 UTC (permalink / raw) To: linux-arm-kernel On Thu, Mar 16, 2017 at 11:45:35AM +0000, Marc Zyngier wrote: > Similarily to the GICv2 case, we need to expose a PMR value that > is either the 8bit value (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR > being set) or the 5bit, truncated value otherwise. I'm a bit puzzled by this patch as well. The struct vmcr is an internal representation of the VMCR register fields without well-defined semantics of how the fields should be laid out. I think I would prefer it if we document on the vmcr struct which format the pmr field is in, and choose the actual ICC_PMR_EL1 or GICC_PMR format for this field, and then we leave the get/set_vmcr functions untouched. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/vgic/vgic-v3.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index be0f4c3e0142..d260214a5bdb 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -184,6 +184,17 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; > vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; > + > + /* > + * If we're emulating GICv2 and that userspace can't deal with My comments above notwithstanding: s/that// > + * the normal PMR range (8 bits), we need to make it GICv3 > + * compatible by shifting the value by 3 bits in order to deal > + * with the full 8bit range. > + */ > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && > + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) > + vmcrp->pmr <<= 3; > + > vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; > vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; > vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; > @@ -205,6 +216,16 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; > vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; > vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; > + > + /* > + * If we're emulating GICv2 and that userspace DOES NOT deal s/that// nit: why do we write DOES NOT here i upper case (feels a bit aggressive)? > + * with the normal PMR range (8 bits), we need to reduce it to > + * the GICv2 range (5 bits). > + */ > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2 && > + !test_bit(VGIC_QUIRK_CANONICAL_PMR, &vcpu->kvm->arch.vgic.quirks)) > + vmcrp->pmr >>= 3; > + > vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; > vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; > } > -- > 2.11.0 > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-03-20 19:03 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-16 11:45 [PATCH 0/2] KVM: arm/arm64: vgic: Workaround GICC_PMR misreporting Marc Zyngier 2017-03-16 11:45 ` Marc Zyngier 2017-03-16 11:45 ` [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace Marc Zyngier 2017-03-16 11:45 ` Marc Zyngier 2017-03-20 14:24 ` Christoffer Dall 2017-03-20 14:24 ` Christoffer Dall 2017-03-20 15:12 ` Marc Zyngier 2017-03-20 15:12 ` Marc Zyngier 2017-03-20 18:31 ` Christoffer Dall 2017-03-20 18:31 ` Christoffer Dall 2017-03-20 18:55 ` Christoffer Dall 2017-03-20 18:55 ` Christoffer Dall 2017-03-20 19:03 ` Marc Zyngier 2017-03-20 19:03 ` Marc Zyngier 2017-03-16 11:45 ` [PATCH 2/2] KVM: arm/arm64: vgic-v3: Format PMR to mimic the GICv2 behaviour Marc Zyngier 2017-03-16 11:45 ` Marc Zyngier 2017-03-20 14:24 ` Christoffer Dall 2017-03-20 14:24 ` Christoffer Dall
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.