[PATCH v2 0/3] target/i386: Use probe_access_full_mmu in ptw_translate

[PATCH v2 0/3] target/i386: Use probe_access_full_mmu in ptw_translate

[Richard Henderson]

Changes for v2:
  - Improve probe_access_full{,_mmu} documentation
  - Remove ra parameter from ptw_translate


r~


Richard Henderson (3):
  include/exec: Improve probe_access_full{,_mmu} documentation
  target/i386: Use probe_access_full_mmu in ptw_translate
  target/i386: Remove ra parameter from ptw_translate

 include/exec/exec-all.h              | 29 ++++++++++++++--------------
 target/i386/tcg/sysemu/excp_helper.c | 28 +++++++++++++--------------
 2 files changed, 27 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] include/exec: Improve probe_access_full{, _mmu} documentation

[Richard Henderson]

Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/exec-all.h | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 72240ef426..2e4c4cc4b4 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -368,6 +368,13 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size,
  * The CPUTLBEntryFull structure returned via @pfull is transient
  * and must be consumed or copied immediately, before any further
  * access or changes to TLB @mmu_idx.
+ *
+ * This function will not fault if @nonfault is set, but will
+ * return TLB_INVALID_MASK if the page is not mapped, or is not
+ * accessible with @access_type.
+ *
+ * This function will return TLB_MMIO in order to force the access
+ * to be handled out-of-line if plugins wish to instrument the access.
  */
 int probe_access_full(CPUArchState *env, vaddr addr, int size,
                       MMUAccessType access_type, int mmu_idx,
@@ -375,22 +382,14 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size,
                       CPUTLBEntryFull **pfull, uintptr_t retaddr);
 
 /**
- * probe_access_mmu() - Like probe_access_full except cannot fault and
- * doesn't trigger instrumentation.
+ * probe_access_full_mmu:
+ * Like probe_access_full, except:
  *
- * @env: CPUArchState
- * @vaddr: virtual address to probe
- * @size: size of the probe
- * @access_type: read, write or execute permission
- * @mmu_idx: softmmu index
- * @phost: ptr to return value host address or NULL
- * @pfull: ptr to return value CPUTLBEntryFull structure or NULL
- *
- * The CPUTLBEntryFull structure returned via @pfull is transient
- * and must be consumed or copied immediately, before any further
- * access or changes to TLB @mmu_idx.
- *
- * Returns: TLB flags as per probe_access_flags()
+ * This function is intended to be used for page table accesses by
+ * the target mmu itself.  Since such page walking happens while
+ * handling another potential mmu fault, this function never raises
+ * exceptions (akin to @nonfault true for probe_access_full).
+ * Likewise this function does not trigger plugin instrumentation.
  */
 int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size,
                           MMUAccessType access_type, int mmu_idx,
-- 
2.43.0

[PATCH v2 2/3] target/i386: Use probe_access_full_mmu in ptw_translate

[Richard Henderson]

The probe_access_full_mmu function was designed for this purpose,
and does not report the memory operation event to plugins.

Cc: qemu-stable@nongnu.org
Fixes: 6d03226b422 ("plugins: force slow path when plugins instrument memory ops")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/tcg/sysemu/excp_helper.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 8fb05b1f53..8f4dc08535 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -62,12 +62,11 @@ typedef struct PTETranslate {
 
 static bool ptw_translate(PTETranslate *inout, hwaddr addr, uint64_t ra)
 {
-    CPUTLBEntryFull *full;
     int flags;
 
     inout->gaddr = addr;
-    flags = probe_access_full(inout->env, addr, 0, MMU_DATA_STORE,
-                              inout->ptw_idx, true, &inout->haddr, &full, ra);
+    flags = probe_access_full_mmu(inout->env, addr, 0, MMU_DATA_STORE,
+                                  inout->ptw_idx, &inout->haddr, NULL);
 
     if (unlikely(flags & TLB_INVALID_MASK)) {
         TranslateFault *err = inout->err;
@@ -429,9 +428,8 @@ do_check_protect_pse36:
         CPUTLBEntryFull *full;
         int flags, nested_page_size;
 
-        flags = probe_access_full(env, paddr, 0, access_type,
-                                  MMU_NESTED_IDX, true,
-                                  &pte_trans.haddr, &full, 0);
+        flags = probe_access_full_mmu(env, paddr, 0, access_type,
+                                      MMU_NESTED_IDX, &pte_trans.haddr, &full);
         if (unlikely(flags & TLB_INVALID_MASK)) {
             *err = (TranslateFault){
                 .error_code = env->error_code,
-- 
2.43.0

[PATCH v2 3/3] target/i386: Remove ra parameter from ptw_translate

[Richard Henderson]

This argument is no longer used.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/sysemu/excp_helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 8f4dc08535..f97594f4ab 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -60,7 +60,7 @@ typedef struct PTETranslate {
     hwaddr gaddr;
 } PTETranslate;
 
-static bool ptw_translate(PTETranslate *inout, hwaddr addr, uint64_t ra)
+static bool ptw_translate(PTETranslate *inout, hwaddr addr)
 {
     int flags;
 
@@ -165,7 +165,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
                  * Page table level 5
                  */
                 pte_addr = (in->cr3 & ~0xfff) + (((addr >> 48) & 0x1ff) << 3);
-                if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+                if (!ptw_translate(&pte_trans, pte_addr)) {
                     return false;
                 }
             restart_5:
@@ -189,7 +189,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
              * Page table level 4
              */
             pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 39) & 0x1ff) << 3);
-            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+            if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
         restart_4:
@@ -209,7 +209,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
              * Page table level 3
              */
             pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) << 3);
-            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+            if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
         restart_3_lma:
@@ -236,7 +236,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
              * Page table level 3
              */
             pte_addr = (in->cr3 & 0xffffffe0ULL) + ((addr >> 27) & 0x18);
-            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+            if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
             rsvd_mask |= PG_HI_USER_MASK;
@@ -258,7 +258,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
          * Page table level 2
          */
         pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 21) & 0x1ff) << 3);
