From: Will Deacon <will@kernel.org>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: iommu@lists.linux-foundation.org,
linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org,
joro@8bytes.org, catalin.marinas@arm.com, robin.murphy@arm.com,
baolu.lu@linux.intel.com, Jonathan.Cameron@huawei.com,
jacob.jun.pan@linux.intel.com, zhangfei.gao@linaro.org,
xuzaibo@huawei.com, zhengxiang9@huawei.com, fenghua.yu@intel.com,
hch@infradead.org
Subject: Re: [PATCH v8 04/12] arm64: mm: Pin down ASIDs for sharing mm with devices
Date: Mon, 13 Jul 2020 16:46:23 +0100 [thread overview]
Message-ID: <20200713154622.GC3072@willie-the-truck> (raw)
In-Reply-To: <20200618155125.1548969-5-jean-philippe@linaro.org>
On Thu, Jun 18, 2020 at 05:51:17PM +0200, Jean-Philippe Brucker wrote:
> To enable address space sharing with the IOMMU, introduce mm_context_get()
> and mm_context_put(), that pin down a context and ensure that it will keep
> its ASID after a rollover. Export the symbols to let the modular SMMUv3
> driver use them.
>
> Pinning is necessary because a device constantly needs a valid ASID,
> unlike tasks that only require one when running. Without pinning, we would
> need to notify the IOMMU when we're about to use a new ASID for a task,
> and it would get complicated when a new task is assigned a shared ASID.
> Consider the following scenario with no ASID pinned:
>
> 1. Task t1 is running on CPUx with shared ASID (gen=1, asid=1)
> 2. Task t2 is scheduled on CPUx, gets ASID (1, 2)
> 3. Task tn is scheduled on CPUy, a rollover occurs, tn gets ASID (2, 1)
> We would now have to immediately generate a new ASID for t1, notify
> the IOMMU, and finally enable task tn. We are holding the lock during
> all that time, since we can't afford having another CPU trigger a
> rollover. The IOMMU issues invalidation commands that can take tens of
> milliseconds.
>
> It gets needlessly complicated. All we wanted to do was schedule task tn,
> that has no business with the IOMMU. By letting the IOMMU pin tasks when
> needed, we avoid stalling the slow path, and let the pinning fail when
> we're out of shareable ASIDs.
>
> After a rollover, the allocator expects at least one ASID to be available
> in addition to the reserved ones (one per CPU). So (NR_ASIDS - NR_CPUS -
> 1) is the maximum number of ASIDs that can be shared with the IOMMU.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> arch/arm64/include/asm/mmu.h | 1 +
> arch/arm64/include/asm/mmu_context.h | 11 +++-
> arch/arm64/mm/context.c | 95 +++++++++++++++++++++++++++-
> 3 files changed, 104 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index 68140fdd89d6b..bbdd291e31d59 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -19,6 +19,7 @@
>
> typedef struct {
> atomic64_t id;
> + unsigned long pinned;
bool?
> void *vdso;
> unsigned long flags;
> } mm_context_t;
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index b0bd9b55594c5..7b0e0f6cb7e87 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -177,7 +177,13 @@ static inline void cpu_replace_ttbr1(pgd_t *pgdp)
> #define destroy_context(mm) do { } while(0)
> void check_and_switch_context(struct mm_struct *mm, unsigned int cpu);
>
> -#define init_new_context(tsk,mm) ({ atomic64_set(&(mm)->context.id, 0); 0; })
> +static inline int
> +init_new_context(struct task_struct *tsk, struct mm_struct *mm)
> +{
> + atomic64_set(&mm->context.id, 0);
> + mm->context.pinned = 0;
> + return 0;
> +}
>
> #ifdef CONFIG_ARM64_SW_TTBR0_PAN
> static inline void update_saved_ttbr0(struct task_struct *tsk,
> @@ -250,6 +256,9 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
> void verify_cpu_asid_bits(void);
> void post_ttbr_update_workaround(void);
>
> +unsigned long mm_context_get(struct mm_struct *mm);
> +void mm_context_put(struct mm_struct *mm);
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* !__ASM_MMU_CONTEXT_H */
> diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
> index d702d60e64dab..d0ddd413f5645 100644
> --- a/arch/arm64/mm/context.c
> +++ b/arch/arm64/mm/context.c
> @@ -27,6 +27,10 @@ static DEFINE_PER_CPU(atomic64_t, active_asids);
> static DEFINE_PER_CPU(u64, reserved_asids);
> static cpumask_t tlb_flush_pending;
>
> +static unsigned long max_pinned_asids;
> +static unsigned long nr_pinned_asids;
> +static unsigned long *pinned_asid_map;
> +
> #define ASID_MASK (~GENMASK(asid_bits - 1, 0))
> #define ASID_FIRST_VERSION (1UL << asid_bits)
>
> @@ -74,6 +78,9 @@ void verify_cpu_asid_bits(void)
>
> static void set_kpti_asid_bits(void)
> {
> + unsigned int k;
> + u8 *dst = (u8 *)asid_map;
> + u8 *src = (u8 *)pinned_asid_map;
> unsigned int len = BITS_TO_LONGS(NUM_USER_ASIDS) * sizeof(unsigned long);
> /*
> * In case of KPTI kernel/user ASIDs are allocated in
> @@ -81,7 +88,8 @@ static void set_kpti_asid_bits(void)
> * is set, then the ASID will map only userspace. Thus
> * mark even as reserved for kernel.
> */
> - memset(asid_map, 0xaa, len);
> + for (k = 0; k < len; k++)
> + dst[k] = src[k] | 0xaa;
Can you use __bitmap_replace() here? I think it would be clearer to use the
bitmap API wherever possible, since casting 'unsigned long *' to 'u8 *'
just makes me worry about endianness issues (although in this case I don't
hink it's a problem).
> }
>
> static void set_reserved_asid_bits(void)
> @@ -89,7 +97,7 @@ static void set_reserved_asid_bits(void)
> if (arm64_kernel_unmapped_at_el0())
> set_kpti_asid_bits();
> else
> - bitmap_clear(asid_map, 0, NUM_USER_ASIDS);
> + bitmap_copy(asid_map, pinned_asid_map, NUM_USER_ASIDS);
> }
>
> #define asid_gen_match(asid) \
> @@ -165,6 +173,14 @@ static u64 new_context(struct mm_struct *mm)
> if (check_update_reserved_asid(asid, newasid))
> return newasid;
>
> + /*
> + * If it is pinned, we can keep using it. Note that reserved
> + * takes priority, because even if it is also pinned, we need to
> + * update the generation into the reserved_asids.
> + */
> + if (mm->context.pinned)
> + return newasid;
> +
> /*
> * We had a valid ASID in a previous life, so try to re-use
> * it if possible.
> @@ -254,6 +270,68 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
> cpu_switch_mm(mm->pgd, mm);
> }
>
> +unsigned long mm_context_get(struct mm_struct *mm)
> +{
> + unsigned long flags;
> + u64 asid;
> +
> + raw_spin_lock_irqsave(&cpu_asid_lock, flags);
> +
> + asid = atomic64_read(&mm->context.id);
> +
> + if (mm->context.pinned) {
> + mm->context.pinned++;
> + asid &= ~ASID_MASK;
> + goto out_unlock;
> + }
> +
> + if (nr_pinned_asids >= max_pinned_asids) {
> + asid = 0;
> + goto out_unlock;
> + }
> +
> + if (!asid_gen_match(asid)) {
> + /*
> + * We went through one or more rollover since that ASID was
> + * used. Ensure that it is still valid, or generate a new one.
> + */
> + asid = new_context(mm);
> + atomic64_set(&mm->context.id, asid);
> + }
> +
> + asid &= ~ASID_MASK;
> +
> + nr_pinned_asids++;
> + __set_bit(asid2idx(asid), pinned_asid_map);
> + mm->context.pinned++;
> +
> +out_unlock:
> + raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
Maybe stick the & ~ASID_MASK here so it's easier to read?
> + /* Set the equivalent of USER_ASID_BIT */
> + if (asid && IS_ENABLED(CONFIG_UNMAP_KERNEL_AT_EL0))
> + asid |= 1;
> +
> + return asid;
> +}
> +EXPORT_SYMBOL_GPL(mm_context_get);
That's quite a generic symbol name to export... maybe throw 'arm64_' in
front of it?
> +
> +void mm_context_put(struct mm_struct *mm)
> +{
> + unsigned long flags;
> + u64 asid = atomic64_read(&mm->context.id) & ~ASID_MASK;
I don't think you need the masking here.
> + raw_spin_lock_irqsave(&cpu_asid_lock, flags);
> +
> + if (--mm->context.pinned == 0) {
> + __clear_bit(asid2idx(asid), pinned_asid_map);
> + nr_pinned_asids--;
> + }
> +
> + raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(mm_context_put);
Same naming comment here.
> /* Errata workaround post TTBRx_EL1 update. */
> asmlinkage void post_ttbr_update_workaround(void)
> {
> @@ -303,6 +381,13 @@ static int asids_update_limit(void)
> WARN_ON(num_available_asids - 1 <= num_possible_cpus());
> pr_info("ASID allocator initialised with %lu entries\n",
> num_available_asids);
> +
> + /*
> + * We assume that an ASID is always available after a rollover. This
> + * means that even if all CPUs have a reserved ASID, there still is at
> + * least one slot available in the asid map.
> + */
> + max_pinned_asids = num_available_asids - num_possible_cpus() - 2;
Is it worth checking that assumption, rather than setting max_pinned_asids
to a massive value?
> return 0;
> }
> arch_initcall(asids_update_limit);
> @@ -317,6 +402,12 @@ static int asids_init(void)
> panic("Failed to allocate bitmap for %lu ASIDs\n",
> NUM_USER_ASIDS);
>
> + pinned_asid_map = kcalloc(BITS_TO_LONGS(NUM_USER_ASIDS),
> + sizeof(*pinned_asid_map), GFP_KERNEL);
> + if (!pinned_asid_map)
> + panic("Failed to allocate pinned ASID bitmap\n");
Why can't we continue in this case without support for pinned ASIDs?
Will
next prev parent reply other threads:[~2020-07-13 15:46 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-18 15:51 [PATCH v8 00/12] iommu: Shared Virtual Addressing for SMMUv3 (PT sharing part) Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 01/12] mm: Define pasid in mm Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 02/12] iommu/ioasid: Add ioasid references Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 03/12] iommu/sva: Add PASID helpers Jean-Philippe Brucker
2020-06-19 7:37 ` Lu Baolu
2020-06-18 15:51 ` [PATCH v8 04/12] arm64: mm: Pin down ASIDs for sharing mm with devices Jean-Philippe Brucker
2020-07-13 15:46 ` Will Deacon [this message]
2020-07-16 15:44 ` Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 05/12] iommu/io-pgtable-arm: Move some definitions to a header Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 06/12] arm64: cpufeature: Export symbol read_sanitised_ftr_reg() Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 07/12] iommu/arm-smmu-v3: Share process page tables Jean-Philippe Brucker
2020-07-13 20:22 ` Will Deacon
2020-07-16 15:45 ` Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 08/12] iommu/arm-smmu-v3: Seize private ASID Jean-Philippe Brucker
2020-07-06 12:40 ` Xiang Zheng
2020-07-06 16:07 ` Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 09/12] iommu/arm-smmu-v3: Check for SVA features Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 10/12] iommu/arm-smmu-v3: Add SVA device feature Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 11/12] iommu/arm-smmu-v3: Implement iommu_sva_bind/unbind() Jean-Philippe Brucker
2020-06-18 15:51 ` [PATCH v8 12/12] iommu/arm-smmu-v3: Hook up ATC invalidation to mm ops Jean-Philippe Brucker
2020-07-09 9:39 ` [PATCH v8 00/12] iommu: Shared Virtual Addressing for SMMUv3 (PT sharing part) Jean-Philippe Brucker
2020-07-20 11:11 ` Will Deacon
2020-07-20 15:39 ` Jean-Philippe Brucker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200713154622.GC3072@willie-the-truck \
--to=will@kernel.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=baolu.lu@linux.intel.com \
--cc=catalin.marinas@arm.com \
--cc=fenghua.yu@intel.com \
--cc=hch@infradead.org \
--cc=iommu@lists.linux-foundation.org \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jean-philippe@linaro.org \
--cc=joro@8bytes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-mm@kvack.org \
--cc=robin.murphy@arm.com \
--cc=xuzaibo@huawei.com \
--cc=zhangfei.gao@linaro.org \
--cc=zhengxiang9@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).