1. Patchew
  2. QEMU
  3. View series

[RFC PATCH] target/ppc: Do not set HPTE R/C bits on !guest_visible xlate

Nicholas Piggin posted 1 patch 11 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250303104755.584897-1-npiggin@gmail.com
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>
target/ppc/mmu-hash32.c | 27 ++++++++++++++-------------
target/ppc/mmu-hash64.c | 27 ++++++++++++++-------------
2 files changed, 28 insertions(+), 26 deletions(-)
[RFC PATCH] target/ppc: Do not set HPTE R/C bits on !guest_visible xlate
Posted by Nicholas Piggin 11 months, 1 week ago 
Perform !guest_visible memory accesses without modifying R/C bits.

It's arguable whether !guest_visible memory accesses should modify
R/C bits. i386 seems to set accessed/dirty bit updates for "probe"
accesses, but ppc with radix MMU does not. Follow the ppc/radix
lead and perform the accesses without updating R/C bits.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/mmu-hash32.c | 27 ++++++++++++++-------------
 target/ppc/mmu-hash64.c | 27 ++++++++++++++-------------
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
index 1f791a7f2f7..b8d7f87507b 100644
--- a/target/ppc/mmu-hash32.c
+++ b/target/ppc/mmu-hash32.c
@@ -410,19 +410,20 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
     qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
 
     /* 8. Update PTE referenced and changed bits if necessary */
