The 68040 MMU must not install write permission in the TLB for a page
that is writable but not yet marked modified. If a read fills the TLB
with write permission before the page descriptor M bit is set, a later
write can bypass the descriptor update and the guest never sees the
page become modified.

Track write protection accumulated from upper-level descriptors and
only grant PAGE_WRITE when no descriptor in the walk is write
protected and the leaf page descriptor is marked modified.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3486
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 target/m68k/helper.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 5f91d206f596..2cc19aa567d5 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -737,6 +737,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     uint32_t next;
     target_ulong page_mask;
     bool debug = access_type & ACCESS_DEBUG;
+    bool writeprot = false;
+    uint32_t ptest_sr;
     int page_bits;
     int i;
     MemTxResult txres;
@@ -756,7 +758,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     }
 
     /* Page Table Root Pointer */
-    *prot = PAGE_READ | PAGE_WRITE;
+    *prot = PAGE_READ;
     if (access_type & ACCESS_CODE) {
         *prot |= PAGE_EXEC;
     }
@@ -787,7 +789,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
         if (access_type & ACCESS_PTEST) {
             env->mmu.mmusr |= M68K_MMU_WP_040;
         }
-        *prot &= ~PAGE_WRITE;
+        writeprot = true;
         if (access_type & ACCESS_STORE) {
             return -1;
         }
@@ -814,7 +816,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
         if (access_type & ACCESS_PTEST) {
             env->mmu.mmusr |= M68K_MMU_WP_040;
         }
-        *prot &= ~PAGE_WRITE;
+        writeprot = true;
         if (access_type & ACCESS_STORE) {
             return -1;
         }
@@ -842,10 +844,12 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
             goto txfail;
         }
     }
+    ptest_sr = next & M68K_MMU_SR_MASK_040;
     if (access_type & ACCESS_STORE) {
         if (next & M68K_DESC_WRITEPROT) {
             if (!(next & M68K_DESC_USED) && !debug) {
-                address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+                next |= M68K_DESC_USED;
+                address_space_stl(cs->as, entry, next,
                                   MEMTXATTRS_UNSPECIFIED, &txres);
                 if (txres != MEMTX_OK) {
                     goto txfail;
@@ -853,8 +857,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
             }
         } else if ((next & (M68K_DESC_MODIFIED | M68K_DESC_USED)) !=
                            (M68K_DESC_MODIFIED | M68K_DESC_USED) && !debug) {
-            address_space_stl(cs->as, entry,
-                              next | (M68K_DESC_MODIFIED | M68K_DESC_USED),
+            next |= M68K_DESC_MODIFIED | M68K_DESC_USED;
+            address_space_stl(cs->as, entry, next,
                               MEMTXATTRS_UNSPECIFIED, &txres);
             if (txres != MEMTX_OK) {
                 goto txfail;
@@ -862,7 +866,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
         }
     } else {
         if (!(next & M68K_DESC_USED) && !debug) {
-            address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+            next |= M68K_DESC_USED;
+            address_space_stl(cs->as, entry, next,
                               MEMTXATTRS_UNSPECIFIED, &txres);
             if (txres != MEMTX_OK) {
                 goto txfail;
@@ -880,23 +885,28 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     *physical = (next & page_mask) + (address & (*page_size - 1));
 
     if (access_type & ACCESS_PTEST) {
-        env->mmu.mmusr |= next & M68K_MMU_SR_MASK_040;
+        env->mmu.mmusr |= ptest_sr;
         env->mmu.mmusr |= *physical & 0xfffff000;
         env->mmu.mmusr |= M68K_MMU_R_040;
     }
 
     if (next & M68K_DESC_WRITEPROT) {
-        *prot &= ~PAGE_WRITE;
+        writeprot = true;
         if (access_type & ACCESS_STORE) {
             return -1;
         }
     }
+
     if (next & M68K_DESC_SUPERONLY) {
         if ((access_type & ACCESS_SUPER) == 0) {
             return -1;
         }
     }
 
+    if (!writeprot && (next & M68K_DESC_MODIFIED)) {
+        *prot |= PAGE_WRITE;
+    }
+
     return 0;
 
 txfail:
-- 
2.54.0

On 6/26/26 07:41, Laurent Vivier wrote:
> +    if (!writeprot && (next & M68K_DESC_MODIFIED)) {
> +        *prot |= PAGE_WRITE;
> +    }
> +

Surely easier to clear PAGE_WRITE as we're currently doing than introduce a separate 
writeprot variable.

Don't you also need to sink the existing test against store?  E.g.

     if ((next & M68K_DESC_WRITEPROT) || !(next & M68K_DESC_MODIFIED)) {
         *prot &= ~PAGE_WRITE;
     }
     if ((access_type & ACCESS_STORE) && !(*prot & PAGE_WRITE)) {
         return -1;
     }

Unrelated to MODIFIED, but elsewhere in this function:

(1) I believe the PAGE_EXEC handling is incorrect.  It shouldn't just be set when 
ACCESS_CODE, but set always.

(2) The dirty/used bit update should be rewritten to be atomic, now that we have MTTCG. 
There are a couple of different approaches between {arm, i386} and {riscv, loongarch}.


r~