From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:47
Hi Paolo and Radim,
Here's a set of KVM/ARM fixes for v4.8-rc3 or as soon as you can make
it. They address:
- ITS init issues
- ITS error handling issues
- ITS IRQ leakage fix
- Plug a couple of ITS race conditions
- An erratum workaround for timers
- Some removal of misleading use of errors and comments
- A fix for GICv3 on 32-bit guests
Note that the patch previously submitted 'KVM: arm64: vgic-its: Grab
kvm->lock when reading kvm->devices', was dropped from this series
because we were already holding the kvm->lock in the call path to the
ITS code.
The following changes since commit 29b4817d4018df78086157ea3a55c1d9424a7cfc:
Linux 4.8-rc1 (2016-08-07 18:18:00 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git kvm-arm-for-v4.8-rc3
for you to fetch changes up to f7f6f2d94f0027242ddfd665289b107a873fde43:
arm64: KVM: report configured SRE value to 32-bit world (2016-08-17 12:46:23 +0200)
Thanks,
-Christoffer
----------------------------------------------------------------
Andre Przywara (4):
KVM: arm64: ITS: return 1 on successful MSI injection
KVM: arm64: ITS: move ITS registration into first VCPU run
KVM: arm64: check for ITS device on MSI injection
KVM: arm64: ITS: avoid re-mapping LPIs
Christoffer Dall (4):
KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
KVM: arm64: vgic-its: Plug race in vgic_put_irq
KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic
KVM: arm/arm64: Change misleading use of is_error_pfn
Marc Zyngier (2):
arm64: Document workaround for Cortex-A72 erratum #853709
KVM: arm/arm64: timer: Workaround misconfigured timer interrupt
Vladimir Murzin (2):
arm64: KVM: remove misleading comment on pmu status
arm64: KVM: report configured SRE value to 32-bit world
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm/kvm/mmu.c | 2 +-
arch/arm64/kvm/hyp/switch.c | 2 +-
arch/arm64/kvm/sys_regs.c | 10 +--
include/linux/irqchip/arm-gic-v3.h | 1 +
virt/kvm/arm/arch_timer.c | 11 ++-
virt/kvm/arm/vgic/vgic-its.c | 158 ++++++++++++++++++++++++---------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 26 +++---
virt/kvm/arm/vgic/vgic-v3.c | 8 ++
virt/kvm/arm/vgic/vgic.c | 10 +--
virt/kvm/arm/vgic/vgic.h | 6 ++
11 files changed, 164 insertions(+), 71 deletions(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:48
From: Andre Przywara <andre.przywara@arm.com>
According to the KVM API documentation a successful MSI injection
should return a value > 0 on success.
Return possible errors in vgic_its_trigger_msi() and report a
successful injection back to userland, while also reporting the
case where the MSI could not be delivered due to the guest not
having the LPI mapped, for instance.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Christoffer Dall <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
include/linux/irqchip/arm-gic-v3.h | 1 +
virt/kvm/arm/vgic/vgic-its.c | 56 +++++++++++++++++++++++++-------------
2 files changed, 38 insertions(+), 19 deletions(-)
@@ -441,39 +441,48 @@ static unsigned long vgic_mmio_read_its_idregs(struct kvm *kvm,*FindthetargetVCPUandtheLPInumberforagivendevid/eventidpair*andmakethisIRQpending,possiblyinjectingit.*Mustbecalledwiththeits_lockmutexheld.+*Returns0onsuccess,apositiveerrorvalueforanyITSmapping+*relatederrorsandnegativeerrorvaluesforgenericerrors.*/-staticvoidvgic_its_trigger_msi(structkvm*kvm,structvgic_its*its,-u32devid,u32eventid)+staticintvgic_its_trigger_msi(structkvm*kvm,structvgic_its*its,+u32devid,u32eventid){+structkvm_vcpu*vcpu;structits_itte*itte;if(!its->enabled)-return;+return-EBUSY;itte=find_itte(its,devid,eventid);-/* Triggering an unmapped IRQ gets silently dropped. */-if(itte&&its_is_collection_mapped(itte->collection)){-structkvm_vcpu*vcpu;--vcpu=kvm_get_vcpu(kvm,itte->collection->target_addr);-if(vcpu&&vcpu->arch.vgic_cpu.lpis_enabled){-spin_lock(&itte->irq->irq_lock);-itte->irq->pending=true;-vgic_queue_irq_unlock(kvm,itte->irq);-}-}+if(!itte||!its_is_collection_mapped(itte->collection))+returnE_ITS_INT_UNMAPPED_INTERRUPT;++vcpu=kvm_get_vcpu(kvm,itte->collection->target_addr);+if(!vcpu)+returnE_ITS_INT_UNMAPPED_INTERRUPT;++if(!vcpu->arch.vgic_cpu.lpis_enabled)+return-EBUSY;++spin_lock(&itte->irq->irq_lock);+itte->irq->pending=true;+vgic_queue_irq_unlock(kvm,itte->irq);++return0;}/**QueriestheKVMIObusframeworktogettheITSpointerfromthegiven*doorbelladdress.*Wethencallvgic_its_trigger_msi()withthedecodeddata.+*AccordingtotheKVM_SIGNAL_MSIAPIdescriptionreturns1onsuccess.*/intvgic_its_inject_msi(structkvm*kvm,structkvm_msi*msi){u64address;structkvm_io_device*kvm_io_dev;structvgic_io_device*iodev;+intret;if(!vgic_has_its(kvm))return-ENODEV;
@@ -490,10 +499,21 @@ int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)iodev=container_of(kvm_io_dev,structvgic_io_device,dev);mutex_lock(&iodev->its->its_lock);-vgic_its_trigger_msi(kvm,iodev->its,msi->devid,msi->data);+ret=vgic_its_trigger_msi(kvm,iodev->its,msi->devid,msi->data);mutex_unlock(&iodev->its->its_lock);-return0;+if(ret<0)+returnret;++/*+*KVM_SIGNAL_MSIdemandsareturnvalue>0forsuccessand0+*iftheguesthasblockedtheMSI.SowemapanyLPImapping+*relatederrortothat.+*/+if(ret)+return0;+else+return1;}/* Requires the its_lock to be held. */
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:49
During low memory conditions, we could be dereferencing a NULL pointer
when vgic_add_lpi fails to allocate memory.
Consider for example this call sequence:
vgic_its_cmd_handle_mapi
itte->irq = vgic_add_lpi(kvm, lpi_nr);
update_lpi_config(kvm, itte->irq, NULL);
ret = kvm_read_guest(kvm, propbase + irq->intid
^^^^
kaboom?
Instead, return an error pointer from vgic_add_lpi and check the return
value from its single caller.
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic-its.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
@@ -522,7 +522,8 @@ static void its_free_itte(struct kvm *kvm, struct its_itte *itte)list_del(&itte->itte_list);/* This put matches the get in vgic_add_lpi. */-vgic_put_irq(kvm,itte->irq);+if(itte->irq)+vgic_put_irq(kvm,itte->irq);kfree(itte);}
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:50
Right now the following sequence of events can happen:
1. Thread X calls vgic_put_irq
2. Thread Y calls vgic_add_lpi
3. Thread Y gets lpi_list_lock
4. Thread X drops the ref count to 0 and blocks on lpi_list_lock
5. Thread Y finds the irq via the lpi_list_lock, raises the ref
count to 1, and release the lpi_list_lock.
6. Thread X proceeds and frees the irq.
Avoid this by holding the spinlock around the kref_put.
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:51
There are two problems with the current implementation of the MMIO
handlers for the propbaser and pendbaser:
First, the write to the value itself is not guaranteed to be an atomic
64-bit write so two concurrent writes to the structure field could be
intermixed.
Second, because we do a read-modify-update operation without any
synchronization, if we have two 32-bit accesses to separate parts of the
register, we can loose one of them.
By using the atomic cmpxchg64 we should cover both issues above.
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
@@ -306,16 +306,19 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,{structvgic_dist*dist=&vcpu->kvm->arch.vgic;structvgic_cpu*vgic_cpu=&vcpu->arch.vgic_cpu;-u64propbaser=dist->propbaser;+u64old_propbaser,propbaser;/* Storing a value with LPIs already enabled is undefined */if(vgic_cpu->lpis_enabled)return;-propbaser=update_64bit_reg(propbaser,addr&4,len,val);-propbaser=vgic_sanitise_propbaser(propbaser);--dist->propbaser=propbaser;+do{+old_propbaser=dist->propbaser;+propbaser=old_propbaser;+propbaser=update_64bit_reg(propbaser,addr&4,len,val);+propbaser=vgic_sanitise_propbaser(propbaser);+}while(cmpxchg64(&dist->propbaser,old_propbaser,+propbaser)!=old_propbaser);}staticunsignedlongvgic_mmio_read_pendbase(structkvm_vcpu*vcpu,
@@ -331,16 +334,19 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,unsignedlongval){structvgic_cpu*vgic_cpu=&vcpu->arch.vgic_cpu;-u64pendbaser=vgic_cpu->pendbaser;+u64old_pendbaser,pendbaser;/* Storing a value with LPIs already enabled is undefined */if(vgic_cpu->lpis_enabled)return;-pendbaser=update_64bit_reg(pendbaser,addr&4,len,val);-pendbaser=vgic_sanitise_pendbaser(pendbaser);--vgic_cpu->pendbaser=pendbaser;+do{+old_pendbaser=vgic_cpu->pendbaser;+pendbaser=old_pendbaser;+pendbaser=update_64bit_reg(pendbaser,addr&4,len,val);+pendbaser=vgic_sanitise_pendbaser(pendbaser);+}while(cmpxchg64(&vgic_cpu->pendbaser,old_pendbaser,+pendbaser)!=old_pendbaser);}/*
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:52
From: Andre Przywara <andre.przywara@arm.com>
Currently we register an ITS device upon userland issuing the CTLR_INIT
ioctl to mark initialization of the ITS as done.
This deviates from the initialization sequence of the existing GIC
devices and does not play well with the way QEMU handles things.
To be more in line with what we are used to, register the ITS(es) just
before the first VCPU is about to run, so in the map_resources() call.
This involves iterating through the list of KVM devices and map each
ITS that we find.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic-its.c | 43 +++++++++++++++++++++++++++++++++----------
virt/kvm/arm/vgic/vgic-v3.c | 8 ++++++++
virt/kvm/arm/vgic/vgic.h | 6 ++++++
3 files changed, 47 insertions(+), 10 deletions(-)
@@ -289,6 +289,14 @@ int vgic_v3_map_resources(struct kvm *kvm)gotoout;}+if(vgic_has_its(kvm)){+ret=vgic_register_its_iodevs(kvm);+if(ret){+kvm_err("Unable to register VGIC ITS MMIO regions\n");+gotoout;+}+}+dist->ready=true;out:
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:53
From: Andre Przywara <andre.przywara@arm.com>
When userspace provides the doorbell address for an MSI to be
injected into the guest, we find a KVM device which feels responsible.
Lets check that this device is really an emulated ITS before we make
real use of the container_of-ed pointer.
[ Moved NULL-pointer check to caller of static function
- Christoffer ]
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic-its.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:54
From: Andre Przywara <andre.przywara@arm.com>
When a guest wants to map a device-ID/event-ID combination that is
already mapped, we may end up in a situation where an LPI is never
"put", thus never being freed.
Since the GICv3 spec says that mapping an already mapped LPI is
UNPREDICTABLE, lets just bail out early in this situation to avoid
any potential leaks.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
@@ -749,6 +749,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,lpi_nr>=max_lpis_propbaser(kvm->arch.vgic.propbaser))returnE_ITS_MAPTI_PHYSICALID_OOR;+/* If there is an existing mapping, behavior is UNPREDICTABLE. */+if(find_itte(its,device_id,event_id))+return0;+collection=find_collection(its,coll_id);if(!collection){intret=vgic_its_alloc_collection(its,&collection,coll_id);
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:55
When converting a gfn to a pfn, we call gfn_to_pfn_prot, which returns
various kinds of error values. It turns out that is_error_pfn() only
returns true when the gfn was found in a memory slot and could somehow
not be used, but it does not return true if the gfn does not belong to
any memory slot.
Change use to is_error_noslot_pfn() which covers both cases.
Note: Since we already check for kvm_is_error_hva(hva) explicitly in the
caller of this function while holding the kvm->srcu lock protecting the
memory slots, this should never be a problem, but nevertheless this
change is warranted as it shows the intention of the code.
Reported-by: James Hogan <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
arch/arm/kvm/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:56
From: Marc Zyngier <redacted>
We already have a workaround for Cortex-A57 erratum #852523,
but Cortex-A72 r0p0 to r0p2 do suffer from the same issue
(known as erratum #853709).
Let's document the fact that we already handle this.
Acked-by: Will Deacon <redacted>
Signed-off-by: Marc Zyngier <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm64/kvm/hyp/switch.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:57
From: Marc Zyngier <redacted>
Similarily to f005bd7e3b84 ("clocksource/arm_arch_timer: Force
per-CPU interrupt to be level-triggered"), make sure we can
survive an interrupt that has been misconfigured as edge-triggered
by forcing it to be level-triggered (active low is assumed, but
the GIC doesn't really care whether this is high or low).
Hopefully, the amount of shouting in the kernel log will convince
the user to do something about their firmware.
Signed-off-by: Marc Zyngier <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
virt/kvm/arm/arch_timer.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:58
From: Vladimir Murzin <redacted>
Comment about how PMU access is handled is not relavant since v4.6
where proper PMU support was added in.
Signed-off-by: Vladimir Murzin <redacted>
Acked-by: Marc Zyngier <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
arch/arm64/kvm/sys_regs.c | 8 --------
1 file changed, 8 deletions(-)
From: Christoffer Dall <hidden> Date: 2016-08-17 19:38:59
From: Vladimir Murzin <redacted>
After commit b34f2bc ("arm64: KVM: Make ICC_SRE_EL1 access return the
configured SRE value") we report SRE value to 64-bit guest, but 32-bit
one still handled as RAZ/WI what leads to funny promise we do not keep:
"GICv3: GIC: unable to set SRE (disabled at EL2), panic ahead"
Instead, return the actual value of the ICC_SRE_EL1 register that the
guest should see.
[ Tweaked commit message - Christoffer ]
Signed-off-by: Vladimir Murzin <redacted>
Acked-by: Marc Zyngier <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
arch/arm64/kvm/sys_regs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Hi Paolo and Radim,
Here's a set of KVM/ARM fixes for v4.8-rc3 or as soon as you can make
it. They address:
- ITS init issues
- ITS error handling issues
- ITS IRQ leakage fix
- Plug a couple of ITS race conditions
- An erratum workaround for timers
- Some removal of misleading use of errors and comments
- A fix for GICv3 on 32-bit guests
Note that the patch previously submitted 'KVM: arm64: vgic-its: Grab
kvm->lock when reading kvm->devices', was dropped from this series
because we were already holding the kvm->lock in the call path to the
ITS code.
The following changes since commit 29b4817d4018df78086157ea3a55c1d9424a7cfc:
Linux 4.8-rc1 (2016-08-07 18:18:00 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git kvm-arm-for-v4.8-rc3
for you to fetch changes up to f7f6f2d94f0027242ddfd665289b107a873fde43:
arm64: KVM: report configured SRE value to 32-bit world (2016-08-17 12:46:23 +0200)
Thanks,
-Christoffer
----------------------------------------------------------------
Andre Przywara (4):
KVM: arm64: ITS: return 1 on successful MSI injection
KVM: arm64: ITS: move ITS registration into first VCPU run
KVM: arm64: check for ITS device on MSI injection
KVM: arm64: ITS: avoid re-mapping LPIs
Christoffer Dall (4):
KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
KVM: arm64: vgic-its: Plug race in vgic_put_irq
KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic
KVM: arm/arm64: Change misleading use of is_error_pfn
Marc Zyngier (2):
arm64: Document workaround for Cortex-A72 erratum #853709
KVM: arm/arm64: timer: Workaround misconfigured timer interrupt
Vladimir Murzin (2):
arm64: KVM: remove misleading comment on pmu status
arm64: KVM: report configured SRE value to 32-bit world
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm/kvm/mmu.c | 2 +-
arch/arm64/kvm/hyp/switch.c | 2 +-
arch/arm64/kvm/sys_regs.c | 10 +--
include/linux/irqchip/arm-gic-v3.h | 1 +
virt/kvm/arm/arch_timer.c | 11 ++-
virt/kvm/arm/vgic/vgic-its.c | 158 ++++++++++++++++++++++++---------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 26 +++---
virt/kvm/arm/vgic/vgic-v3.c | 8 ++
virt/kvm/arm/vgic/vgic.c | 10 +--
virt/kvm/arm/vgic/vgic.h | 6 ++
11 files changed, 164 insertions(+), 71 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html