-        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+        if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
     restart_2_pae:
@@ -284,7 +284,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
          * Page table level 1
          */
         pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 12) & 0x1ff) << 3);
-        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+        if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
         pte = ptw_ldq(&pte_trans, ra);
@@ -302,7 +302,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
          * Page table level 2
          */
         pte_addr = (in->cr3 & 0xfffff000ULL) + ((addr >> 20) & 0xffc);
-        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+        if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
     restart_2_nopae:
@@ -331,7 +331,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
          * Page table level 1
          */
         pte_addr = (pte & ~0xfffu) + ((addr >> 10) & 0xffc);
-        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
+        if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
         pte = ptw_ldl(&pte_trans, ra);
-- 
2.43.0

Re: [PATCH v2 3/3] target/i386: Remove ra parameter from ptw_translate

[Philippe Mathieu-Daudé]

On 13/10/24 15:47, Richard Henderson wrote:
> This argument is no longer used.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/sysemu/excp_helper.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/3] include/exec: Improve probe_access_full{, _mmu} documentation

[Pierrick Bouvier]

On 10/13/24 11:47, Richard Henderson wrote:
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/exec-all.h | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 72240ef426..2e4c4cc4b4 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -368,6 +368,13 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size,
>    * The CPUTLBEntryFull structure returned via @pfull is transient
>    * and must be consumed or copied immediately, before any further
>    * access or changes to TLB @mmu_idx.
> + *
> + * This function will not fault if @nonfault is set, but will
> + * return TLB_INVALID_MASK if the page is not mapped, or is not
> + * accessible with @access_type.
> + *
> + * This function will return TLB_MMIO in order to force the access
> + * to be handled out-of-line if plugins wish to instrument the access.
>    */
>   int probe_access_full(CPUArchState *env, vaddr addr, int size,
>                         MMUAccessType access_type, int mmu_idx,
> @@ -375,22 +382,14 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size,
>                         CPUTLBEntryFull **pfull, uintptr_t retaddr);
>   
>   /**
> - * probe_access_mmu() - Like probe_access_full except cannot fault and
> - * doesn't trigger instrumentation.
> + * probe_access_full_mmu:
> + * Like probe_access_full, except:
>    *
> - * @env: CPUArchState
> - * @vaddr: virtual address to probe
> - * @size: size of the probe
> - * @access_type: read, write or execute permission
> - * @mmu_idx: softmmu index
> - * @phost: ptr to return value host address or NULL
> - * @pfull: ptr to return value CPUTLBEntryFull structure or NULL
> - *
> - * The CPUTLBEntryFull structure returned via @pfull is transient
> - * and must be consumed or copied immediately, before any further
> - * access or changes to TLB @mmu_idx.
> - *
> - * Returns: TLB flags as per probe_access_flags()
> + * This function is intended to be used for page table accesses by
> + * the target mmu itself.  Since such page walking happens while
> + * handling another potential mmu fault, this function never raises
> + * exceptions (akin to @nonfault true for probe_access_full).
> + * Likewise this function does not trigger plugin instrumentation.
>    */
>   int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size,
>                             MMUAccessType access_type, int mmu_idx,

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v2 1/3] include/exec: Improve probe_access_full{,_mmu} documentation

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 2/3] target/i386: Use probe_access_full_mmu in ptw_translate

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The probe_access_full_mmu function was designed for this purpose,
> and does not report the memory operation event to plugins.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 6d03226b422 ("plugins: force slow path when plugins instrument memory ops")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/3] target/i386: Use probe_access_full_mmu in ptw_translate

[Richard Henderson]

On 10/13/24 11:47, Richard Henderson wrote:
> Changes for v2:
>    - Improve probe_access_full{,_mmu} documentation
>    - Remove ra parameter from ptw_translate
> 
> 
> r~
> 
> 
> Richard Henderson (3):
>    include/exec: Improve probe_access_full{,_mmu} documentation
>    target/i386: Use probe_access_full_mmu in ptw_translate
>    target/i386: Remove ra parameter from ptw_translate
> 
>   include/exec/exec-all.h              | 29 ++++++++++++++--------------
>   target/i386/tcg/sysemu/excp_helper.c | 28 +++++++++++++--------------
>   2 files changed, 27 insertions(+), 30 deletions(-)

Queued.


r~