[PATCH 00/10] accel/kvm: Spring cleaning

[PATCH 00/10] accel/kvm: Spring cleaning

[Philippe Mathieu-Daudé]

Misc KVM-related cleanup patches split from a bigger
"extract KVM specific fields from CPUState" series.

Philippe Mathieu-Daudé (10):
  sysemu/kvm: Remove unused headers
  accel/kvm: Declare kvm_direct_msi_allowed in stubs
  hw/intc/arm_gic: Un-inline GIC*/ITS class_name() helpers
  hw/intc/arm_gic: Rename 'first_cpu' argument
  hw/arm/sbsa-ref: Include missing 'sysemu/kvm.h' header
  target/arm: Reduce QMP header pressure by not including 'kvm_arm.h'
  target/arm: Restrict KVM-specific fields from ArchCPU
  target/ppc: Restrict KVM-specific field from ArchCPU
  target/riscv: Restrict KVM-specific fields from ArchCPU
  hw/s390x: Rename pv.c -> pv-kvm.c

 include/hw/intc/arm_gic.h              |  2 ++
 include/hw/intc/arm_gicv3_common.h     | 10 ++++++
 include/hw/intc/arm_gicv3_its_common.h |  9 ++++++
 include/sysemu/kvm.h                   |  3 --
 target/arm/cpu.h                       |  2 ++
 target/arm/kvm_arm.h                   | 45 --------------------------
 target/ppc/cpu.h                       |  2 ++
 target/riscv/cpu.h                     |  2 ++
 accel/stubs/kvm-stub.c                 |  1 +
 hw/arm/sbsa-ref.c                      |  1 +
 hw/arm/virt-acpi-build.c               |  2 +-
 hw/arm/virt.c                          |  1 +
 hw/intc/arm_gic_common.c               | 12 +++++--
 hw/intc/arm_gicv3_common.c             | 14 ++++++++
 hw/intc/arm_gicv3_its_common.c         | 12 +++++++
 hw/s390x/{pv.c => pv-kvm.c}            |  0
 target/arm/arm-qmp-cmds.c              |  3 +-
 target/ppc/mmu_common.c                |  4 +++
 target/riscv/machine.c                 |  4 +++
 hw/s390x/meson.build                   |  2 +-
 20 files changed, 78 insertions(+), 53 deletions(-)
 rename hw/s390x/{pv.c => pv-kvm.c} (100%)

-- 
2.38.1

[PATCH 01/10] sysemu/kvm: Remove unused headers

[Philippe Mathieu-Daudé]

All types used are forward-declared in "qemu/typedefs.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/kvm.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index cc6c678ed8..7902acdfd9 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -16,9 +16,6 @@
 #ifndef QEMU_KVM_H
 #define QEMU_KVM_H
 
-#include "qemu/queue.h"
-#include "hw/core/cpu.h"
-#include "exec/memattrs.h"
 #include "qemu/accel.h"
 #include "qom/object.h"
 
-- 
2.38.1

Re: [PATCH 01/10] sysemu/kvm: Remove unused headers

[Alex Bennée]

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

> All types used are forward-declared in "qemu/typedefs.h".
>
> Signed-off-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 01/10] sysemu/kvm: Remove unused headers

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> All types used are forward-declared in "qemu/typedefs.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/sysemu/kvm.h | 3 ---
>   1 file changed, 3 deletions(-)

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

r~

Re: [PATCH 01/10] sysemu/kvm: Remove unused headers

[Philippe Mathieu-Daudé]

On 5/4/23 18:04, Philippe Mathieu-Daudé wrote:
> All types used are forward-declared in "qemu/typedefs.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/sysemu/kvm.h | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index cc6c678ed8..7902acdfd9 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -16,9 +16,6 @@
>   #ifndef QEMU_KVM_H
>   #define QEMU_KVM_H
>   
> -#include "qemu/queue.h"
> -#include "hw/core/cpu.h"
> -#include "exec/memattrs.h"

Oops this is incorrect...

   MemTxAttrs kvm_arch_post_run(CPUState *cpu, struct kvm_run *run);

MemTxAttrs is not:
- forward-declared
- used as pointer

Since this is now merged as commit 1e05888ab5 I'll send a fix.

>   #include "qemu/accel.h"
>   #include "qom/object.h"
>   

[PATCH 02/10] accel/kvm: Declare kvm_direct_msi_allowed in stubs

[Philippe Mathieu-Daudé]

Avoid when calling kvm_direct_msi_enabled() from
arm_gicv3_its_common.c the next commit:

  Undefined symbols for architecture arm64:
    "_kvm_direct_msi_allowed", referenced from:
        _its_class_name in hw_intc_arm_gicv3_its_common.c.o
  ld: symbol(s) not found for architecture arm64

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/stubs/kvm-stub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 5d2dd8f351..235dc661bc 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -27,6 +27,7 @@ bool kvm_allowed;
 bool kvm_readonly_mem_allowed;
 bool kvm_ioeventfd_any_length_allowed;
 bool kvm_msi_use_devid;
