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;
>
