Re: [Qemu-devel] [PATCH v3 15/15] translate-all: mark updates to PageDesc as atomic

[Paolo Bonzini]

On 30/09/2016 23:31, Alex Bennée wrote:
> Updates to the internal page table are protected by the mmap_lock.
> However for defined C11 semantics things that are access across threads
> need to accessed using at least relaxed atomics.

Again, this is only necessary for the initial load-acquire operation.
Everything else synchronizes indirectly, so:

> +    PageDesc *pd = atomic_rcu_read(lp);

Using atomics here is correct, but I'd write:

        void *p = atomic_rcu_read(lp);

and then assign from p into either "p" or "pp".

>  
> -    if (*lp == NULL) {
> -        return;

Unnecessary and causes indentation changes elsewhere.  Just use "if (p
== NULL) return;".

> -    }
> -    if (level == 0) {
> -        PageDesc *pd = *lp;
> +    if (pd) {
> +        if (level == 0) {
>  
> -        for (i = 0; i < V_L2_SIZE; ++i) {
> -            pd[i].first_tb = NULL;
> -            invalidate_page_bitmap(pd + i);
> -        }
> -    } else {
> -        void **pp = *lp;
> +            for (i = 0; i < V_L2_SIZE; ++i) {
> +                atomic_set(&pd[i].first_tb, NULL);

Right.

> +                invalidate_page_bitmap(pd + i);
> +            }
> +        } else {
> +            void **pp = (void **) pd;
>  
> -        for (i = 0; i < V_L2_SIZE; ++i) {
> -            page_flush_tb_1(level - 1, pp + i);
> +            for (i = 0; i < V_L2_SIZE; ++i) {
> +                page_flush_tb_1(level - 1, pp + i);
> +            }
>          }
>      }
>  }
> @@ -1360,7 +1359,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t 
> start, tb_page_addr_t end,
>      /* we remove all the TBs in the range [start, end[ */
>      /* XXX: see if in some cases it could be faster to invalidate all
>         the code */
> -    tb = p->first_tb;
> +    tb = atomic_read(&p->first_tb);
>      while (tb != NULL) {
>          n = (uintptr_t)tb & 3;
>          tb = (TranslationBlock *)((uintptr_t)tb & ~3);
> @@ -1968,16 +1967,15 @@ void page_set_flags(target_ulong start, target_ulong 
> end, int flags)
>             the code inside.  */
>          if (!(p->flags & PAGE_WRITE) &&
>              (flags & PAGE_WRITE) &&
> -            p->first_tb) {
> +            atomic_read(&p->first_tb)) {
>              tb_invalidate_phys_page(addr, 0);
>          }
> -        p->flags = flags;
> +        atomic_set(&p->flags, flags);

Should not be necessary, p->flags is only accessed under mmap_lock (or
is it)?

Paolo

>      }
>  }
>  
>  int page_check_range(target_ulong start, target_ulong len, int flags)
>  {
> -    PageDesc *p;
>      target_ulong end;
>      target_ulong addr;
>  
> @@ -2003,28 +2001,31 @@ int page_check_range(target_ulong start, target_ulong 
> len, int flags)
>      for (addr = start, len = end - start;
>           len != 0;
>           len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
> -        p = page_find(addr >> TARGET_PAGE_BITS);
> -        if (!p) {
> -            return -1;
> -        }
> -        if (!(p->flags & PAGE_VALID)) {
> -            return -1;
> -        }
> +        PageDesc *p = page_find(addr >> TARGET_PAGE_BITS);
> +        if (p) {
> +            int cur_flags = atomic_read(&p->flags);
>  
> -        if ((flags & PAGE_READ) && !(p->flags & PAGE_READ)) {
> -            return -1;
> -        }
> -        if (flags & PAGE_WRITE) {
> -            if (!(p->flags & PAGE_WRITE_ORG)) {
> +            if (!(cur_flags & PAGE_VALID)) {
>                  return -1;
>              }
> -            /* unprotect the page if it was put read-only because it
> -               contains translated code */
> -            if (!(p->flags & PAGE_WRITE)) {
> -                if (!page_unprotect(addr, 0)) {
> +
> +            if ((flags & PAGE_READ) && !(cur_flags & PAGE_READ)) {
> +                return -1;
> +            }
> +            if (flags & PAGE_WRITE) {
> +                if (!(cur_flags & PAGE_WRITE_ORG)) {
>                      return -1;
>                  }
> +                /* unprotect the page if it was put read-only because it
> +                   contains translated code */
> +                if (!(cur_flags & PAGE_WRITE)) {
> +                    if (!page_unprotect(addr, 0)) {
> +                        return -1;
> +                    }
> +                }
>              }
> +        } else {
> +            return -1;
>          }
>      }
>      return 0;
>