[PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[Philippe Mathieu-Daudé]

In order to keep trace event headers local to their
directory, introduce s390_skeys_get/s390_skeys_set
helpers, fixing:

  In file included from ../../target/s390x/tcg/mem_helper.c:33:
  ../../target/s390x/tcg/trace.h:1:10: fatal error: 'trace/trace-target_s390x_tcg.h' file not found
  #include "trace/trace-target_s390x_tcg.h"
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1 error generated.
  ninja: build stopped: subcommand failed.

Philippe Mathieu-Daudé (2):
  hw/s390x: Introduce s390_skeys_get|set() helpers
  target/s390x: Use s390_skeys_get|set() helper

 include/hw/s390x/storage-keys.h | 10 ++++++++++
 hw/s390x/s390-skeys.c           | 27 +++++++++++++++++++++++++++
 target/s390x/mmu_helper.c       | 11 ++---------
 target/s390x/tcg/mem_helper.c   | 16 ++++------------
 hw/s390x/trace-events           |  4 ++++
 target/s390x/trace-events       |  4 ----
 6 files changed, 47 insertions(+), 25 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] hw/s390x: Introduce s390_skeys_get|set() helpers

[Philippe Mathieu-Daudé]

s390_skeys_set() dispatch to S390SKeysClass::set_skeys(),
and s390_skeys_get() to S390SKeysClass::get_skeys().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/s390x/storage-keys.h | 10 ++++++++++
 hw/s390x/s390-skeys.c           | 27 +++++++++++++++++++++++++++
 hw/s390x/trace-events           |  4 ++++
 3 files changed, 41 insertions(+)

diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index aa2ec2aae5..976ffb2039 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -111,6 +111,16 @@ struct QEMUS390SKeysState {
 };
 
 void s390_skeys_init(void);
+/**
+ * @s390_skeys_get: See S390SKeysClass::get_skeys()
+ */
+int s390_skeys_get(S390SKeysState *ks, uint64_t start_gfn,
+                   uint64_t count, uint8_t *keys);
+/**
+ * @s390_skeys_set: See S390SKeysClass::set_skeys()
+ */
+int s390_skeys_set(S390SKeysState *ks, uint64_t start_gfn,
+                   uint64_t count, uint8_t *keys);
 
 S390SKeysState *s390_get_skeys_device(void);
 
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5c535d483e..bf22d6863e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -23,6 +23,7 @@
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
+#include "trace.h"
 
 #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -54,6 +55,32 @@ void s390_skeys_init(void)
     qdev_realize(DEVICE(obj), NULL, &error_fatal);
 }
 