+bool kvm_direct_msi_allowed;
 
 void kvm_flush_coalesced_mmio_buffer(void)
 {
-- 
2.38.1

Re: [PATCH 02/10] accel/kvm: Declare kvm_direct_msi_allowed in stubs

[Alex Bennée]

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

> Avoid when calling kvm_direct_msi_enabled() from
> arm_gicv3_its_common.c the next commit:
>
>   Undefined symbols for architecture arm64:
>     "_kvm_direct_msi_allowed", referenced from:
>         _its_class_name in hw_intc_arm_gicv3_its_common.c.o
>   ld: symbol(s) not found for architecture arm64
>
> Signed-off-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 02/10] accel/kvm: Declare kvm_direct_msi_allowed in stubs

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> Avoid when calling kvm_direct_msi_enabled() from
> arm_gicv3_its_common.c the next commit:
> 
>    Undefined symbols for architecture arm64:
>      "_kvm_direct_msi_allowed", referenced from:
>          _its_class_name in hw_intc_arm_gicv3_its_common.c.o
>    ld: symbol(s) not found for architecture arm64
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   accel/stubs/kvm-stub.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

[PATCH 03/10] hw/intc/arm_gic: Un-inline GIC*/ITS class_name() helpers

[Philippe Mathieu-Daudé]

"kvm_arm.h" contains external and internal prototype declarations.
Files under the hw/ directory should only access the KVM external
API.

In order to avoid machine / device models to include "kvm_arm.h"
simply to get the QOM GIC/ITS class name, un-inline each class
name getter to the proper device model file.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/intc/arm_gic.h              |  2 ++
 include/hw/intc/arm_gicv3_common.h     | 10 ++++++
 include/hw/intc/arm_gicv3_its_common.h |  9 ++++++
 target/arm/kvm_arm.h                   | 45 --------------------------
 hw/arm/virt-acpi-build.c               |  2 +-
 hw/arm/virt.c                          |  1 +
 hw/intc/arm_gic_common.c               |  7 ++++
 hw/intc/arm_gicv3_common.c             | 14 ++++++++
 hw/intc/arm_gicv3_its_common.c         | 12 +++++++
 9 files changed, 56 insertions(+), 46 deletions(-)

diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
index 116ccbb5a9..48f6a51a70 100644
--- a/include/hw/intc/arm_gic.h
+++ b/include/hw/intc/arm_gic.h
@@ -86,4 +86,6 @@ struct ARMGICClass {
     DeviceRealize parent_realize;
 };
 
+const char *gic_class_name(void);
+
 #endif
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index ab5182a28a..4e2fb518e7 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -329,4 +329,14 @@ struct ARMGICv3CommonClass {
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
                               const MemoryRegionOps *ops);
 
+/**
+ * gicv3_class_name
+ *
+ * Return name of GICv3 class to use depending on whether KVM acceleration is
+ * in use. May throw an error if the chosen implementation is not available.
+ *
+ * Returns: class name to use
+ */
+const char *gicv3_class_name(void);
+
 #endif
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index a11a0f6654..7dc712b38d 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -122,5 +122,14 @@ struct GICv3ITSCommonClass {
     void (*post_load)(GICv3ITSState *s);
 };
 
+/**
+ * its_class_name:
+ *
+ * Return the ITS class name to use depending on whether KVM acceleration
+ * and KVM CAP_SIGNAL_MSI are supported
+ *
+ * Returns: class name to use or NULL
+ */
+const char *its_class_name(void);
 
 #endif
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 99017b635c..fe6d824a52 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -445,32 +445,6 @@ static inline uint32_t kvm_arm_sve_get_vls(CPUState *cs)
 
 #endif
 
-static inline const char *gic_class_name(void)
-{
-    return kvm_irqchip_in_kernel() ? "kvm-arm-gic" : "arm_gic";
-}
-
-/**
- * gicv3_class_name
- *
- * Return name of GICv3 class to use depending on whether KVM acceleration is
- * in use. May throw an error if the chosen implementation is not available.
- *
- * Returns: class name to use
- */
-static inline const char *gicv3_class_name(void)
-{
-    if (kvm_irqchip_in_kernel()) {
-        return "kvm-arm-gicv3";
-    } else {
-        if (kvm_enabled()) {
-            error_report("Userspace GICv3 is not supported with KVM");
-            exit(1);
-        }
-        return "arm-gicv3";
-    }
-}
-
 /**
  * kvm_arm_handle_debug:
  * @cs: CPUState
@@ -508,23 +482,4 @@ void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
  */
 bool kvm_arm_verify_ext_dabt_pending(CPUState *cs);
 
-/**
- * its_class_name:
- *
- * Return the ITS class name to use depending on whether KVM acceleration
- * and KVM CAP_SIGNAL_MSI are supported
- *
- * Returns: class name to use or NULL
- */
-static inline const char *its_class_name(void)
-{
-    if (kvm_irqchip_in_kernel()) {
-        /* KVM implementation requires this capability */
-        return kvm_direct_msi_enabled() ? "arm-its-kvm" : NULL;
-    } else {
-        /* Software emulation based model */
-        return "arm-gicv3-its";
-    }
-}
-
 #endif
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4156111d49..e8bab19847 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -48,12 +48,12 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/arm/virt.h"
+#include "hw/intc/arm_gicv3_its_common.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/platform-bus.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "sysemu/tpm.h"
-#include "kvm_arm.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/ghes.h"
 #include "hw/acpi/viot.h"
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1fe39c6683..dbbe639e61 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -63,6 +63,7 @@
 #include "hw/arm/fdt.h"
 #include "hw/intc/arm_gic.h"
 #include "hw/intc/arm_gicv3_common.h"
