[PATCH 0/5] target/s390x: Header cleanups around "cpu.h"

[PATCH 0/5] target/s390x: Header cleanups around "cpu.h"

[Philippe Mathieu-Daudé]

These patches are part of a big refactor cleanup
around "cpu.h". Most changes should be trivial IMHO.

Philippe Mathieu-Daudé (5):
  exec/memory: Expose memory_region_access_valid()
  hw/s390x/pv: Un-inline s390_pv_init()
  hw/s390x/pv: Simplify s390_is_pv() for user emulation
  target/s390x/tcg/misc_helper: Remove unused "memory.h" include
  target/s390x/tcg/excp_helper: Restrict system headers to sysemu

 hw/s390x/pv.c                  | 13 +++++++++++++
 hw/s390x/s390-pci-inst.c       |  2 +-
 include/exec/memory-internal.h |  4 ----
 include/exec/memory.h          |  4 ++++
 include/hw/s390x/pv.h          | 20 ++++++++------------
 target/s390x/tcg/excp_helper.c | 10 +++++-----
 target/s390x/tcg/misc_helper.c |  1 -
 7 files changed, 31 insertions(+), 23 deletions(-)

-- 
2.38.1

[PATCH 1/5] exec/memory: Expose memory_region_access_valid()

[Philippe Mathieu-Daudé]

Instead of having hardware device poking into memory
internal API, expose memory_region_access_valid().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/s390-pci-inst.c       | 2 +-
 include/exec/memory-internal.h | 4 ----
 include/exec/memory.h          | 4 ++++
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 66e764f901..35db7777e4 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -13,7 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "exec/memop.h"
-#include "exec/memory-internal.h"
+#include "exec/memory.h"
 #include "qemu/error-report.h"
 #include "sysemu/hw_accel.h"
 #include "hw/s390x/s390-pci-inst.h"
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 9fcc2af25c..100c1237ac 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -38,10 +38,6 @@ void flatview_unref(FlatView *view);
 
 extern const MemoryRegionOps unassigned_mem_ops;
 
-bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
-                                unsigned size, bool is_write,
-                                MemTxAttrs attrs);
-
 void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section);
 AddressSpaceDispatch *address_space_dispatch_new(FlatView *fv);
 void address_space_dispatch_compact(AddressSpaceDispatch *d);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 91f8a2395a..c37ffdbcd1 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2442,6 +2442,10 @@ void memory_global_dirty_log_stop(unsigned int flags);
 
 void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled);
 
+bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
+                                unsigned size, bool is_write,
+                                MemTxAttrs attrs);
+
 /**
  * memory_region_dispatch_read: perform a read directly to the specified
  * MemoryRegion.
-- 
2.38.1

Re: [PATCH 1/5] exec/memory: Expose memory_region_access_valid()

[Richard Henderson]

On 12/16/22 14:04, Philippe Mathieu-Daudé wrote:
> Instead of having hardware device poking into memory
> internal API, expose memory_region_access_valid().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/s390x/s390-pci-inst.c       | 2 +-
>   include/exec/memory-internal.h | 4 ----
>   include/exec/memory.h          | 4 ++++
>   3 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/5] hw/s390x/pv: Un-inline s390_pv_init()

[Philippe Mathieu-Daudé]

There is no point in having s390_pv_init() inlined.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/pv.c         | 13 +++++++++++++
 include/hw/s390x/pv.h | 14 +-------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8dfe92d8df..9c511369b2 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -26,6 +26,19 @@ static bool info_valid;
 static struct kvm_s390_pv_info_vm info_vm;
 static struct kvm_s390_pv_info_dump info_dump;
 
+int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
+{
+    if (!cgs) {
+        return 0;
+    }
+    if (kvm_enabled()) {
+        return s390_pv_kvm_init(cgs, errp);
+    }
+
+    error_setg(errp, "Protected Virtualization requires KVM");
+    return -1;
+}
+
 static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
 {
     struct kvm_pv_cmd pv_cmd = {
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 9360aa1091..5bca5bcaf1 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -12,7 +12,6 @@
 #ifndef HW_S390_PV_H
 #define HW_S390_PV_H
 
-#include "qapi/error.h"
 #include "sysemu/kvm.h"
 
 #ifdef CONFIG_KVM
@@ -78,17 +77,6 @@ static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
 #endif /* CONFIG_KVM */
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
-{
-    if (!cgs) {
-        return 0;
-    }
-    if (kvm_enabled()) {
-        return s390_pv_kvm_init(cgs, errp);
-    }
-
-    error_setg(errp, "Protected Virtualization requires KVM");
-    return -1;
-}
+int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp);
 
 #endif /* HW_S390_PV_H */
