KVM: arm64: vgic: Collection of fixes for 6.13
Subsystems (3)
Marc Zyngier
maintainercover KVM: arm64: vgic: Collection of fixes for 6.13
This is a more or less random collection of vgic fixes, addressing issues ranging from "purely cosmetic" all the way to "pretty annoying" (of which patch #1 is an example).
The general theme is around the tightening of the internal API in order to make it a bit more difficult to get things wrong, something that I'm particularly good at. There is a bit more I'd like to do on the ITS, but I'm testing the waters here.
Patches on top of kvmarm-6.13, tested on the funky x1e box.
Marc Zyngier (4):
KVM: arm64: vgic-v3: Sanitise guest writes to GICR_INVLPIR
KVM: arm64: vgic: Make vgic_get_irq() more robust
KVM: arm64: vgic: Kill VGIC_MAX_PRIVATE definition
KVM: arm64: vgic-its: Add stronger type-checking to the ITS entry
sizes
arch/arm64/kvm/vgic/vgic-debug.c | 5 +- arch/arm64/kvm/vgic/vgic-init.c | 2 +- arch/arm64/kvm/vgic/vgic-its.c | 77 +++++++++++++++++++++--------- arch/arm64/kvm/vgic/vgic-mmio-v2.c | 12 ++--- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 13 +++-- arch/arm64/kvm/vgic/vgic-mmio.c | 38 +++++++-------- arch/arm64/kvm/vgic/vgic-v2.c | 2 +- arch/arm64/kvm/vgic/vgic-v3.c | 2 +- arch/arm64/kvm/vgic/vgic-v4.c | 4 +- arch/arm64/kvm/vgic/vgic.c | 43 ++++++++++------- arch/arm64/kvm/vgic/vgic.h | 27 +---------- include/kvm/arm_vgic.h | 1 - 12 files changed, 126 insertions(+), 100 deletions(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 1/4 KVM: arm64: vgic-v3: Sanitise guest writes to GICR_INVLPIR
Make sure we filter out non-LPI invalidation when handling writes to GICR_INVLPIR.
Fixes 4645d11f4a553 ("KVM: arm64: vgic-v3: Implement MMIO-based LPI invalidation")
Reported-by Alexander Potapenko <[email protected]>
Tested-by Alexander Potapenko <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
--- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/kvm/vgic/vgic-mmio-v3.c +6 -1
@@ -530,6 +530,7 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu, unsigned long val) {struct vgic_irq *irq; +u32 intid;/* * If the guest wrote only to the upper 32bit part of the
@@ -541,9 +542,13 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,if ((addr &4 ) || !vgic_lpis_enabled (vcpu))return ; + intid =lower_32_bits (val); +if (intid <VGIC_MIN_LPI ) +return ; +vgic_set_rdist_busy (vcpu, true); - irq =vgic_get_irq (vcpu->kvm,NULL ,lower_32_bits (val)); + irq =vgic_get_irq (vcpu->kvm,NULL , intid);if (irq) {vgic_its_inv_lpi (vcpu->kvm, irq);vgic_put_irq (vcpu->kvm, irq);
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 2/4 KVM: arm64: vgic: Make vgic_get_irq() more robust
In reply to Marc Zyngiervgic_get_irq() has an awkward signature, as it takes both a kvm *and* a vcpu, where the vcpu is allowed to be NULL if the INTID being looked up is a global interrupt (SPI or LPI).
This leads to potentially problematic situations where the INTID passed is a private interrupt, but that there is no vcpu.
In order to make things less ambiguous, let have *two* helpers instead:
- vgic_get_irq(struct kvm *kvm, u32 intid), which is only concerned with *global* interrupts, as indicated by the lack of vcpu.
- vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid), which can return *any* interrupt class, but must have of course a non-NULL vcpu.
Most of the code nicely falls under one or the other situations, except for a couple of cases (close to the UABI or in the debug code) where we have to distinguish between the two cases.
Signed-off-by Marc Zyngier <[email protected]>
--- arch/arm64/kvm/vgic/vgic-debug.c | 5 +++- arch/arm64/kvm/vgic/vgic-init.c | 2 +- arch/arm64/kvm/vgic/vgic-its.c | 8 +++--- arch/arm64/kvm/vgic/vgic-mmio-v2.c | 12 ++++----- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 8 +++--- arch/arm64/kvm/vgic/vgic-mmio.c | 38 +++++++++++++------------- arch/arm64/kvm/vgic/vgic-v2.c | 2 +- arch/arm64/kvm/vgic/vgic-v3.c | 2 +- arch/arm64/kvm/vgic/vgic-v4.c | 4 +-- arch/arm64/kvm/vgic/vgic.c | 43 +++++++++++++++++++----------- arch/arm64/kvm/vgic/vgic.h | 4 +-- 11 files changed, 71 insertions(+), 57 deletions(-)
arch/arm64/kvm/vgic/vgic-debug.c +4 -1
@@ -287,7 +287,10 @@ static int vgic_debug_show(struct seq_file *s, void *v)* Expect this to succeed, as iter_mark_lpis() takes a reference on * every LPI to be visited. */ - irq =vgic_get_irq (kvm, vcpu, iter->intid); +if (iter->intid <VGIC_NR_PRIVATE_IRQS ) + irq =vgic_get_vcpu_irq (vcpu, iter->intid); +else + irq =vgic_get_irq (kvm, iter->intid);if (WARN_ON_ONCE (!irq))return -EINVAL ;
arch/arm64/kvm/vgic/vgic-init.c +1 -1
@@ -322,7 +322,7 @@ int vgic_init(struct kvm *kvm) goto out;for (i =0 ; i <VGIC_NR_PRIVATE_IRQS ; i++) { -struct vgic_irq *irq =vgic_get_irq (kvm, vcpu, i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, i);switch (dist->vgic_model) {case KVM_DEV_TYPE_ARM_VGIC_V3 :
arch/arm64/kvm/vgic/vgic-its.c +4 -4
@@ -42,7 +42,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, struct kvm_vcpu *vcpu) {struct vgic_dist *dist = &kvm->arch.vgic; -struct vgic_irq *irq =vgic_get_irq (kvm,NULL , intid), *oldirq; +struct vgic_irq *irq =vgic_get_irq (kvm, intid), *oldirq;unsigned long flags;int ret;
@@ -419,7 +419,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) last_byte_offset = byte_offset; } - irq =vgic_get_irq (vcpu->kvm,NULL , intid); + irq =vgic_get_irq (vcpu->kvm, intid);if (!irq)continue ;
@@ -1288,7 +1288,7 @@ int vgic_its_invall(struct kvm_vcpu *vcpu)unsigned long intid;xa_for_each (&dist->lpi_xa, intid, irq) { - irq =vgic_get_irq (kvm,NULL , intid); + irq =vgic_get_irq (kvm, intid);if (!irq)continue ;
@@ -1354,7 +1354,7 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,return 0 ;xa_for_each (&dist->lpi_xa, intid, irq) { - irq =vgic_get_irq (kvm,NULL , intid); + irq =vgic_get_irq (kvm, intid);if (!irq)continue ;
arch/arm64/kvm/vgic/vgic-mmio-v2.c +6 -6
@@ -148,7 +148,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,if (!(targets & (1U << c)))continue ; - irq =vgic_get_irq (source_vcpu->kvm, vcpu, intid); + irq =vgic_get_vcpu_irq (vcpu, intid);raw_spin_lock_irqsave (&irq->irq_lock, flags); irq->pending_latch = true;
@@ -167,7 +167,7 @@ static unsigned long vgic_mmio_read_target(struct kvm_vcpu *vcpu,u64 val =0 ;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i); val |= (u64 )irq->targets << (i *8 );
@@ -191,7 +191,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,return ;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm,NULL , intid + i); +struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, intid + i);int target;raw_spin_lock_irqsave (&irq->irq_lock, flags);
@@ -213,7 +213,7 @@ static unsigned long vgic_mmio_read_sgipend(struct kvm_vcpu *vcpu,u64 val =0 ;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i); val |= (u64 )irq->source << (i *8 );
@@ -231,7 +231,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu *vcpu,unsigned long flags;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);
@@ -253,7 +253,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,unsigned long flags;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);
arch/arm64/kvm/vgic/vgic-mmio-v3.c +4 -4
@@ -194,7 +194,7 @@ static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { int intid = VGIC_ADDR_TO_INTID(addr, 64); -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm,NULL , intid); +struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, intid);unsigned long ret =0 ;if (!irq)
@@ -220,7 +220,7 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,if (addr &4 )return ; - irq =vgic_get_irq (vcpu->kvm,NULL , intid); + irq =vgic_get_irq (vcpu->kvm, intid);if (!irq)return ;
@@ -548,7 +548,7 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,vgic_set_rdist_busy (vcpu, true); - irq =vgic_get_irq (vcpu->kvm,NULL , intid); + irq =vgic_get_irq (vcpu->kvm, intid);if (irq) {vgic_its_inv_lpi (vcpu->kvm, irq);vgic_put_irq (vcpu->kvm, irq);
@@ -1025,7 +1025,7 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)static void vgic_v3_queue_sgi (struct kvm_vcpu *vcpu,u32 sgi,bool allow_group1) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, sgi); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, sgi);unsigned long flags;raw_spin_lock_irqsave (&irq->irq_lock, flags);
arch/arm64/kvm/vgic/vgic-mmio.c +19 -19
@@ -50,7 +50,7 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,/* Loop over all IRQs affected by this read */ for (i =0 ; i < len *8 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);if (irq->group) value |=BIT (i);
@@ -74,7 +74,7 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,unsigned long flags;for (i =0 ; i < len *8 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags); irq->group = !!(val &BIT (i));
@@ -102,7 +102,7 @@ unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,/* Loop over all IRQs affected by this read */ for (i =0 ; i < len *8 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);if (irq->enabled) value |= (1U << i);
@@ -122,7 +122,7 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);if (irq->hw &&vgic_irq_is_sgi (irq->intid)) {
@@ -171,7 +171,7 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);if (irq->hw &&vgic_irq_is_sgi (irq->intid) && irq->enabled)
@@ -193,7 +193,7 @@ int vgic_uaccess_write_senable(struct kvm_vcpu *vcpu,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags); irq->enabled = true;
@@ -214,7 +214,7 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags); irq->enabled = false;
@@ -236,7 +236,7 @@ static unsigned long __read_pending(struct kvm_vcpu *vcpu,/* Loop over all IRQs affected by this read */ for (i =0 ; i < len *8 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);unsigned long flags;bool val;
@@ -309,7 +309,7 @@ static void __set_pending(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);/* GICD_ISPENDR0 SGI bits are WI when written from the guest. */ if (is_vgic_v2_sgi (vcpu, irq) && !is_user) {
@@ -395,7 +395,7 @@ static void __clear_pending(struct kvm_vcpu *vcpu,unsigned long flags;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);/* GICD_ICPENDR0 SGI bits are WI when written from the guest. */ if (is_vgic_v2_sgi (vcpu, irq) && !is_user) {
@@ -494,7 +494,7 @@ static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,/* Loop over all IRQs affected by this read */ for (i =0 ; i < len *8 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);/* * Even for HW interrupts, don't evaluate the HW state as
@@ -598,7 +598,7 @@ static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,int i;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);vgic_mmio_change_active (vcpu, irq, false);vgic_put_irq (vcpu->kvm, irq); }
@@ -635,7 +635,7 @@ static void __vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,int i;for_each_set_bit (i, &val, len *8 ) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);vgic_mmio_change_active (vcpu, irq, true);vgic_put_irq (vcpu->kvm, irq); }
@@ -672,7 +672,7 @@ unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,u64 val =0 ;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i); val |= (u64 )irq->priority << (i *8 );
@@ -698,7 +698,7 @@ void vgic_mmio_write_priority(struct kvm_vcpu *vcpu,unsigned long flags;for (i =0 ; i < len; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);/* Narrow the priority range to what we actually support */
@@ -719,7 +719,7 @@ unsigned long vgic_mmio_read_config(struct kvm_vcpu *vcpu,int i;for (i =0 ; i < len *4 ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, intid + i);if (irq->config ==VGIC_CONFIG_EDGE ) value |= (2U << (i *2 ));
@@ -750,7 +750,7 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,if (intid + i <VGIC_NR_PRIVATE_IRQS )continue ; - irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); + irq =vgic_get_irq (vcpu->kvm, intid + i);raw_spin_lock_irqsave (&irq->irq_lock, flags);if (test_bit (i *2 +1 , &val))
@@ -775,7 +775,7 @@ u32 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid)if ((intid + i) <VGIC_NR_SGIS || (intid + i) >= nr_irqs)continue ; - irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); + irq =vgic_get_vcpu_irq (vcpu, intid + i);if (irq->config ==VGIC_CONFIG_LEVEL && irq->line_level) val |= (1U << i);
@@ -799,7 +799,7 @@ void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,if ((intid + i) <VGIC_NR_SGIS || (intid + i) >= nr_irqs)continue ; - irq =vgic_get_irq (vcpu->kvm, vcpu, intid + i); + irq =vgic_get_vcpu_irq (vcpu, intid + i);/* * Line level is set irrespective of irq type
arch/arm64/kvm/vgic/vgic-v2.c +1 -1
@@ -72,7 +72,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)kvm_notify_acked_irq (vcpu->kvm,0 , intid -VGIC_NR_PRIVATE_IRQS ); - irq =vgic_get_irq (vcpu->kvm, vcpu, intid); + irq =vgic_get_vcpu_irq (vcpu, intid);raw_spin_lock (&irq->irq_lock);
arch/arm64/kvm/vgic/vgic-v3.c +1 -1
@@ -65,7 +65,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)kvm_notify_acked_irq (vcpu->kvm,0 , intid -VGIC_NR_PRIVATE_IRQS ); - irq =vgic_get_irq (vcpu->kvm, vcpu, intid); + irq =vgic_get_vcpu_irq (vcpu, intid);if (!irq)/* An LPI could have been unmapped. */ continue ;
arch/arm64/kvm/vgic/vgic-v4.c +2 -2
@@ -123,7 +123,7 @@ static void vgic_v4_enable_vsgis(struct kvm_vcpu *vcpu)* IRQ. The SGI code will do its magic. */ for (i =0 ; i <VGIC_NR_SGIS ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, i);struct irq_desc *desc;unsigned long flags;int ret;
@@ -160,7 +160,7 @@ static void vgic_v4_disable_vsgis(struct kvm_vcpu *vcpu)int i;for (i =0 ; i <VGIC_NR_SGIS ; i++) { -struct vgic_irq *irq =vgic_get_irq (vcpu->kvm, vcpu, i); +struct vgic_irq *irq =vgic_get_vcpu_irq (vcpu, i);struct irq_desc *desc;unsigned long flags;int ret;
arch/arm64/kvm/vgic/vgic.c +27 -16
99 lines not on this page · raw message
arch/arm64/kvm/vgic/vgic.h +2 -2
@@ -202,8 +202,8 @@ int vgic_v2_parse_attr(struct kvm_device *dev, struct kvm_device_attr *attr,const struct vgic_register_region *vgic_get_mmio_region (struct kvm_vcpu *vcpu,struct vgic_io_device *iodev,gpa_t addr,int len); -struct vgic_irq *vgic_get_irq (struct kvm *kvm,struct kvm_vcpu *vcpu, -u32 intid); +struct vgic_irq *vgic_get_irq (struct kvm *kvm,u32 intid); +struct vgic_irq *vgic_get_vcpu_irq (struct kvm_vcpu *vcpu,u32 intid);void vgic_put_irq (struct kvm *kvm,struct vgic_irq *irq);bool vgic_get_phys_line_level (struct vgic_irq *irq);void vgic_irq_set_phys_pending (struct vgic_irq *irq,bool pending);
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 3/4 KVM: arm64: vgic: Kill VGIC_MAX_PRIVATE definition
In reply to Marc ZyngierVGIC_MAX_PRIVATE is a pretty useless definition, and is better replaced with VGIC_NR_PRIVATE_IRQS.
Signed-off-by Marc Zyngier <[email protected]>
--- arch/arm64/kvm/vgic/vgic.c | 4 ++-- include/kvm/arm_vgic.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)
arch/arm64/kvm/vgic/vgic.c +2 -2
@@ -106,8 +106,8 @@ struct vgic_irq *vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid)return NULL ;/* SGIs and PPIs */ -if (intid <=VGIC_MAX_PRIVATE ) { - intid =array_index_nospec (intid,VGIC_MAX_PRIVATE +1 ); +if (intid <VGIC_NR_PRIVATE_IRQS ) { + intid =array_index_nospec (intid,VGIC_NR_PRIVATE_IRQS );return &vcpu->arch.vgic_cpu.private_irqs[intid]; }
include/kvm/arm_vgic.h +0 -1
@@ -26,7 +26,6 @@#define VGIC_NR_SGIS 16#define VGIC_NR_PPIS 16#define VGIC_NR_PRIVATE_IRQS (VGIC_NR_SGIS + VGIC_NR_PPIS) -#define VGIC_MAX_PRIVATE (VGIC_NR_PRIVATE_IRQS - 1)#define VGIC_MAX_SPI 1019#define VGIC_MAX_RESERVED 1023#define VGIC_MIN_LPI 8192
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 4/4 KVM: arm64: vgic-its: Add stronger type-checking to the ITS entry sizes
In reply to Marc ZyngierThe ITS ABI infrastructure allows for some pretty lax code, where the size of the data doesn't have to match the size of the entry, potentially leading to a collection of interesting bugs.
Commit 7fe28d7e68f9 ("KVM: arm64: vgic-its: Add a data length check in vgic_its_save_*") added some checks, but starts by implicitly casting all writes to a 64bit value, hiding some of the issues.
Instead, introduce macros that will check the data type actually used for dealing with the table entries. The macros are taking a symbolic entry type that is used to fetch the size of the entry type for the current ABI. This immediately catches a couple of low-impact gotchas (zero values that are implicitly 32bit), easy enough to fix.
Given that we currently only have a single ABI, hardcode a couple of BUILD_BUG_ON()s that will fire if we use anything but a 64bit quantity, and some (currently unreachable) fallback code that may become useful one day.
Signed-off-by Marc Zyngier <[email protected]>
--- arch/arm64/kvm/vgic/vgic-its.c | 69 ++++++++++++++++++++++++---------- arch/arm64/kvm/vgic/vgic.h | 23 ------------ 2 files changed, 50 insertions(+), 42 deletions(-)
arch/arm64/kvm/vgic/vgic-its.c +50 -19
@@ -31,6 +31,41 @@ static int vgic_its_commit_v0(struct vgic_its *its);static int update_lpi_config (struct kvm *kvm,struct vgic_irq *irq,struct kvm_vcpu *filter_vcpu,bool needs_inv); +#define vgic_its_read_entry_lock (i, g, valp, t) \ + ({ \ + int __sz = vgic_its_get_abi(i)->t##_esz; \ + struct kvm *__k = (i)->dev->kvm; \ + int __ret; \ + \ + BUILD_BUG_ON(NR_ITS_ABIS == 1 && \ + sizeof(*(valp)) != ABI_0_ESZ); \ + if (NR_ITS_ABIS > 1 && \ + KVM_BUG_ON(__sz != sizeof(*(valp)), __k)) \ + __ret = -EINVAL; \ + else \ + __ret = kvm_read_guest_lock(__k, (g), \ + valp, __sz); \ + __ret; \ + }) + +#define vgic_its_write_entry_lock (i, g, val, t) \ + ({ \ + int __sz = vgic_its_get_abi(i)->t##_esz; \ + struct kvm *__k = (i)->dev->kvm; \ + typeof(val) __v = (val); \ + int __ret; \ + \ + BUILD_BUG_ON(NR_ITS_ABIS == 1 && \ + sizeof(__v) != ABI_0_ESZ); \ + if (NR_ITS_ABIS > 1 && \ + KVM_BUG_ON(__sz != sizeof(__v), __k)) \ + __ret = -EINVAL; \ + else \ + __ret = vgic_write_guest_lock(__k, (g), \ + &__v, __sz); \ + __ret; \ + }) +/* * Creates a new (reference to a) struct vgic_irq for a given LPI. * If this LPI is already mapped on another ITS, we increase its refcount
@@ -794,7 +829,7 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,its_free_ite (kvm, ite); -return vgic_its_write_entry_lock (its, gpa,0 , ite_esz); +return vgic_its_write_entry_lock (its, gpa,0ULL , ite); }return E_ITS_DISCARD_UNMAPPED_INTERRUPT ;
@@ -1143,7 +1178,6 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,bool valid =its_cmd_get_validbit (its_cmd);u8 num_eventid_bits =its_cmd_get_size (its_cmd);gpa_t itt_addr =its_cmd_get_ittaddr (its_cmd); -int dte_esz =vgic_its_get_abi (its)->dte_esz;struct its_device *device;gpa_t gpa;
@@ -1168,7 +1202,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,* is an error, so we are done in any case. */ if (!valid) -return vgic_its_write_entry_lock (its, gpa,0 , dte_esz); +return vgic_its_write_entry_lock (its, gpa,0ULL , dte); device =vgic_its_alloc_device (its, device_id, itt_addr, num_eventid_bits);
@@ -2090,7 +2124,7 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, u32 esz,* vgic_its_save_ite - Save an interrupt translation entry at @gpa */ static int vgic_its_save_ite (struct vgic_its *its,struct its_device *dev, -struct its_ite *ite,gpa_t gpa,int ite_esz) +struct its_ite *ite,gpa_t gpa) {u32 next_offset;u64 val;
@@ -2101,7 +2135,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, ite->collection->collection_id; val =cpu_to_le64 (val); -return vgic_its_write_entry_lock (its, gpa, val, ite_esz); +return vgic_its_write_entry_lock (its, gpa, val, ite); }/**
@@ -2201,7 +2235,7 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)return -EACCES ; - ret =vgic_its_save_ite (its, device, ite, gpa, ite_esz); + ret =vgic_its_save_ite (its, device, ite, gpa);if (ret)return ret; }
@@ -2240,10 +2274,9 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)* @its: ITS handle * @dev: ITS device * @ptr: GPA -* @dte_esz: device table entry size */ static int vgic_its_save_dte (struct vgic_its *its,struct its_device *dev, -gpa_t ptr,int dte_esz) +gpa_t ptr) {u64 val, itt_addr_field;u32 next_offset;
@@ -2256,7 +2289,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev, (dev->num_eventid_bits - 1)); val =cpu_to_le64 (val); -return vgic_its_write_entry_lock (its, ptr, val, dte_esz); +return vgic_its_write_entry_lock (its, ptr, val, dte); }/**
@@ -2332,10 +2365,8 @@ static int vgic_its_device_cmp(void *priv, const struct list_head *a,*/ static int vgic_its_save_device_tables (struct vgic_its *its) { -const struct vgic_its_abi *abi =vgic_its_get_abi (its);u64 baser = its->baser_device_table;struct its_device *dev; -int dte_esz = abi->dte_esz;if (!(baser &GITS_BASER_VALID ))return 0 ;
@@ -2354,7 +2385,7 @@ static int vgic_its_save_device_tables(struct vgic_its *its)if (ret)return ret; - ret =vgic_its_save_dte (its, dev, eaddr, dte_esz); + ret =vgic_its_save_dte (its, dev, eaddr);if (ret)return ret; }
@@ -2435,7 +2466,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)static int vgic_its_save_cte (struct vgic_its *its,struct its_collection *collection, -gpa_t gpa,int esz) +gpa_t gpa) {u64 val;
@@ -2444,7 +2475,7 @@ static int vgic_its_save_cte(struct vgic_its *its, collection->collection_id); val =cpu_to_le64 (val); -return vgic_its_write_entry_lock (its, gpa, val, esz); +return vgic_its_write_entry_lock (its, gpa, val, cte); }/*
@@ -2452,7 +2483,7 @@ static int vgic_its_save_cte(struct vgic_its *its,* Return +1 on success, 0 if the entry was invalid (which should be * interpreted as end-of-table), and a negative error value for generic errors. */ -static int vgic_its_restore_cte (struct vgic_its *its,gpa_t gpa,int esz) +static int vgic_its_restore_cte (struct vgic_its *its,gpa_t gpa) {struct its_collection *collection;struct kvm *kvm = its->dev->kvm;
@@ -2460,7 +2491,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)u64 val;int ret; - ret =vgic_its_read_entry_lock (its, gpa, &val, esz); + ret =vgic_its_read_entry_lock (its, gpa, &val, cte);if (ret)return ret; val =le64_to_cpu (val);
@@ -2507,7 +2538,7 @@ static int vgic_its_save_collection_table(struct vgic_its *its) max_size =GITS_BASER_NR_PAGES (baser) *SZ_64K ;list_for_each_entry (collection, &its->collection_list, coll_list) { - ret =vgic_its_save_cte (its, collection, gpa, cte_esz); + ret =vgic_its_save_cte (its, collection, gpa);if (ret)return ret; gpa += cte_esz;
@@ -2521,7 +2552,7 @@ static int vgic_its_save_collection_table(struct vgic_its *its)* table is not fully filled, add a last dummy element * with valid bit unset */ -return vgic_its_write_entry_lock (its, gpa,0 , cte_esz); +return vgic_its_write_entry_lock (its, gpa,0ULL , cte); }/*
@@ -2546,7 +2577,7 @@ static int vgic_its_restore_collection_table(struct vgic_its *its) max_size =GITS_BASER_NR_PAGES (baser) *SZ_64K ;while (read < max_size) { - ret =vgic_its_restore_cte (its, gpa, cte_esz); + ret =vgic_its_restore_cte (its, gpa);if (ret <=0 )break ; gpa += cte_esz;
arch/arm64/kvm/vgic/vgic.h +0 -23
@@ -146,29 +146,6 @@ static inline int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa,return ret; } -static inline int vgic_its_read_entry_lock (struct vgic_its *its,gpa_t eaddr, -u64 *eval,unsigned long esize) -{ -struct kvm *kvm = its->dev->kvm; - -if (KVM_BUG_ON (esize !=sizeof (*eval), kvm)) -return -EINVAL ; - -return kvm_read_guest_lock (kvm, eaddr, eval, esize); - -} - -static inline int vgic_its_write_entry_lock (struct vgic_its *its,gpa_t eaddr, -u64 eval,unsigned long esize) -{ -struct kvm *kvm = its->dev->kvm; - -if (KVM_BUG_ON (esize !=sizeof (eval), kvm)) -return -EINVAL ; - -return vgic_write_guest_lock (kvm, eaddr, &eval, esize); -} -/* * This struct provides an intermediate representation of the fields contained * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
Signature · 1 line
2.39.2
Quoted 10 lines · Marc Zyngier
This is a more or less random collection of vgic fixes, addressing issues ranging from "purely cosmetic" all the way to "pretty annoying" (of which patch #1 is an example).
The general theme is around the tightening of the internal API in order to make it a bit more difficult to get things wrong, something that I'm particularly good at. There is a bit more I'd like to do on the ITS, but I'm testing the waters here.
Patches on top of kvmarm-6.13, tested on the funky x1e box.
I'm quite happy with the look of these patches. FYI I'll start picking up fixes in a few days.
Signature · 2 lines
Thanks, Oliver
Oliver Upton
AppliedQuoted 10 lines · Marc Zyngier
This is a more or less random collection of vgic fixes, addressing issues ranging from "purely cosmetic" all the way to "pretty annoying" (of which patch #1 is an example).
The general theme is around the tightening of the internal API in order to make it a bit more difficult to get things wrong, something that I'm particularly good at. There is a bit more I'd like to do on the ITS, but I'm testing the waters here.
[...]
Applied to fixes, thanks!
[1/4] KVM: arm64: vgic-v3: Sanitise guest writes to GICR_INVLPIR
https://git.kernel.org/kvmarm/kvmarm/c/d561491ba927 [2/4] KVM: arm64: vgic: Make vgic_get_irq() more robust https://git.kernel.org/kvmarm/kvmarm/c/add570b39f9f [3/4] KVM: arm64: vgic: Kill VGIC_MAX_PRIVATE definition https://git.kernel.org/kvmarm/kvmarm/c/e7619f2a2f8f [4/4] KVM: arm64: vgic-its: Add stronger type-checking to the ITS entry sizes https://git.kernel.org/kvmarm/kvmarm/c/3b2c81d5feb2
--
Best,
Oliver