+#include "hw/intc/arm_gicv3_its_common.h"
 #include "hw/irq.h"
 #include "kvm_arm.h"
 #include "hw/firmware/smbios.h"
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index a379cea395..9702197856 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -21,10 +21,12 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "gic_internal.h"
 #include "hw/arm/linux-boot-if.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "sysemu/kvm.h"
 
 static int gic_pre_save(void *opaque)
 {
@@ -393,3 +395,8 @@ static void register_types(void)
 }
 
 type_init(register_types)
+
+const char *gic_class_name(void)
+{
+    return kvm_irqchip_in_kernel() ? "kvm-arm-gic" : "arm_gic";
+}
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 642a8243ed..2ebf880ead 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -24,6 +24,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "hw/core/cpu.h"
 #include "hw/intc/arm_gicv3_common.h"
 #include "hw/qdev-properties.h"
@@ -608,3 +609,16 @@ static void register_types(void)
 }
 
 type_init(register_types)
+
+const char *gicv3_class_name(void)
+{
+    if (kvm_irqchip_in_kernel()) {
+        return "kvm-arm-gicv3";
+    } else {
+        if (kvm_enabled()) {
+            error_report("Userspace GICv3 is not supported with KVM");
+            exit(1);
+        }
+        return "arm-gicv3";
+    }
+}
diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index d7532a7a89..abaf77057e 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -24,6 +24,7 @@
 #include "hw/intc/arm_gicv3_its_common.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "sysemu/kvm.h"
 
 static int gicv3_its_pre_save(void *opaque)
 {
@@ -158,3 +159,14 @@ static void gicv3_its_common_register_types(void)
 }
 
 type_init(gicv3_its_common_register_types)
+
+const char *its_class_name(void)
+{
+    if (kvm_irqchip_in_kernel()) {
+        /* KVM implementation requires this capability */
+        return kvm_direct_msi_enabled() ? "arm-its-kvm" : NULL;
+    } else {
+        /* Software emulation based model */
+        return "arm-gicv3-its";
+    }
+}
-- 
2.38.1

Re: [PATCH 03/10] hw/intc/arm_gic: Un-inline GIC*/ITS class_name() helpers

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> "kvm_arm.h" contains external and internal prototype declarations.
> Files under the hw/ directory should only access the KVM external
> API.
> 
> In order to avoid machine / device models to include "kvm_arm.h"
> simply to get the QOM GIC/ITS class name, un-inline each class
> name getter to the proper device model file.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/intc/arm_gic.h              |  2 ++
>   include/hw/intc/arm_gicv3_common.h     | 10 ++++++
>   include/hw/intc/arm_gicv3_its_common.h |  9 ++++++
>   target/arm/kvm_arm.h                   | 45 --------------------------
>   hw/arm/virt-acpi-build.c               |  2 +-
>   hw/arm/virt.c                          |  1 +
>   hw/intc/arm_gic_common.c               |  7 ++++
>   hw/intc/arm_gicv3_common.c             | 14 ++++++++
>   hw/intc/arm_gicv3_its_common.c         | 12 +++++++
>   9 files changed, 56 insertions(+), 46 deletions(-)

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

r~

[PATCH 04/10] hw/intc/arm_gic: Rename 'first_cpu' argument

[Philippe Mathieu-Daudé]

"hw/core/cpu.h" defines 'first_cpu' as QTAILQ_FIRST_RCU(&cpus).

arm_gic_common_reset_irq_state() calls its second argument
'first_cpu', producing a build failure when "hw/core/cpu.h"
is included:

  hw/intc/arm_gic_common.c:238:68: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
    static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
                                                                       ^
  include/hw/core/cpu.h:451:26: note: expanded from macro 'first_cpu'
    #define first_cpu        QTAILQ_FIRST_RCU(&cpus)
                             ^

KISS, rename the function argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/arm_gic_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 9702197856..889327a8cf 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -235,12 +235,13 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
     }
 }
 
-static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
+static inline void arm_gic_common_reset_irq_state(GICState *s,
+                                                  int first_cpu_index,
                                                   int resetprio)
 {
     int i, j;
 
-    for (i = first_cpu; i < first_cpu + s->num_cpu; i++) {
+    for (i = first_cpu_index; i < first_cpu_index + s->num_cpu; i++) {
         if (s->revision == REV_11MPCORE) {
             s->priority_mask[i] = 0xf0;
         } else {
-- 
2.38.1

Re: [PATCH 04/10] hw/intc/arm_gic: Rename 'first_cpu' argument

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> "hw/core/cpu.h" defines 'first_cpu' as QTAILQ_FIRST_RCU(&cpus).
> 
> arm_gic_common_reset_irq_state() calls its second argument
> 'first_cpu', producing a build failure when "hw/core/cpu.h"
> is included:
> 
>    hw/intc/arm_gic_common.c:238:68: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
>      static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
>                                                                         ^
>    include/hw/core/cpu.h:451:26: note: expanded from macro 'first_cpu'
>      #define first_cpu        QTAILQ_FIRST_RCU(&cpus)
>                               ^
> 
> KISS, rename the function argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/intc/arm_gic_common.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Wow, that's ugly.  But a reasonable work-around.


r~

Re: [PATCH 04/10] hw/intc/arm_gic: Rename 'first_cpu' argument

[Richard Henderson]

On 4/7/23 21:23, Richard Henderson wrote:
> On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
>> "hw/core/cpu.h" defines 'first_cpu' as QTAILQ_FIRST_RCU(&cpus).
>>
>> arm_gic_common_reset_irq_state() calls its second argument
>> 'first_cpu', producing a build failure when "hw/core/cpu.h"
>> is included:
>>
>>    hw/intc/arm_gic_common.c:238:68: warning: omitting the parameter name in a function 
>> definition is a C2x extension [-Wc2x-extensions]
>>      static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
>>                                                                         ^
>>    include/hw/core/cpu.h:451:26: note: expanded from macro 'first_cpu'
>>      #define first_cpu        QTAILQ_FIRST_RCU(&cpus)
>>                               ^
>>
>> KISS, rename the function argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
>> ---
>>   hw/intc/arm_gic_common.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Wow, that's ugly.  But a reasonable work-around.

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


r~

Re: [PATCH 04/10] hw/intc/arm_gic: Rename 'first_cpu' argument

[Alex Bennée]

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

> "hw/core/cpu.h" defines 'first_cpu' as QTAILQ_FIRST_RCU(&cpus).
>
> arm_gic_common_reset_irq_state() calls its second argument
> 'first_cpu', producing a build failure when "hw/core/cpu.h"
> is included:
>
>   hw/intc/arm_gic_common.c:238:68: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
>     static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
>                                                                        ^
>   include/hw/core/cpu.h:451:26: note: expanded from macro 'first_cpu'
>     #define first_cpu        QTAILQ_FIRST_RCU(&cpus)
>                              ^
>
> KISS, rename the function argument.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/intc/arm_gic_common.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 9702197856..889327a8cf 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -235,12 +235,13 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
>      }
>  }
>  
> -static inline void arm_gic_common_reset_irq_state(GICState *s, int first_cpu,
> +static inline void arm_gic_common_reset_irq_state(GICState *s,
> +                                                  int
> first_cpu_index,

I'd have gone for a shorter name like cidx maybe, naming things is hard.

Anyway:

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


>                                                    int resetprio)
>  {
>      int i, j;
>  
> -    for (i = first_cpu; i < first_cpu + s->num_cpu; i++) {
> +    for (i = first_cpu_index; i < first_cpu_index + s->num_cpu; i++) {
>          if (s->revision == REV_11MPCORE) {
>              s->priority_mask[i] = 0xf0;
>          } else {


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 05/10] hw/arm/sbsa-ref: Include missing 'sysemu/kvm.h' header

[Philippe Mathieu-Daudé]

"sysemu/kvm.h" is indirectly pulled in. Explicit its
inclusion to avoid when refactoring include/:

  hw/arm/sbsa-ref.c:693:9: error: implicit declaration of function 'kvm_enabled' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    if (kvm_enabled()) {
        ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/sbsa-ref.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 0b93558dde..7df4d7b712 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -26,6 +26,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/kvm.h"
 #include "exec/hwaddr.h"
 #include "kvm_arm.h"
 #include "hw/arm/boot.h"
-- 
2.38.1

Re: [PATCH 05/10] hw/arm/sbsa-ref: Include missing 'sysemu/kvm.h' header

[Leif Lindholm]

On Wed, Apr 05, 2023 at 18:04:49 +0200, Philippe Mathieu-Daudé wrote:
> "sysemu/kvm.h" is indirectly pulled in. Explicit its
> inclusion to avoid when refactoring include/:
> 
>   hw/arm/sbsa-ref.c:693:9: error: implicit declaration of function 'kvm_enabled' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>     if (kvm_enabled()) {
>         ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/sbsa-ref.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 0b93558dde..7df4d7b712 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -26,6 +26,7 @@
>  #include "sysemu/numa.h"
>  #include "sysemu/runstate.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/kvm.h"

Can I do my traditional nitpick and ask this to be added above
sysemu/numa.h in order to maintain alphabetical ordering within the
sysemu block?

With that:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>


>  #include "exec/hwaddr.h"
>  #include "kvm_arm.h"
>  #include "hw/arm/boot.h"
> -- 
> 2.38.1
> 

Re: [PATCH 05/10] hw/arm/sbsa-ref: Include missing 'sysemu/kvm.h' header

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> "sysemu/kvm.h" is indirectly pulled in. Explicit its
> inclusion to avoid when refactoring include/:
> 
>    hw/arm/sbsa-ref.c:693:9: error: implicit declaration of function 'kvm_enabled' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>      if (kvm_enabled()) {
>          ^
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

[PATCH 06/10] target/arm: Reduce QMP header pressure by not including 'kvm_arm.h'

[Philippe Mathieu-Daudé]

We only need "sysemu/kvm.h" for kvm_enabled() and "cpu.h"
for the QOM type definitions (TYPE_ARM_CPU). Avoid including
the heavy "kvm_arm.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/arm-qmp-cmds.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index c8fa524002..91eb450565 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -22,7 +22,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/boards.h"
-#include "kvm_arm.h"
+#include "sysemu/kvm.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qapi/qobject-input-visitor.h"
@@ -31,6 +31,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
 #include "qom/qom-qobject.h"
+#include "target/arm/cpu.h"
 
 static GICCapability *gic_cap_new(int version)
 {
-- 
2.38.1

Re: [PATCH 06/10] target/arm: Reduce QMP header pressure by not including 'kvm_arm.h'

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> We only need "sysemu/kvm.h" for kvm_enabled() and "cpu.h"
> for the QOM type definitions (TYPE_ARM_CPU). Avoid including
> the heavy "kvm_arm.h" header.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/arm-qmp-cmds.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

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

r~

[PATCH 07/10] target/arm: Restrict KVM-specific fields from ArchCPU

[Philippe Mathieu-Daudé]

These fields shouldn't be accessed when KVM is not available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index c097cae988..efd1b010d5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -945,6 +945,7 @@ struct ArchCPU {
      */
     uint32_t kvm_target;
 
+#ifdef CONFIG_KVM
     /* KVM init features for this CPU */
     uint32_t kvm_init_features[7];
 
@@ -957,6 +958,7 @@ struct ArchCPU {
 
     /* KVM steal time */
     OnOffAuto kvm_steal_time;
+#endif /* CONFIG_KVM */
 
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
-- 
2.38.1

Re: [PATCH 07/10] target/arm: Restrict KVM-specific fields from ArchCPU

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> These fields shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/cpu.h | 2 ++
>   1 file changed, 2 insertions(+)

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

r~

[PATCH 08/10] target/ppc: Restrict KVM-specific field from ArchCPU

[Philippe Mathieu-Daudé]

The 'kvm_sw_tlb' field shouldn't be accessed when KVM is not available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/cpu.h        | 2 ++
 target/ppc/mmu_common.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 557d736dab..0ec3957397 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1148,7 +1148,9 @@ struct CPUArchState {
     int tlb_type;    /* Type of TLB we're dealing with */
     ppc_tlb_t tlb;   /* TLB is optional. Allocate them only if needed */
     bool tlb_dirty;  /* Set to non-zero when modifying TLB */
+#ifdef CONFIG_KVM
     bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active */
+#endif /* CONFIG_KVM */
     uint32_t tlb_need_flush; /* Delayed flush needed */
 #define TLB_NEED_LOCAL_FLUSH   0x1
 #define TLB_NEED_GLOBAL_FLUSH  0x2
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 7235a4befe..21843c69f6 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -917,10 +917,12 @@ static void mmubooke_dump_mmu(CPUPPCState *env)
     ppcemb_tlb_t *entry;
     int i;
 
+#ifdef CONFIG_KVM
     if (kvm_enabled() && !env->kvm_sw_tlb) {
         qemu_printf("Cannot access KVM TLB\n");
         return;
     }
+#endif
 
     qemu_printf("\nTLB:\n");
     qemu_printf("Effective          Physical           Size PID   Prot     "
@@ -1008,10 +1010,12 @@ static void mmubooke206_dump_mmu(CPUPPCState *env)
     int offset = 0;
     int i;
 
+#ifdef CONFIG_KVM
     if (kvm_enabled() && !env->kvm_sw_tlb) {
         qemu_printf("Cannot access KVM TLB\n");
         return;
     }
+#endif
 
     for (i = 0; i < BOOKE206_MAX_TLBN; i++) {
         int size = booke206_tlb_size(env, i);
-- 
2.38.1

Re: [PATCH 08/10] target/ppc: Restrict KVM-specific field from ArchCPU

[Daniel Henrique Barboza]

On 4/5/23 13:04, Philippe Mathieu-Daudé wrote:
> The 'kvm_sw_tlb' field shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   target/ppc/cpu.h        | 2 ++
>   target/ppc/mmu_common.c | 4 ++++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 557d736dab..0ec3957397 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1148,7 +1148,9 @@ struct CPUArchState {
>       int tlb_type;    /* Type of TLB we're dealing with */
>       ppc_tlb_t tlb;   /* TLB is optional. Allocate them only if needed */
>       bool tlb_dirty;  /* Set to non-zero when modifying TLB */
> +#ifdef CONFIG_KVM
>       bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active */
> +#endif /* CONFIG_KVM */
>       uint32_t tlb_need_flush; /* Delayed flush needed */
>   #define TLB_NEED_LOCAL_FLUSH   0x1
>   #define TLB_NEED_GLOBAL_FLUSH  0x2
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 7235a4befe..21843c69f6 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -917,10 +917,12 @@ static void mmubooke_dump_mmu(CPUPPCState *env)
>       ppcemb_tlb_t *entry;
>       int i;
>   
> +#ifdef CONFIG_KVM
>       if (kvm_enabled() && !env->kvm_sw_tlb) {
>           qemu_printf("Cannot access KVM TLB\n");
>           return;
>       }
> +#endif
>   
>       qemu_printf("\nTLB:\n");
>       qemu_printf("Effective          Physical           Size PID   Prot     "
> @@ -1008,10 +1010,12 @@ static void mmubooke206_dump_mmu(CPUPPCState *env)
>       int offset = 0;
>       int i;
>   
> +#ifdef CONFIG_KVM
>       if (kvm_enabled() && !env->kvm_sw_tlb) {
>           qemu_printf("Cannot access KVM TLB\n");
>           return;
>       }
> +#endif
>   
>       for (i = 0; i < BOOKE206_MAX_TLBN; i++) {
>           int size = booke206_tlb_size(env, i);

Re: [PATCH 08/10] target/ppc: Restrict KVM-specific field from ArchCPU

[Cédric Le Goater]

Hello Philippe

On 4/5/23 18:04, Philippe Mathieu-Daudé wrote:
> The 'kvm_sw_tlb' field shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/cpu.h        | 2 ++
>   target/ppc/mmu_common.c | 4 ++++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 557d736dab..0ec3957397 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1148,7 +1148,9 @@ struct CPUArchState {
>       int tlb_type;    /* Type of TLB we're dealing with */
>       ppc_tlb_t tlb;   /* TLB is optional. Allocate them only if needed */
>       bool tlb_dirty;  /* Set to non-zero when modifying TLB */

'tlb_dirty' was part of the same commit 93dd5e852c ("kvm: ppc: booke206:
use MMU API"). So we might as well include it in the #ifdef section.

Thanks,

C.

> +#ifdef CONFIG_KVM
>       bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active */
> +#endif /* CONFIG_KVM */
>       uint32_t tlb_need_flush; /* Delayed flush needed */
>   #define TLB_NEED_LOCAL_FLUSH   0x1
>   #define TLB_NEED_GLOBAL_FLUSH  0x2
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 7235a4befe..21843c69f6 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -917,10 +917,12 @@ static void mmubooke_dump_mmu(CPUPPCState *env)
>       ppcemb_tlb_t *entry;
>       int i;
>   
> +#ifdef CONFIG_KVM
>       if (kvm_enabled() && !env->kvm_sw_tlb) {
>           qemu_printf("Cannot access KVM TLB\n");
>           return;
>       }
> +#endif
>   
>       qemu_printf("\nTLB:\n");
>       qemu_printf("Effective          Physical           Size PID   Prot     "
> @@ -1008,10 +1010,12 @@ static void mmubooke206_dump_mmu(CPUPPCState *env)
>       int offset = 0;
>       int i;
>   
> +#ifdef CONFIG_KVM
>       if (kvm_enabled() && !env->kvm_sw_tlb) {
>           qemu_printf("Cannot access KVM TLB\n");
>           return;
>       }
> +#endif
>   
>       for (i = 0; i < BOOKE206_MAX_TLBN; i++) {
>           int size = booke206_tlb_size(env, i);

[RFC PATCH 09/10] target/riscv: Restrict KVM-specific fields from ArchCPU

[Philippe Mathieu-Daudé]

These fields shouldn't be accessed when KVM is not available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: The migration part is likely invalid...

kvmtimer_needed() is defined in target/riscv/machine.c as

  static bool kvmtimer_needed(void *opaque)
  {
      return kvm_enabled();
  }

which depends on a host feature.
---
 target/riscv/cpu.h     | 2 ++
 target/riscv/machine.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..82939235ab 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -377,12 +377,14 @@ struct CPUArchState {
     hwaddr kernel_addr;
     hwaddr fdt_addr;
 
+#ifdef CONFIG_KVM
     /* kvm timer */
     bool kvm_timer_dirty;
     uint64_t kvm_timer_time;
     uint64_t kvm_timer_compare;
     uint64_t kvm_timer_state;
     uint64_t kvm_timer_frequency;
+#endif /* CONFIG_KVM */
 };
 
 OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 9c455931d8..e45d564ec3 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -201,10 +201,12 @@ static bool kvmtimer_needed(void *opaque)
 
 static int cpu_post_load(void *opaque, int version_id)
 {
+#ifdef CONFIG_KVM
     RISCVCPU *cpu = opaque;
     CPURISCVState *env = &cpu->env;
 
     env->kvm_timer_dirty = true;
+#endif
     return 0;
 }
 
@@ -215,9 +217,11 @@ static const VMStateDescription vmstate_kvmtimer = {
     .needed = kvmtimer_needed,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
+#ifdef CONFIG_KVM
         VMSTATE_UINT64(env.kvm_timer_time, RISCVCPU),
         VMSTATE_UINT64(env.kvm_timer_compare, RISCVCPU),
         VMSTATE_UINT64(env.kvm_timer_state, RISCVCPU),
+#endif
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.38.1

Re: [RFC PATCH 09/10] target/riscv: Restrict KVM-specific fields from ArchCPU

[Daniel Henrique Barboza]

On 4/5/23 13:04, Philippe Mathieu-Daudé wrote:
> These fields shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: The migration part is likely invalid...
> 
> kvmtimer_needed() is defined in target/riscv/machine.c as
> 
>    static bool kvmtimer_needed(void *opaque)
>    {
>        return kvm_enabled();
>    }
> 
> which depends on a host feature.


kvm_enabled() can be false even when CONFIG_KVM is true when a KVM capable host
is running a TCG guest, for example. In that case env->kvm_timer_* states exist
but aren't initialized, and shouldn't be migrated.

Thus it's not just a host feature, but a host feature + accel option. I think
this is fine.

> ---
>   target/riscv/cpu.h     | 2 ++
>   target/riscv/machine.c | 4 ++++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..82939235ab 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -377,12 +377,14 @@ struct CPUArchState {
>       hwaddr kernel_addr;
>       hwaddr fdt_addr;
>   
> +#ifdef CONFIG_KVM
>       /* kvm timer */
>       bool kvm_timer_dirty;
>       uint64_t kvm_timer_time;
>       uint64_t kvm_timer_compare;
>       uint64_t kvm_timer_state;
>       uint64_t kvm_timer_frequency;
> +#endif /* CONFIG_KVM */
>   };
>   
>   OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 9c455931d8..e45d564ec3 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -201,10 +201,12 @@ static bool kvmtimer_needed(void *opaque)
>   
>   static int cpu_post_load(void *opaque, int version_id)
>   {
> +#ifdef CONFIG_KVM
>       RISCVCPU *cpu = opaque;
>       CPURISCVState *env = &cpu->env;
>   
>       env->kvm_timer_dirty = true;
> +#endif
>       return 0;
>   }
>   
> @@ -215,9 +217,11 @@ static const VMStateDescription vmstate_kvmtimer = {
>       .needed = kvmtimer_needed,
>       .post_load = cpu_post_load,
>       .fields = (VMStateField[]) {
> +#ifdef CONFIG_KVM
>           VMSTATE_UINT64(env.kvm_timer_time, RISCVCPU),
>           VMSTATE_UINT64(env.kvm_timer_compare, RISCVCPU),
>           VMSTATE_UINT64(env.kvm_timer_state, RISCVCPU),
> +#endif

Here you're creating an empty 'cpu/kvmtimer' vmstate that won't be migrated anyway
because kvmtimer_needed (== kvm_enabled()) will be always false if CONFIG_KVM=n.

I'd say it's better to just get rid of the whole vmstate in this case, but I don't
like the precedence of having vmstates being gated by build flags.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>



>           VMSTATE_END_OF_LIST()
>       }
>   };

Re: [RFC PATCH 09/10] target/riscv: Restrict KVM-specific fields from ArchCPU

[Richard Henderson]

On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> These fields shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
> RFC: The migration part is likely invalid...
> 
> kvmtimer_needed() is defined in target/riscv/machine.c as
> 
>    static bool kvmtimer_needed(void *opaque)
>    {
>        return kvm_enabled();
>    }
> 
> which depends on a host feature.
> ---
>   target/riscv/cpu.h     | 2 ++
>   target/riscv/machine.c | 4 ++++
>   2 files changed, 6 insertions(+)

Yeah, the kvm parts need to be extracted to their own subsection.


r~

Re: [RFC PATCH 09/10] target/riscv: Restrict KVM-specific fields from ArchCPU

[Richard Henderson]

On 4/7/23 21:28, Richard Henderson wrote:
> On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
>> These fields shouldn't be accessed when KVM is not available.
>>
>> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
>> ---
>> RFC: The migration part is likely invalid...
>>
>> kvmtimer_needed() is defined in target/riscv/machine.c as
>>
>>    static bool kvmtimer_needed(void *opaque)
>>    {
>>        return kvm_enabled();
>>    }
>>
>> which depends on a host feature.
>> ---
>>   target/riscv/cpu.h     | 2 ++
>>   target/riscv/machine.c | 4 ++++
>>   2 files changed, 6 insertions(+)
> 
> Yeah, the kvm parts need to be extracted to their own subsection.

Oh, but they are.  Ho hum, it's getting late.


r~

[PATCH 10/10] hw/s390x: Rename pv.c -> pv-kvm.c

[Philippe Mathieu-Daudé]

Protected Virtualization is specific to KVM.
Rename the file as 'pv-kvm.c' to make this clearer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/{pv.c => pv-kvm.c} | 0
 hw/s390x/meson.build        | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename hw/s390x/{pv.c => pv-kvm.c} (100%)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv-kvm.c
similarity index 100%
rename from hw/s390x/pv.c
rename to hw/s390x/pv-kvm.c
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f291016fee..2f43b6c473 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -22,7 +22,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
   'tod-kvm.c',
   's390-skeys-kvm.c',
   's390-stattrib-kvm.c',
-  'pv.c',
+  'pv-kvm.c',
   's390-pci-kvm.c',
 ))
 s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
-- 
2.38.1

Re: [PATCH 10/10] hw/s390x: Rename pv.c -> pv-kvm.c

[Thomas Huth]

On 05/04/2023 18.04, Philippe Mathieu-Daudé wrote:
> Protected Virtualization is specific to KVM.
> Rename the file as 'pv-kvm.c' to make this clearer.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/s390x/{pv.c => pv-kvm.c} | 0
>   hw/s390x/meson.build        | 2 +-
>   2 files changed, 1 insertion(+), 1 deletion(-)
>   rename hw/s390x/{pv.c => pv-kvm.c} (100%)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv-kvm.c
> similarity index 100%
> rename from hw/s390x/pv.c
> rename to hw/s390x/pv-kvm.c
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..2f43b6c473 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -22,7 +22,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>     'tod-kvm.c',
>     's390-skeys-kvm.c',
>     's390-stattrib-kvm.c',
> -  'pv.c',
> +  'pv-kvm.c',
>     's390-pci-kvm.c',
>   ))
>   s390x_ss.add(when: 'CONFIG_TCG', if_true: files(

Hmmm, maybe we should rather move it to target/s390x/kvm/ instead?

Janosch, what's your opinion?

  Thomas

Re: [PATCH 10/10] hw/s390x: Rename pv.c -> pv-kvm.c

[Janosch Frank]

On 4/6/23 09:50, Thomas Huth wrote:
> On 05/04/2023 18.04, Philippe Mathieu-Daudé wrote:
>> Protected Virtualization is specific to KVM.
>> Rename the file as 'pv-kvm.c' to make this clearer.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>    hw/s390x/{pv.c => pv-kvm.c} | 0
>>    hw/s390x/meson.build        | 2 +-
>>    2 files changed, 1 insertion(+), 1 deletion(-)
>>    rename hw/s390x/{pv.c => pv-kvm.c} (100%)
>>
>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv-kvm.c
>> similarity index 100%
>> rename from hw/s390x/pv.c
>> rename to hw/s390x/pv-kvm.c
>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>> index f291016fee..2f43b6c473 100644
>> --- a/hw/s390x/meson.build
>> +++ b/hw/s390x/meson.build
>> @@ -22,7 +22,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>      'tod-kvm.c',
>>      's390-skeys-kvm.c',
>>      's390-stattrib-kvm.c',
>> -  'pv.c',
>> +  'pv-kvm.c',
>>      's390-pci-kvm.c',
>>    ))
>>    s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
> 
> Hmmm, maybe we should rather move it to target/s390x/kvm/ instead?
> 
> Janosch, what's your opinion?
> 
>    Thomas
> 
> 

Don't care as long as the file is not deleted :)

Re: [PATCH 10/10] hw/s390x: Rename pv.c -> pv-kvm.c

[Philippe Mathieu-Daudé]

On 6/4/23 10:04, Janosch Frank wrote:
> On 4/6/23 09:50, Thomas Huth wrote:
>> On 05/04/2023 18.04, Philippe Mathieu-Daudé wrote:
>>> Protected Virtualization is specific to KVM.
>>> Rename the file as 'pv-kvm.c' to make this clearer.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/s390x/{pv.c => pv-kvm.c} | 0
>>>    hw/s390x/meson.build        | 2 +-
>>>    2 files changed, 1 insertion(+), 1 deletion(-)
>>>    rename hw/s390x/{pv.c => pv-kvm.c} (100%)
>>>
>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv-kvm.c
>>> similarity index 100%
>>> rename from hw/s390x/pv.c
>>> rename to hw/s390x/pv-kvm.c
>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>> index f291016fee..2f43b6c473 100644
>>> --- a/hw/s390x/meson.build
>>> +++ b/hw/s390x/meson.build
>>> @@ -22,7 +22,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>      'tod-kvm.c',
>>>      's390-skeys-kvm.c',
>>>      's390-stattrib-kvm.c',
>>> -  'pv.c',
>>> +  'pv-kvm.c',
>>>      's390-pci-kvm.c',
>>>    ))
>>>    s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>
>> Hmmm, maybe we should rather move it to target/s390x/kvm/ instead?
>>
>> Janosch, what's your opinion?
>>
>>    Thomas
>>
>>
> 
> Don't care as long as the file is not deleted :)

I followed the current pattern:

$ ls -1 hw/s390x/*kvm*
hw/s390x/s390-pci-kvm.c
hw/s390x/s390-skeys-kvm.c
hw/s390x/s390-stattrib-kvm.c
hw/s390x/tod-kvm.c

I'm still unsure where is the best place to put hw files which are
arch (and accel) specific.

Re: [PATCH 10/10] hw/s390x: Rename pv.c -> pv-kvm.c

[Thomas Huth]

On 06/04/2023 10.22, Philippe Mathieu-Daudé wrote:
> On 6/4/23 10:04, Janosch Frank wrote:
>> On 4/6/23 09:50, Thomas Huth wrote:
>>> On 05/04/2023 18.04, Philippe Mathieu-Daudé wrote:
>>>> Protected Virtualization is specific to KVM.
>>>> Rename the file as 'pv-kvm.c' to make this clearer.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    hw/s390x/{pv.c => pv-kvm.c} | 0
>>>>    hw/s390x/meson.build        | 2 +-
>>>>    2 files changed, 1 insertion(+), 1 deletion(-)
>>>>    rename hw/s390x/{pv.c => pv-kvm.c} (100%)
>>>>
>>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv-kvm.c
>>>> similarity index 100%
>>>> rename from hw/s390x/pv.c
>>>> rename to hw/s390x/pv-kvm.c
>>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>>> index f291016fee..2f43b6c473 100644
>>>> --- a/hw/s390x/meson.build
>>>> +++ b/hw/s390x/meson.build
>>>> @@ -22,7 +22,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>>      'tod-kvm.c',
>>>>      's390-skeys-kvm.c',
>>>>      's390-stattrib-kvm.c',
>>>> -  'pv.c',
>>>> +  'pv-kvm.c',
>>>>      's390-pci-kvm.c',
>>>>    ))
>>>>    s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>
>>> Hmmm, maybe we should rather move it to target/s390x/kvm/ instead?
>>>
>>> Janosch, what's your opinion?
>>>
>>>    Thomas
>>>
>>>
>>
>> Don't care as long as the file is not deleted :)
> 
> I followed the current pattern:
> 
> $ ls -1 hw/s390x/*kvm*
> hw/s390x/s390-pci-kvm.c
> hw/s390x/s390-skeys-kvm.c
> hw/s390x/s390-stattrib-kvm.c
> hw/s390x/tod-kvm.c

There's a differences for those: First, these devices have an implementation 
that works with TCG, too. Second, protected virtualization (pv) is not a 
real hardware device, it's a feature of the firmware on s390x that is 
exposed to userspace via the KVM interface. So target/s390x/kvm/ slightly 
sounds like a better place to me ... no strong opinion, though.

  Thomas