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