-- 
2.38.1

Re: [PATCH 2/5] hw/s390x/pv: Un-inline s390_pv_init()

[Philippe Mathieu-Daudé]

On 16/12/22 23:04, Philippe Mathieu-Daudé wrote:
> There is no point in having s390_pv_init() inlined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/s390x/pv.c         | 13 +++++++++++++
>   include/hw/s390x/pv.h | 14 +-------------
>   2 files changed, 14 insertions(+), 13 deletions(-)


> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 9360aa1091..5bca5bcaf1 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -12,7 +12,6 @@
>   #ifndef HW_S390_PV_H
>   #define HW_S390_PV_H
>   
> -#include "qapi/error.h"
>   #include "sysemu/kvm.h"
>   
>   #ifdef CONFIG_KVM
> @@ -78,17 +77,6 @@ static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
>   #endif /* CONFIG_KVM */
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> -static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
> -{
> -    if (!cgs) {
> -        return 0;
> -    }
> -    if (kvm_enabled()) {
> -        return s390_pv_kvm_init(cgs, errp);
> -    }
> -
> -    error_setg(errp, "Protected Virtualization requires KVM");
> -    return -1;
> -}
> +int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp);

OK I understood why this function is inlined. It is called from:

$ git grep s390_pv_init
hw/s390x/pv.c:29:int s390_pv_init(ConfidentialGuestSupport *cgs, Error 
**errp)
hw/s390x/s390-virtio-ccw.c:259:    s390_pv_init(machine->cgs, &error_fatal);
include/hw/s390x/pv.h:86:int s390_pv_init(ConfidentialGuestSupport *cgs, 
Error **errp);

Now note in meson.build:

s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
   'tod-kvm.c',
   's390-skeys-kvm.c',
   's390-stattrib-kvm.c',
   'pv.c',
   's390-pci-kvm.c',
))

So when CONFIG_KVM=false s390-virtio-ccw.c doesn't have to link with
the kvm-specific pv.o.

I'll rework this patch.

[PATCH 3/5] hw/s390x/pv: Simplify s390_is_pv() for user emulation

[Philippe Mathieu-Daudé]

Protected Virtualization is irrelevant in user emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/s390x/pv.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 5bca5bcaf1..a4814ce303 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -12,6 +12,12 @@
 #ifndef HW_S390_PV_H
 #define HW_S390_PV_H
 
+#ifdef CONFIG_USER_ONLY
+
+static inline bool s390_is_pv(void) { return false; }
+
+#else /* !CONFIG_USER_ONLY */
+
 #include "sysemu/kvm.h"
 
 #ifdef CONFIG_KVM
@@ -79,4 +85,6 @@ static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp);
 
+#endif /* CONFIG_USER_ONLY */
+
 #endif /* HW_S390_PV_H */
-- 
2.38.1

Re: [PATCH 3/5] hw/s390x/pv: Simplify s390_is_pv() for user emulation

[Philippe Mathieu-Daudé]

On 16/12/22 23:04, Philippe Mathieu-Daudé wrote:
> Protected Virtualization is irrelevant in user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/s390x/pv.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 5bca5bcaf1..a4814ce303 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -12,6 +12,12 @@
>   #ifndef HW_S390_PV_H
>   #define HW_S390_PV_H
>   
> +#ifdef CONFIG_USER_ONLY
> +
> +static inline bool s390_is_pv(void) { return false; }

This change is because I'm trying to get ride of:

../target/s390x/cpu_features.c: In function ‘s390_fill_feat_block’:
../target/s390x/cpu_features.c:112:10: error: implicit declaration of 
function ‘s390_is_pv’ [-Werror=implicit-function-declaration]
   112 |     if (!s390_is_pv()) {
       |          ^~~~~~~~~~
../target/s390x/cpu_features.c:112:10: error: nested extern declaration 
of ‘s390_is_pv’ [-Werror=nested-externs]

../target/s390x/cpu_models.c: In function ‘s390_has_feat’:
../target/s390x/cpu_models.c:239:9: error: implicit declaration of 
function ‘s390_is_pv’ [-Werror=implicit-function-declaration]
   239 |     if (s390_is_pv()) {
       |         ^~~~~~~~~~
../target/s390x/cpu_models.c:239:9: error: nested extern declaration of 
‘s390_is_pv’ [-Werror=nested-externs]

without including <sysemu/kvm.h> in common code. Common code shouldn't
include hw/ header...

I'll respin guarding more code against CONFIG_USER_ONLY.

[PATCH 4/5] target/s390x/tcg/misc_helper: Remove unused "memory.h" include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/misc_helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 71388a7119..576157b1f3 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -23,7 +23,6 @@
 #include "qemu/main-loop.h"
 #include "cpu.h"
 #include "s390x-internal.h"
-#include "exec/memory.h"
 #include "qemu/host-utils.h"
 #include "exec/helper-proto.h"
 #include "qemu/timer.h"
-- 
2.38.1

Re: [PATCH 4/5] target/s390x/tcg/misc_helper: Remove unused "memory.h" include

[Richard Henderson]

On 12/16/22 14:04, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/s390x/tcg/misc_helper.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 5/5] target/s390x/tcg/excp_helper: Restrict system headers to sysemu

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/excp_helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index fe02d82201..e9eb7c455e 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -21,15 +21,15 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "cpu.h"
-#include "s390x-internal.h"
 #include "exec/helper-proto.h"
-#include "qemu/timer.h"
 #include "exec/exec-all.h"
-#include "exec/cpu_ldst.h"
-#include "hw/s390x/ioinst.h"
-#include "exec/address-spaces.h"
+#include "s390x-internal.h"
 #include "tcg_s390x.h"
 #ifndef CONFIG_USER_ONLY
+#include "qemu/timer.h"
+#include "exec/cpu_ldst.h"
+#include "exec/address-spaces.h"
+#include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/boards.h"
 #endif
-- 
2.38.1

Re: [PATCH 5/5] target/s390x/tcg/excp_helper: Restrict system headers to sysemu

[Richard Henderson]

On 12/16/22 14:04, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/tcg/excp_helper.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
> index fe02d82201..e9eb7c455e 100644
> --- a/target/s390x/tcg/excp_helper.c
> +++ b/target/s390x/tcg/excp_helper.c
> @@ -21,15 +21,15 @@
>   #include "qemu/osdep.h"
>   #include "qemu/log.h"
>   #include "cpu.h"
> -#include "s390x-internal.h"
>   #include "exec/helper-proto.h"
> -#include "qemu/timer.h"
>   #include "exec/exec-all.h"
> -#include "exec/cpu_ldst.h"
> -#include "hw/s390x/ioinst.h"
> -#include "exec/address-spaces.h"
> +#include "s390x-internal.h"
>   #include "tcg_s390x.h"
>   #ifndef CONFIG_USER_ONLY
> +#include "qemu/timer.h"
> +#include "exec/cpu_ldst.h"

cpu_ldst.h is not a system header.  The others look plausible.


r~

> +#include "exec/address-spaces.h"
> +#include "hw/s390x/ioinst.h"
>   #include "hw/s390x/s390_flic.h"
>   #include "hw/boards.h"
>   #endif

Re: [PATCH 5/5] target/s390x/tcg/excp_helper: Restrict system headers to sysemu

[Philippe Mathieu-Daudé]

On 17/12/22 01:45, Richard Henderson wrote:
> On 12/16/22 14:04, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/s390x/tcg/excp_helper.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/tcg/excp_helper.c 
>> b/target/s390x/tcg/excp_helper.c
>> index fe02d82201..e9eb7c455e 100644
>> --- a/target/s390x/tcg/excp_helper.c
>> +++ b/target/s390x/tcg/excp_helper.c
>> @@ -21,15 +21,15 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/log.h"
>>   #include "cpu.h"
>> -#include "s390x-internal.h"
>>   #include "exec/helper-proto.h"
>> -#include "qemu/timer.h"
>>   #include "exec/exec-all.h"
>> -#include "exec/cpu_ldst.h"
>> -#include "hw/s390x/ioinst.h"
>> -#include "exec/address-spaces.h"
>> +#include "s390x-internal.h"
>>   #include "tcg_s390x.h"
>>   #ifndef CONFIG_USER_ONLY
>> +#include "qemu/timer.h"
>> +#include "exec/cpu_ldst.h"
> 
> cpu_ldst.h is not a system header.  The others look plausible.

Oops, correct.