Re: [RFC v2 12/14] arm64/lib: asid: Allow user to update the context under the lock

[Julien Grall]

On 03/07/2019 18:35, James Morse wrote:

Hi Julien,


Hi James,


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

Some users of the ASID allocator (e.g VMID) will require to update the
context when a new ASID is generated. This has to be protected by a lock
to prevent concurrent modification.

Rather than introducing yet another lock, it is possible to re-use the
allocator lock for that purpose. This patch introduces a new callback
that will be call when updating the context.


You're using this later in the series to mask out the generation from the 
atomic64 to
leave just the vmid.


You are right.



Where does this concurrent modification happen? The value is only written if we 
have a
rollover, and while its active the only bits that could change are the 
generation.
(subsequent vCPUs that take the slow path for the same VM will see the updated 
generation
and skip the new_context call)

If we did the generation filtering in update_vmid() after the call to
asid_check_context(), what would go wrong?
It happens more often than is necessary and would need a WRITE_ONCE(), but the 
vmid can't
change until we become preemptible and another vCPU gets a chance to make its 
vmid active.


I think I was over cautious. Pre-filtering after asid_check_context() is equally 
fine as long as update_vttbr() is called from preemptible context.


Cheers,

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

Re: [RFC v2 12/14] arm64/lib: asid: Allow user to update the context under the lock

[James Morse]

Hi Julien,

On 20/06/2019 14:06, Julien Grall wrote:
> Some users of the ASID allocator (e.g VMID) will require to update the
> context when a new ASID is generated. This has to be protected by a lock
> to prevent concurrent modification.
> 
> Rather than introducing yet another lock, it is possible to re-use the
> allocator lock for that purpose. This patch introduces a new callback
> that will be call when updating the context.

You're using this later in the series to mask out the generation from the 
atomic64 to
leave just the vmid.

Where does this concurrent modification happen? The value is only written if we 
have a
rollover, and while its active the only bits that could change are the 
generation.
(subsequent vCPUs that take the slow path for the same VM will see the updated 
generation
and skip the new_context call)

If we did the generation filtering in update_vmid() after the call to
asid_check_context(), what would go wrong?
It happens more often than is necessary and would need a WRITE_ONCE(), but the 
vmid can't
change until we become preemptible and another vCPU gets a chance to make its 
vmid active.

This thing is horribly subtle, so I'm sure I've missed something here!

I can't see where the arch code's equivalent case is. It also filters the 
generation out
of the atomic64 in cpu_do_switch_mm(). This happens with interrupts masked so 
we can't
re-schedule and set another asid as 'active'. KVM's equivalent is !preemptible.


Thanks,

James

[RFC v2 12/14] arm64/lib: asid: Allow user to update the context under the lock

[Julien Grall]

Some users of the ASID allocator (e.g VMID) will require to update the
context when a new ASID is generated. This has to be protected by a lock
to prevent concurrent modification.

Rather than introducing yet another lock, it is possible to re-use the
allocator lock for that purpose. This patch introduces a new callback
that will be call when updating the context.

Signed-off-by: Julien Grall 
---
 arch/arm64/include/asm/lib_asid.h | 12 
 arch/arm64/lib/asid.c | 10 --
 arch/arm64/mm/context.c   | 11 ---
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/lib_asid.h 
b/arch/arm64/include/asm/lib_asid.h
index c18e9eca500e..810f0b05a8da 100644
--- a/arch/arm64/include/asm/lib_asid.h
+++ b/arch/arm64/include/asm/lib_asid.h
@@ -23,6 +23,8 @@ struct asid_info
unsigned intctxt_shift;
/* Callback to locally flush the context. */
void(*flush_cpu_ctxt_cb)(void);
+   /* Callback to call when a context is updated */
+   void(*update_ctxt_cb)(void *ctxt);
 };
 
 #define NUM_ASIDS(info)(1UL << ((info)->bits))
@@ -31,7 +33,7 @@ struct asid_info
 #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);
+ unsigned int cpu, void *ctxt);
 
 /*
  * Check the ASID is still valid for the context. If not generate a new ASID.
@@ -40,7 +42,8 @@ void asid_new_context(struct asid_info *info, atomic64_t 
*pasid,
  * @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)
+  atomic64_t *pasid, unsigned int cpu,
+  void *ctxt)
 {
u64 asid, old_active_asid;
 
@@ -67,11 +70,12 @@ static inline void asid_check_context(struct asid_info 
*info,
 old_active_asid, asid))
return;
 
-   asid_new_context(info, pasid, cpu);
+   asid_new_context(info, pasid, cpu, ctxt);
 }
 
 int asid_allocator_init(struct asid_info *info,
u32 bits, unsigned int asid_per_ctxt,
-   void (*flush_cpu_ctxt_cb)(void));
+   void (*flush_cpu_ctxt_cb)(void),
+   void (*update_ctxt_cb)(void *ctxt));
 
 #endif
diff --git a/arch/arm64/lib/asid.c b/arch/arm64/lib/asid.c
index 7252e4fdd5e9..dd2c6e4c1ff0 100644
--- a/arch/arm64/lib/asid.c
+++ b/arch/arm64/lib/asid.c
@@ -130,9 +130,10 @@ static u64 new_context(struct asid_info *info, atomic64_t 
*pasid)
  * @pasid: Pointer to the current ASID batch allocated. It will be updated
  * with the new ASID batch.
  * @cpu: current CPU ID. Must have been acquired through get_cpu()
+ * @ctxt: Context to update when calling update_context
  */
 void asid_new_context(struct asid_info *info, atomic64_t *pasid,
- unsigned int cpu)
+ unsigned int cpu, void *ctxt)
 {
unsigned long flags;
u64 asid;
@@ -149,6 +150,9 @@ void asid_new_context(struct asid_info *info, atomic64_t 
*pasid,
info->flush_cpu_ctxt_cb();
 
atomic64_set(&active_asid(info, cpu), asid);
+
+   info->update_ctxt_cb(ctxt);
+
raw_spin_unlock_irqrestore(&info->lock, flags);
 }
 
@@ -163,11 +167,13 @@ void asid_new_context(struct asid_info *info, atomic64_t 
*pasid,
  */
 int asid_allocator_init(struct asid_info *info,
u32 bits, unsigned int asid_per_ctxt,
-   void (*flush_cpu_ctxt_cb)(void))
+   void (*flush_cpu_ctxt_cb)(void),
+   void (*update_ctxt_cb)(void *ctxt))
 {
info->bits = bits;
info->ctxt_shift = ilog2(asid_per_ctxt);
info->flush_cpu_ctxt_cb = flush_cpu_ctxt_cb;
+   info->update_ctxt_cb = update_ctxt_cb;
/*
 * Expect allocation after rollover to fail if we don't have at least
 * one more ASID than CPUs. ASID #0 is always reserved.
diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index b745cf356fe1..527ea82983d7 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -82,7 +82,7 @@ void check_and_switch_context(struct mm_struct *mm, unsigned 
int cpu)
if (system_supports_cnp())
cpu_set_reserved_ttbr0();
 
-   asid_check_context(&asid_info, &mm->context.id, cpu);
+   asid_check_context(&asid_info, &mm->context.id, cpu, mm);
 
arm64_apply_bp_hardening();
 
@@ -108,12 +108,17 @@ static void asid_flush_cpu_ctxt(void)
local_flush_tlb_all();
 }
 
+static void asid_update_ctxt(void *ctxt)
+{
+   /* Nothing to do */
+}
+
 static int asids_init(void)
 {
u32 bits = get_cpu_asid_b