diff mbox

[v1,1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

Message ID 20170320153441.2181-2-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée March 20, 2017, 3:34 p.m. UTC 
This was an oversight when the rest of cputlb was being updated. As
before it falls back to the non-atomic version when the host can't
support wider-than-bus atomics.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cputlb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Paolo Bonzini March 20, 2017, 3:43 p.m. UTC | #1 
On 20/03/2017 16:34, Alex Bennée wrote:
>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>  {
> +#if TCG_OVERSIZED_GUEST
>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>          tlb_entry->addr_write = vaddr;
>      }
> +#else
> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);

atomic_read is enough, since we don't care at all about cases where the
address doesn't match.  Otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
> +    }
> +#endif
>  }
Alex Bennée March 20, 2017, 4:16 p.m. UTC | #2 
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/03/2017 16:34, Alex Bennée wrote:
>>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>>  {
>> +#if TCG_OVERSIZED_GUEST
>>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>>          tlb_entry->addr_write = vaddr;
>>      }
>> +#else
>> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
>
> atomic_read is enough, since we don't care at all about cases where the
> address doesn't match.  Otherwise

Good catch. Will fix for my pullreq

>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Paolo
>
>> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
>> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
>> +    }
>> +#endif
>>  }


--
Alex Bennée
Richard Henderson March 20, 2017, 9:49 p.m. UTC | #3 
On 03/21/2017 01:34 AM, Alex Bennée wrote:
> This was an oversight when the rest of cputlb was being updated. As
> before it falls back to the non-atomic version when the host can't
> support wider-than-bus atomics.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  cputlb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/cputlb.c b/cputlb.c
> index f5d056cc08..0d52e45dfd 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -540,9 +540,17 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
>
>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>  {
> +#if TCG_OVERSIZED_GUEST
>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>          tlb_entry->addr_write = vaddr;
>      }
> +#else
> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
> +
> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
> +    }

What's the state machine here?  How can the per-cpu tlb change in a way other 
than dirty->clean / clean->dirty?  AFAIK, we shouldn't be swapping out the tlb 
entry to somthing completely different.

So how does cmpxchg improve over atomic_write?


r~
diff mbox

Patch

diff --git a/cputlb.c b/cputlb.c
index f5d056cc08..0d52e45dfd 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -540,9 +540,17 @@  void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
 
 static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
 {
+#if TCG_OVERSIZED_GUEST
     if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
         tlb_entry->addr_write = vaddr;
     }
+#else
+    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
+
+    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
+        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
+    }
+#endif
 }
 
 /* update the TLB corresponding to virtual page vaddr