+int s390_skeys_get(S390SKeysState *ks, uint64_t start_gfn,
+                   uint64_t count, uint8_t *keys)
+{
+    S390SKeysClass *kc = S390_SKEYS_GET_CLASS(ks);
+    int rc;
+
+    rc = kc->get_skeys(ks, start_gfn, count, keys);
+    if (rc) {
+        trace_s390_skeys_get_nonzero(rc);
+    }
+    return rc;
+}
+
+int s390_skeys_set(S390SKeysState *ks, uint64_t start_gfn,
+                   uint64_t count, uint8_t *keys)
+{
+    S390SKeysClass *kc = S390_SKEYS_GET_CLASS(ks);
+    int rc;
+
+    rc = kc->set_skeys(ks, start_gfn, count, keys);
+    if (rc) {
+        trace_s390_skeys_set_nonzero(rc);
+    }
+    return rc;
+}
+
 static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
                        uint64_t count, Error **errp)
 {
diff --git a/hw/s390x/trace-events b/hw/s390x/trace-events
index 34da5ea323..4e74bf4484 100644
--- a/hw/s390x/trace-events
+++ b/hw/s390x/trace-events
@@ -36,3 +36,7 @@ s390_pci_unknown(const char *msg, uint32_t cmd) "%s unknown command 0x%x"
 s390_pci_bar(uint32_t bar, uint32_t addr, uint64_t size, uint32_t barsize) "bar %d addr 0x%x size 0x%" PRIx64 "barsize 0x%x"
 s390_pci_nodev(const char *cmd, uint32_t fh) "%s no pci dev fh 0x%x"
 s390_pci_invalid(const char *cmd, uint32_t fh) "%s invalid space fh 0x%x"
+
+# s390-skeys.c
+s390_skeys_get_nonzero(int rc) "SKEY: Call to get_skeys unexpectedly returned %d"
+s390_skeys_set_nonzero(int rc) "SKEY: Call to set_skeys unexpectedly returned %d"
-- 
2.41.0

[PATCH v2 2/2] target/s390x: Use s390_skeys_get|set() helper

[Philippe Mathieu-Daudé]

Commit c9274b6bf0 ("target/s390x: start moving TCG-only code
to tcg/") moved mem_helper.c, but the trace-events file is
still in the parent directory, so is the generated trace.h.

Call the s390_skeys_get|set() helper, removing the need
for the trace event shared with the tcg/ sub-directory

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/mmu_helper.c     | 11 ++---------
 target/s390x/tcg/mem_helper.c | 16 ++++------------
 target/s390x/trace-events     |  4 ----
 3 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index f3a2f25a5c..6c59d0d216 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -25,7 +25,6 @@
 #include "sysemu/tcg.h"
 #include "exec/exec-all.h"
 #include "exec/page-protection.h"
-#include "trace.h"
 #include "hw/hw.h"
 #include "hw/s390x/storage-keys.h"
 #include "hw/boards.h"
@@ -303,7 +302,6 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
     static S390SKeysClass *skeyclass;
     static S390SKeysState *ss;
     uint8_t key, old_key;
-    int rc;
 
     /*
      * We expect to be called with an absolute address that has already been
@@ -341,9 +339,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
      *
      * TODO: we have races between getting and setting the key.
      */
-    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-    if (rc) {
-        trace_get_skeys_nonzero(rc);
+    if (s390_skeys_get(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
         return;
     }
     old_key = key;
@@ -371,10 +367,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
     key |= SK_R;
 
     if (key != old_key) {
-        rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-        if (rc) {
-            trace_set_skeys_nonzero(rc);
-        }
+        s390_skeys_set(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     }
 }
 
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6a308c5553..6cdbc34178 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -30,7 +30,6 @@
 #include "hw/core/tcg-cpu-ops.h"
 #include "qemu/int128.h"
 #include "qemu/atomic128.h"
-#include "trace.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/storage-keys.h"
@@ -2093,9 +2092,8 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
         }
     }
 
-    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    rc = s390_skeys_get(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (rc) {
-        trace_get_skeys_nonzero(rc);
         return 0;
     }
     return key;
@@ -2108,7 +2106,6 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
     static S390SKeysClass *skeyclass;
     uint64_t addr = wrap_address(env, r2);
     uint8_t key;
-    int rc;
 
     addr = mmu_real2abs(env, addr);
     if (!mmu_absolute_addr_valid(addr, false)) {
@@ -2124,10 +2121,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
     }
 
     key = r1 & 0xfe;
-    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-    if (rc) {
-        trace_set_skeys_nonzero(rc);
-    }
+    s390_skeys_set(ss, addr / TARGET_PAGE_SIZE, 1, &key);
    /*
     * As we can only flush by virtual address and not all the entries
     * that point to a physical address we have to flush the whole TLB.
@@ -2157,18 +2151,16 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
         }
     }
 
-    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    rc = s390_skeys_get(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (rc) {
-        trace_get_skeys_nonzero(rc);
         return 0;
     }
 
     re = key & (SK_R | SK_C);
     key &= ~SK_R;
 
-    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    rc = s390_skeys_set(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (rc) {
-        trace_set_skeys_nonzero(rc);
         return 0;
     }
    /*
diff --git a/target/s390x/trace-events b/target/s390x/trace-events
index 729cb012b4..d371ef71b9 100644
--- a/target/s390x/trace-events
+++ b/target/s390x/trace-events
@@ -1,9 +1,5 @@
 # See docs/devel/tracing.rst for syntax documentation.
 
-# mmu_helper.c
-get_skeys_nonzero(int rc) "SKEY: Call to get_skeys unexpectedly returned %d"
-set_skeys_nonzero(int rc) "SKEY: Call to set_skeys unexpectedly returned %d"
-
 # ioinst.c
 ioinst(const char *insn) "IOINST: %s"
 ioinst_sch_id(const char *insn, int cssid, int ssid, int schid) "IOINST: %s (%x.%x.%04x)"
-- 
2.41.0

Re: [PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[Philippe Mathieu-Daudé]

ping?

On 13/6/24 12:44, Philippe Mathieu-Daudé wrote:
> In order to keep trace event headers local to their
> directory, introduce s390_skeys_get/s390_skeys_set
> helpers, fixing:
> 
>    In file included from ../../target/s390x/tcg/mem_helper.c:33:
>    ../../target/s390x/tcg/trace.h:1:10: fatal error: 'trace/trace-target_s390x_tcg.h' file not found
>    #include "trace/trace-target_s390x_tcg.h"
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.
>    ninja: build stopped: subcommand failed.
> 
> Philippe Mathieu-Daudé (2):
>    hw/s390x: Introduce s390_skeys_get|set() helpers
>    target/s390x: Use s390_skeys_get|set() helper

Re: [PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[Thomas Huth]

On 13/06/2024 12.44, Philippe Mathieu-Daudé wrote:
> In order to keep trace event headers local to their
> directory, introduce s390_skeys_get/s390_skeys_set
> helpers, fixing:
> 
>    In file included from ../../target/s390x/tcg/mem_helper.c:33:
>    ../../target/s390x/tcg/trace.h:1:10: fatal error: 'trace/trace-target_s390x_tcg.h' file not found
>    #include "trace/trace-target_s390x_tcg.h"
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.
>    ninja: build stopped: subcommand failed.
> 
> Philippe Mathieu-Daudé (2):
>    hw/s390x: Introduce s390_skeys_get|set() helpers
>    target/s390x: Use s390_skeys_get|set() helper
> 
>   include/hw/s390x/storage-keys.h | 10 ++++++++++
>   hw/s390x/s390-skeys.c           | 27 +++++++++++++++++++++++++++
>   target/s390x/mmu_helper.c       | 11 ++---------
>   target/s390x/tcg/mem_helper.c   | 16 ++++------------
>   hw/s390x/trace-events           |  4 ++++
>   target/s390x/trace-events       |  4 ----
>   6 files changed, 47 insertions(+), 25 deletions(-)
> 

Series
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[Paolo Bonzini]

On 6/13/24 12:44, Philippe Mathieu-Daudé wrote:
> In order to keep trace event headers local to their
> directory, introduce s390_skeys_get/s390_skeys_set
> helpers, fixing:
> 
>    In file included from ../../target/s390x/tcg/mem_helper.c:33:
>    ../../target/s390x/tcg/trace.h:1:10: fatal error: 'trace/trace-target_s390x_tcg.h' file not found
>    #include "trace/trace-target_s390x_tcg.h"
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.
>    ninja: build stopped: subcommand failed.
> 
> Philippe Mathieu-Daudé (2):
>    hw/s390x: Introduce s390_skeys_get|set() helpers
>    target/s390x: Use s390_skeys_get|set() helper

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

>   include/hw/s390x/storage-keys.h | 10 ++++++++++
>   hw/s390x/s390-skeys.c           | 27 +++++++++++++++++++++++++++
>   target/s390x/mmu_helper.c       | 11 ++---------
>   target/s390x/tcg/mem_helper.c   | 16 ++++------------
>   hw/s390x/trace-events           |  4 ++++
>   target/s390x/trace-events       |  4 ----
>   6 files changed, 47 insertions(+), 25 deletions(-)
> 

Re: [PATCH v2 0/2] target/s390x: Fix tracing header path in TCG mem_helper.c

[Philippe Mathieu-Daudé]

On 13/6/24 12:44, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (2):
>    hw/s390x: Introduce s390_skeys_get|set() helpers
>    target/s390x: Use s390_skeys_get|set() helper

Series queued, thanks.