From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:33:12
Hi all,
While reviewing Shameer's reworked VMID allocator [1] and discussing
with Marc, we spotted a race between TLB invalidation (which typically
takes an ASID or VMID argument) and reallocation of ASID/VMID for the
context being targetted.
The first patch spells out an example with try_to_unmap_one() in a
comment, which Catalin has kindly modelled in TLA+ at [2].
Although I'm posting all this together for ease of review, the intention
is that the first patch will go via arm64 with the latter going via kvm.
Cheers,
Will
[1] https://lore.kernel.org/r/20210729104009.382-1-shameerali.kolothum.thodi@huawei.com
[2] https://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/kernel-tla.git/commit/
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Jade Alglave <redacted>
Cc: Shameer Kolothum <redacted>
Cc: <redacted>
Cc: <redacted>
--->8
Marc Zyngier (3):
KVM: arm64: Move kern_hyp_va() usage in __load_guest_stage2() into the
callers
KVM: arm64: Convert the host S2 over to __load_guest_stage2()
KVM: arm64: Upgrade VMID accesses to {READ,WRITE}_ONCE
Will Deacon (1):
arm64: mm: Fix TLBI vs ASID rollover
arch/arm64/include/asm/kvm_mmu.h | 17 ++++++-----
arch/arm64/include/asm/mmu.h | 29 ++++++++++++++++---
arch/arm64/include/asm/tlbflush.h | 11 +++----
arch/arm64/kvm/arm.c | 2 +-
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 ++--
arch/arm64/kvm/hyp/nvhe/switch.c | 4 ++-
arch/arm64/kvm/hyp/nvhe/tlb.c | 2 +-
arch/arm64/kvm/hyp/vhe/switch.c | 2 +-
arch/arm64/kvm/hyp/vhe/tlb.c | 2 +-
arch/arm64/kvm/mmu.c | 2 +-
11 files changed, 52 insertions(+), 27 deletions(-)
--
2.32.0.605.g8dce9f2422-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:31:24
When switching to an 'mm_struct' for the first time following an ASID
rollover, a new ASID may be allocated and assigned to 'mm->context.id'.
This reassignment can happen concurrently with other operations on the
mm, such as unmapping pages and subsequently issuing TLB invalidation.
Consequently, we need to ensure that (a) accesses to 'mm->context.id'
are atomic and (b) all page-table updates made prior to a TLBI using the
old ASID are guaranteed to be visible to CPUs running with the new ASID.
This was found by inspection after reviewing the VMID changes from
Shameer but it looks like a real (yet hard to hit) bug.
Cc: <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Jade Alglave <redacted>
Cc: Shameer Kolothum <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/mmu.h | 29 +++++++++++++++++++++++++----
arch/arm64/include/asm/tlbflush.h | 11 ++++++-----
2 files changed, 31 insertions(+), 9 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:33:52
From: Marc Zyngier <maz@kernel.org>
It is a bit awkward to use kern_hyp_va() in __load_guest_stage2(),
specially as the helper is shared between VHE and nVHE.
Instead, move the use of kern_hyp_va() in the nVHE code, and
pass a pointer to the kvm->arch structure instead. Although
this may look a bit awkward, it allows for some further simplification.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Jade Alglave <redacted>
Cc: Shameer Kolothum <redacted>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_mmu.h | 5 +++--
arch/arm64/kvm/hyp/nvhe/switch.c | 4 +++-
arch/arm64/kvm/hyp/nvhe/tlb.c | 2 +-
arch/arm64/kvm/hyp/vhe/switch.c | 2 +-
arch/arm64/kvm/hyp/vhe/tlb.c | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:33:53
If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
to a "restricted-dma-pool" will fail with a reasonably cryptic error:
| pci-host-generic: probe of 10000.pci failed with error -22
Print a more helpful message in this case and try to continue probing
the device as we do if the kernel doesn't have the restricted DMA patches
applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
Cc: Claire Chang <redacted>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
drivers/of/address.c | 8 ++++----
drivers/of/device.c | 2 +-
drivers/of/of_private.h | 8 +++-----
3 files changed, 8 insertions(+), 10 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:34:30
From: Marc Zyngier <maz@kernel.org>
Since TLB invalidation can run in parallel with VMID allocation,
we need to be careful and avoid any sort of load/store tearing.
Use {READ,WRITE}_ONCE consistently to avoid any surprise.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Jade Alglave <redacted>
Cc: Shameer Kolothum <redacted>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_mmu.h | 7 ++++++-
arch/arm64/kvm/arm.c | 2 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 4 ++--
arch/arm64/kvm/mmu.c | 2 +-
4 files changed, 10 insertions(+), 5 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-06 11:37:41
On Fri, Aug 06, 2021 at 12:31:05PM +0100, Will Deacon wrote:
If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
to a "restricted-dma-pool" will fail with a reasonably cryptic error:
| pci-host-generic: probe of 10000.pci failed with error -22
Print a more helpful message in this case and try to continue probing
the device as we do if the kernel doesn't have the restricted DMA patches
applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
Cc: Claire Chang <redacted>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
drivers/of/address.c | 8 ++++----
drivers/of/device.c | 2 +-
drivers/of/of_private.h | 8 +++-----
3 files changed, 8 insertions(+), 10 deletions(-)
Sorry, didn't mean to send this patch a second time, it was still kicking
around in my tree from yesterday and I accidentally picked it up when
sending my TLBI series.
Please ignore.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
We'd need specs clarified (ARM ARM, cat model) that the DSB ISHST is
sufficient to order the pte write with the subsequent ASID read.
Otherwise the patch looks fine to me:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
We'd need specs clarified (ARM ARM, cat model) that the DSB ISHST is
sufficient to order the pte write with the subsequent ASID read.
Although I agree that the cat model needs updating and also that the Arm
ARM isn't helpful by trying to define DMB and DSB at the same time, it
does clearly state the following:
// B2-149
| A DSB instruction executed by a PE, PEe, completes when all of the
| following apply:
|
| * All explicit memory accesses of the required access types appearing
| in program order before the DSB are complete for the set of observers
| in the required shareability domain.
[...]
// B2-150
| In addition, no instruction that appears in program order after the
| DSB instruction can alter any state of the system or perform any part
| of its functionality until the DSB completes other than:
|
| * Being fetched from memory and decoded.
| * Reading the general-purpose, SIMD and floating-point, Special-purpose,
| or System registers that are directly or indirectly read without
| causing side-effects.
Which means that the ASID read cannot return its data before the DSB ISHST
has completed and the DSB ISHST cannot complete until the PTE write has
completed.
Otherwise the patch looks fine to me:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Thanks! Do you want to queue it for 5.15? I don't think there's a need to
rush it into 5.14 given that we don't have any evidence of it happening
in practice.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
We'd need specs clarified (ARM ARM, cat model) that the DSB ISHST is
sufficient to order the pte write with the subsequent ASID read.
Although I agree that the cat model needs updating and also that the Arm
ARM isn't helpful by trying to define DMB and DSB at the same time, it
does clearly state the following:
// B2-149
| A DSB instruction executed by a PE, PEe, completes when all of the
| following apply:
|
| * All explicit memory accesses of the required access types appearing
| in program order before the DSB are complete for the set of observers
| in the required shareability domain.
[...]
// B2-150
| In addition, no instruction that appears in program order after the
| DSB instruction can alter any state of the system or perform any part
| of its functionality until the DSB completes other than:
|
| * Being fetched from memory and decoded.
| * Reading the general-purpose, SIMD and floating-point, Special-purpose,
| or System registers that are directly or indirectly read without
| causing side-effects.
Which means that the ASID read cannot return its data before the DSB ISHST
has completed and the DSB ISHST cannot complete until the PTE write has
completed.
Thanks for the explanation.
quoted
Otherwise the patch looks fine to me:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Thanks! Do you want to queue it for 5.15? I don't think there's a need to
rush it into 5.14 given that we don't have any evidence of it happening
in practice.
@@ -126,7 +126,7 @@ int __pkvm_prot_finalize(void)kvm_flush_dcache_to_poc(params,sizeof(*params));write_sysreg(params->hcr_el2,hcr_el2);-__load_stage2(&host_kvm.arch.mmu,host_kvm.arch.vtcr);+__load_guest_stage2(&host_kvm.arch.mmu,&host_kvm.arch);
Nit: clearly we're not loading a guest stage-2 here, so maybe the
function should take a more generic name?
Thanks,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Friday 06 Aug 2021 at 12:31:08 (+0100), Will Deacon wrote:
quoted hunk
From: Marc Zyngier <maz@kernel.org>
Since TLB invalidation can run in parallel with VMID allocation,
we need to be careful and avoid any sort of load/store tearing.
Use {READ,WRITE}_ONCE consistently to avoid any surprise.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Jade Alglave <redacted>
Cc: Shameer Kolothum <redacted>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_mmu.h | 7 ++++++-
arch/arm64/kvm/arm.c | 2 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 4 ++--
arch/arm64/kvm/mmu.c | 2 +-
4 files changed, 10 insertions(+), 5 deletions(-)
@@ -109,8 +109,8 @@ int kvm_host_prepare_stage2(void *pgt_pool_base)mmu->pgd_phys=__hyp_pa(host_kvm.pgt.pgd);mmu->arch=&host_kvm.arch;mmu->pgt=&host_kvm.pgt;-mmu->vmid.vmid_gen=0;-mmu->vmid.vmid=0;+WRITE_ONCE(mmu->vmid.vmid_gen,0);+WRITE_ONCE(mmu->vmid.vmid,0);
I'm guessing it should be safe to omit those? But they certainly don't
harm and can serve as documentation anyway, so:
Reviewed-by: Quentin Perret <redacted>
Thanks,
Quentin
On Fri, 6 Aug 2021 12:31:03 +0100, Will Deacon wrote:
While reviewing Shameer's reworked VMID allocator [1] and discussing
with Marc, we spotted a race between TLB invalidation (which typically
takes an ASID or VMID argument) and reallocation of ASID/VMID for the
context being targetted.
The first patch spells out an example with try_to_unmap_one() in a
comment, which Catalin has kindly modelled in TLA+ at [2].
[...]
@@ -126,7 +126,7 @@ int __pkvm_prot_finalize(void)kvm_flush_dcache_to_poc(params,sizeof(*params));write_sysreg(params->hcr_el2,hcr_el2);-__load_stage2(&host_kvm.arch.mmu,host_kvm.arch.vtcr);+__load_guest_stage2(&host_kvm.arch.mmu,&host_kvm.arch);
Nit: clearly we're not loading a guest stage-2 here, so maybe the
function should take a more generic name?
How about we rename __load_guest_stage2() to __load_stage2() instead,
with the same parameters?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-----Original Message-----
From: Will Deacon [mailto:will@kernel.org]
Sent: 06 August 2021 12:31
To: linux-arm-kernel@lists.infradead.org
Cc: kernel-team@android.com; Will Deacon <will@kernel.org>; Catalin
Marinas [off-list ref]; Marc Zyngier [off-list ref]; Jade
Alglave [off-list ref]; Shameerali Kolothum Thodi
[off-list ref]; kvmarm@lists.cs.columbia.edu;
linux-arch@vger.kernel.org
Subject: [PATCH 0/4] Fix racing TLBI with ASID/VMID reallocation
Hi all,
While reviewing Shameer's reworked VMID allocator [1] and discussing
with Marc, we spotted a race between TLB invalidation (which typically
takes an ASID or VMID argument) and reallocation of ASID/VMID for the
context being targetted.
The first patch spells out an example with try_to_unmap_one() in a
comment, which Catalin has kindly modelled in TLA+ at [2].
Although I'm posting all this together for ease of review, the intention
is that the first patch will go via arm64 with the latter going via kvm.
Cheers,
Will
[1]
https://lore.kernel.org/r/20210729104009.382-1-shameerali.kolothum.thodi
@huawei.com
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/kernel-tla.git/commi
t/
Hi Catalin,
I am going through the ASID TLA+ model and in the above commit, it appears that the
different ASID check(=> ActiveAsid(c1) # ActiveAsid(c2)) for the Invariant
UniqueASIDActiveTask is now removed.
Just wondering why that is not relevant anymore?
Thanks,
Shameer
I am going through the ASID TLA+ model and in the above commit, it appears that the
different ASID check(=> ActiveAsid(c1) # ActiveAsid(c2)) for the Invariant
UniqueASIDActiveTask is now removed.
Just wondering why that is not relevant anymore?
It's still relevant. I probably deleted it by mistake, I'll add it back
now. Thanks for carefully looking at this commit.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel