Re: [RFC v2 11/14] arm64: Move the ASID allocator code in a separate file

2019-07-15 Thread Julien Grall

On 04/07/2019 15:56, James Morse wrote:

Hi Julien,


Hi James,

Thank you for the review.



On 20/06/2019 14:06, Julien Grall wrote:

We will want to re-use the ASID allocator in a separate context (e.g
allocating VMID). So move the code in a new file.

The function asid_check_context has been moved in the header as a static
inline function because we want to avoid add a branch when checking if the
ASID is still valid.



diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index 3df63a28856c..b745cf356fe1 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -23,46 +23,21 @@



-#define ASID_FIRST_VERSION(info)   NUM_ASIDS(info)



diff --git a/arch/arm64/lib/asid.c b/arch/arm64/lib/asid.c
new file mode 100644
index ..7252e4fdd5e9
--- /dev/null
+++ b/arch/arm64/lib/asid.c
@@ -0,0 +1,185 @@



+#define ASID_FIRST_VERSION(info)   (1UL << ((info)->bits))


(oops!)


Good catch, I will fix it in the next version.





@@ -344,7 +115,7 @@ static int asids_init(void)
if (!asid_allocator_init(_info, bits, ASID_PER_CONTEXT,
 asid_flush_cpu_ctxt))
panic("Unable to initialize ASID allocator for %lu ASIDs\n",
- 1UL << bits);
+ NUM_ASIDS(_info));


Could this go in the patch that adds NUM_ASIDS()?


Actually this change is potentially wrong. This relies on asid_allocator_init() 
to set asid_info.bits even if the function fails.


So I think it would be best to keep 1UL << bits here.

Cheers,

--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [RFC v2 11/14] arm64: Move the ASID allocator code in a separate file

2019-07-04 Thread James Morse
Hi Julien,

On 20/06/2019 14:06, Julien Grall wrote:
> We will want to re-use the ASID allocator in a separate context (e.g
> allocating VMID). So move the code in a new file.
> 
> The function asid_check_context has been moved in the header as a static
> inline function because we want to avoid add a branch when checking if the
> ASID is still valid.

> diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
> index 3df63a28856c..b745cf356fe1 100644
> --- a/arch/arm64/mm/context.c
> +++ b/arch/arm64/mm/context.c
> @@ -23,46 +23,21 @@

> -#define ASID_FIRST_VERSION(info) NUM_ASIDS(info)

> diff --git a/arch/arm64/lib/asid.c b/arch/arm64/lib/asid.c
> new file mode 100644
> index ..7252e4fdd5e9
> --- /dev/null
> +++ b/arch/arm64/lib/asid.c
> @@ -0,0 +1,185 @@

> +#define ASID_FIRST_VERSION(info) (1UL << ((info)->bits))

(oops!)


> @@ -344,7 +115,7 @@ static int asids_init(void)
>   if (!asid_allocator_init(_info, bits, ASID_PER_CONTEXT,
>asid_flush_cpu_ctxt))
>   panic("Unable to initialize ASID allocator for %lu ASIDs\n",
> -   1UL << bits);
> +   NUM_ASIDS(_info));

Could this go in the patch that adds NUM_ASIDS()?


Thanks,

James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[RFC v2 11/14] arm64: Move the ASID allocator code in a separate file

2019-06-20 Thread Julien Grall
We will want to re-use the ASID allocator in a separate context (e.g
allocating VMID). So move the code in a new file.

The function asid_check_context has been moved in the header as a static
inline function because we want to avoid add a branch when checking if the
ASID is still valid.

Signed-off-by: Julien Grall 

---

This code will be used in the virt code for allocating VMID. I am not
entirely sure where to place it. Lib could potentially be a good place but I
am not entirely convinced the algo as it is could be used by other
architecture.

Looking at x86, it seems that it will not be possible to re-use because
the number of PCID (aka ASID) could be smaller than the number of CPUs.
See commit message 10af6235e0d327d42e1bad974385197817923dc1 "x86/mm:
Implement PCID based optimization: try to preserve old TLB entries using
PCI".