-
-    if (!(pte.pte1 & HPTE32_R_R)) {
-        ppc_hash32_set_r(cpu, pte_offset, pte.pte1);
-    }
-    if (!(pte.pte1 & HPTE32_R_C)) {
-        if (access_type == MMU_DATA_STORE) {
-            ppc_hash32_set_c(cpu, pte_offset, pte.pte1);
-        } else {
-            /*
-             * Treat the page as read-only for now, so that a later write
-             * will pass through this function again to set the C bit
-             */
-            prot &= ~PAGE_WRITE;
+    if (guest_visible) {
+        if (!(pte.pte1 & HPTE32_R_R)) {
+            ppc_hash32_set_r(cpu, pte_offset, pte.pte1);
+        }
+        if (!(pte.pte1 & HPTE32_R_C)) {
+            if (access_type == MMU_DATA_STORE) {
+                ppc_hash32_set_c(cpu, pte_offset, pte.pte1);
+            } else {
+                /*
+                 * Treat the page as read-only for now, so that a later write
+                 * will pass through this function again to set the C bit
+                 */
+                prot &= ~PAGE_WRITE;
+            }
         }
     }
     *protp = prot;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 5ca4faee2ab..de5eb5fb221 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -1183,19 +1183,20 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
     qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
 
     /* 6. Update PTE referenced and changed bits if necessary */
-
-    if (!(pte.pte1 & HPTE64_R_R)) {
-        ppc_hash64_set_r(cpu, ptex, pte.pte1);
-    }
-    if (!(pte.pte1 & HPTE64_R_C)) {
-        if (access_type == MMU_DATA_STORE) {
-            ppc_hash64_set_c(cpu, ptex, pte.pte1);
-        } else {
-            /*
-             * Treat the page as read-only for now, so that a later write
-             * will pass through this function again to set the C bit
-             */
-            prot &= ~PAGE_WRITE;
+    if (guest_visible) {
+        if (!(pte.pte1 & HPTE64_R_R)) {
+            ppc_hash64_set_r(cpu, ptex, pte.pte1);
+        }
+        if (!(pte.pte1 & HPTE64_R_C)) {
+            if (access_type == MMU_DATA_STORE) {
+                ppc_hash64_set_c(cpu, ptex, pte.pte1);
+            } else {
+                /*
+                 * Treat the page as read-only for now, so that a later write
+                 * will pass through this function again to set the C bit
+                 */
+                prot &= ~PAGE_WRITE;
+            }
         }
     }
 
-- 
2.47.1
Re: [RFC PATCH] target/ppc: Do not set HPTE R/C bits on !guest_visible xlate
Posted by BALATON Zoltan 11 months, 1 week ago 
On Mon, 3 Mar 2025, Nicholas Piggin wrote:
> Perform !guest_visible memory accesses without modifying R/C bits.
>
> It's arguable whether !guest_visible memory accesses should modify
> R/C bits. i386 seems to set accessed/dirty bit updates for "probe"
> accesses, but ppc with radix MMU does not. Follow the ppc/radix
> lead and perform the accesses without updating R/C bits.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/mmu-hash32.c | 27 ++++++++++++++-------------
> target/ppc/mmu-hash64.c | 27 ++++++++++++++-------------
> 2 files changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> index 1f791a7f2f7..b8d7f87507b 100644
> --- a/target/ppc/mmu-hash32.c
> +++ b/target/ppc/mmu-hash32.c
> @@ -410,19 +410,20 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>     qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
>
>     /* 8. Update PTE referenced and changed bits if necessary */
> -
> -    if (!(pte.pte1 & HPTE32_R_R)) {
> -        ppc_hash32_set_r(cpu, pte_offset, pte.pte1);
> -    }
> -    if (!(pte.pte1 & HPTE32_R_C)) {
> -        if (access_type == MMU_DATA_STORE) {
> -            ppc_hash32_set_c(cpu, pte_offset, pte.pte1);
> -        } else {
> -            /*
> -             * Treat the page as read-only for now, so that a later write
> -             * will pass through this function again to set the C bit
> -             */
> -            prot &= ~PAGE_WRITE;
> +    if (guest_visible) {

Are these unlikely() ? Not sure if that makes a difference but if we know 
it may help some compilers.

Regards,
BALATON Zoltan

> +        if (!(pte.pte1 & HPTE32_R_R)) {
> +            ppc_hash32_set_r(cpu, pte_offset, pte.pte1);
> +        }
> +        if (!(pte.pte1 & HPTE32_R_C)) {
> +            if (access_type == MMU_DATA_STORE) {
> +                ppc_hash32_set_c(cpu, pte_offset, pte.pte1);
> +            } else {
> +                /*
> +                 * Treat the page as read-only for now, so that a later write
> +                 * will pass through this function again to set the C bit
> +                 */
> +                prot &= ~PAGE_WRITE;
> +            }
>         }
>     }
>     *protp = prot;
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 5ca4faee2ab..de5eb5fb221 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1183,19 +1183,20 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>     qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
>
>     /* 6. Update PTE referenced and changed bits if necessary */
> -
> -    if (!(pte.pte1 & HPTE64_R_R)) {
> -        ppc_hash64_set_r(cpu, ptex, pte.pte1);
> -    }
> -    if (!(pte.pte1 & HPTE64_R_C)) {
> -        if (access_type == MMU_DATA_STORE) {
> -            ppc_hash64_set_c(cpu, ptex, pte.pte1);
> -        } else {
> -            /*
> -             * Treat the page as read-only for now, so that a later write
> -             * will pass through this function again to set the C bit
> -             */
> -            prot &= ~PAGE_WRITE;
> +    if (guest_visible) {
> +        if (!(pte.pte1 & HPTE64_R_R)) {
> +            ppc_hash64_set_r(cpu, ptex, pte.pte1);
> +        }
> +        if (!(pte.pte1 & HPTE64_R_C)) {
> +            if (access_type == MMU_DATA_STORE) {
> +                ppc_hash64_set_c(cpu, ptex, pte.pte1);
> +            } else {
> +                /*
> +                 * Treat the page as read-only for now, so that a later write
> +                 * will pass through this function again to set the C bit
> +                 */
> +                prot &= ~PAGE_WRITE;
> +            }
>         }
>     }
>
>
Re: [RFC PATCH] target/ppc: Do not set HPTE R/C bits on !guest_visible xlate
Posted by Nicholas Piggin 11 months, 1 week ago 
On Mon Mar 3, 2025 at 8:58 PM AEST, BALATON Zoltan wrote:
> On Mon, 3 Mar 2025, Nicholas Piggin wrote:
>> Perform !guest_visible memory accesses without modifying R/C bits.
>>
>> It's arguable whether !guest_visible memory accesses should modify
>> R/C bits. i386 seems to set accessed/dirty bit updates for "probe"
>> accesses, but ppc with radix MMU does not. Follow the ppc/radix
>> lead and perform the accesses without updating R/C bits.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> target/ppc/mmu-hash32.c | 27 ++++++++++++++-------------
>> target/ppc/mmu-hash64.c | 27 ++++++++++++++-------------
>> 2 files changed, 28 insertions(+), 26 deletions(-)
>>
>> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
>> index 1f791a7f2f7..b8d7f87507b 100644
>> --- a/target/ppc/mmu-hash32.c
>> +++ b/target/ppc/mmu-hash32.c
>> @@ -410,19 +410,20 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>>     qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
>>
>>     /* 8. Update PTE referenced and changed bits if necessary */
>> -
>> -    if (!(pte.pte1 & HPTE32_R_R)) {
>> -        ppc_hash32_set_r(cpu, pte_offset, pte.pte1);
>> -    }
>> -    if (!(pte.pte1 & HPTE32_R_C)) {
>> -        if (access_type == MMU_DATA_STORE) {
>> -            ppc_hash32_set_c(cpu, pte_offset, pte.pte1);
>> -        } else {
>> -            /*
>> -             * Treat the page as read-only for now, so that a later write
>> -             * will pass through this function again to set the C bit
>> -             */
>> -            prot &= ~PAGE_WRITE;
>> +    if (guest_visible) {
>
> Are these unlikely() ? Not sure if that makes a difference but if we know 
> it may help some compilers.

Yes it probably is. Although we don't tend to use unlikely very much. I
guess we have to start somewhere.

Thanks,
Nick

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.