Changes in v2:
- Rename the header from asid.h to lib_asid.h
---
 arch/arm64/include/asm/lib_asid.h |  77 +
 arch/arm64/lib/Makefile   |   2 +
 arch/arm64/lib/asid.c | 185 ++
 arch/arm64/mm/context.c   | 235 +-
 4 files changed, 267 insertions(+), 232 deletions(-)
 create mode 100644 arch/arm64/include/asm/lib_asid.h
 create mode 100644 arch/arm64/lib/asid.c

diff --git a/arch/arm64/include/asm/lib_asid.h 
b/arch/arm64/include/asm/lib_asid.h
new file mode 100644
index ..c18e9eca500e
--- /dev/null
+++ b/arch/arm64/include/asm/lib_asid.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_ASM_LIB_ASID_H
+#define __ASM_ASM_LIB_ASID_H
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct asid_info
+{
+   atomic64_t  generation;
+   unsigned long   *map;
+   atomic64_t __percpu *active;
+   u64 __percpu*reserved;
+   u32 bits;
+   /* Lock protecting the structure */
+   raw_spinlock_t  lock;
+   /* Which CPU requires context flush on next call */
+   cpumask_t   flush_pending;
+   /* Number of ASID allocated by context (shift value) */
+   unsigned intctxt_shift;
+   /* Callback to locally flush the context. */
+   void(*flush_cpu_ctxt_cb)(void);
+};
+
+#define NUM_ASIDS(info)(1UL << ((info)->bits))
+#define NUM_CTXT_ASIDS(info)   (NUM_ASIDS(info) >> (info)->ctxt_shift)
+
+#define active_asid(info, cpu) *per_cpu_ptr((info)->active, cpu)
+
+void asid_new_context(struct asid_info *info, atomic64_t *pasid,
+ unsigned int cpu);
+
+/*
+ * Check the ASID is still valid for the context. If not generate a new ASID.
+ *
+ * @pasid: Pointer to the current ASID batch
+ * @cpu: current CPU ID. Must have been acquired throught get_cpu()
+ */
+static inline void asid_check_context(struct asid_info *info,
+ atomic64_t *pasid, unsigned int cpu)
+{
+   u64 asid, old_active_asid;
+
+   asid = atomic64_read(pasid);
+
+   /*
+* The memory ordering here is subtle.
+* If our active_asid is non-zero and the ASID matches the current
+* generation, then we update the active_asid entry with a relaxed
+* cmpxchg. Racing with a concurrent rollover means that either:
+*
+* - We get a zero back from the cmpxchg and end up waiting on the
+*   lock. Taking the lock synchronises with the rollover and so
+*   we are forced to see the updated generation.
+*
+* - We get a valid ASID back from the cmpxchg, which means the
+*   relaxed xchg in flush_context will treat us as reserved
+*   because atomic RmWs are totally ordered for a given location.
+*/
+   old_active_asid = atomic64_read(_asid(info, cpu));
+   if (old_active_asid &&
+   !((asid ^ atomic64_read(>generation)) >> info->bits) &&
+   atomic64_cmpxchg_relaxed(_asid(info, cpu),
+old_active_asid, asid))
+   return;
+
+   asid_new_context(info, pasid, cpu);
+}
+
+int asid_allocator_init(struct asid_info *info,
+   u32 bits, unsigned int asid_per_ctxt,
+   void (*flush_cpu_ctxt_cb)(void));
+
+#endif
diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile
index 33c2a4abda04..37169d541ab5 100644
--- a/arch/arm64/lib/Makefile
+++ b/arch/arm64/lib/Makefile
@@ -5,6 +5,8 @@ lib-y   := clear_user.o delay.o copy_from_user.o
\
   memcmp.o strcmp.o strncmp.o strlen.o strnlen.o   \
   strchr.o strrchr.o tishift.o
 
+lib-y  += asid.o
+
 ifeq ($(CONFIG_KERNEL_MODE_NEON), y)
 obj-$(CONFIG_XOR_BLOCKS)   += xor-neon.o
 CFLAGS_REMOVE_xor-neon.o   += -mgeneral-regs-only
diff --git a/arch/arm64/lib/asid.c b/arch/arm64/lib/asid.c
new file mode 100644
index ..7252e4fdd5e9