[PATCH v2 0/6] hw/xen: Housekeeping

[PATCH v2 0/6] hw/xen: Housekeeping

[Philippe Mathieu-Daudé]

Hard to make an exciting cover of this series.

Basically:
- Make better separation between Xen accel and Xen hardware,
- Move stuff around to restrict PCMachineState to hw/i386/.

Since v1:
- added missing include in stubs/xen-hw-stub.c
- added missing 'exec/cpu-common.h' for ram_addr_t

(Due to a bug in 'make', while rebasing the archives
 might not be updated... so I missed this missing
 hunk as hw/core/libhwcore.fa didn't rebuilt stubs/)

Philippe Mathieu-Daudé (6):
  hw/i386/q35: Remove unreachable Xen code on Q35 machine
  hw/i386/xen: Rename X86/PC specific function as xen_hvm_init_pc()
  sysemu/xen: Add missing 'exec/cpu-common.h' header for ram_addr_t type
  stubs: Split accelerator / hardware related stubs
  hw/xen: Split x86-specific declaration from generic hardware ones
  typedefs: Restrict PCMachineState to 'hw/i386/pc.h'

 include/hw/i386/pc.h     |  4 ++--
 include/hw/xen/xen-x86.h | 15 ++++++++++++
 include/hw/xen/xen.h     |  2 --
 include/qemu/typedefs.h  |  1 -
 include/sysemu/xen.h     |  2 ++
 accel/stubs/xen-stub.c   | 41 +-------------------------------
 hw/i386/pc_piix.c        |  8 +++----
 hw/i386/pc_q35.c         | 13 ++---------
 hw/i386/xen/xen-hvm.c    |  3 ++-
 stubs/xen-hw-stub.c      | 50 ++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS              |  1 +
 stubs/meson.build        |  1 +
 12 files changed, 80 insertions(+), 61 deletions(-)
 create mode 100644 include/hw/xen/xen-x86.h
 create mode 100644 stubs/xen-hw-stub.c

-- 
2.26.2

[PATCH v2 1/6] hw/i386/q35: Remove unreachable Xen code on Q35 machine

[Philippe Mathieu-Daudé]

Xen accelerator requires specific changes to a machine to be able
to use it. See for example the 'Xen PC' machine configure its PCI
bus calling pc_xen_hvm_init_pci(). There is no 'Xen Q35' machine
declared. This code was probably added while introducing the Q35
machine, based on the existing PC machine (see commit df2d8b3ed4
"Introduce q35 pc based chipset emulator"). Remove the unreachable
code to simplify this file.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/i386/pc_q35.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0cb9c18cd44..f3506269fe0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -34,9 +34,7 @@
 #include "sysemu/arch_init.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/xen/xen.h"
 #include "sysemu/kvm.h"
-#include "sysemu/xen.h"
 #include "hw/kvm/clock.h"
 #include "hw/pci-host/q35.h"
 #include "hw/qdev-properties.h"
@@ -179,10 +177,6 @@ static void pc_q35_init(MachineState *machine)
         x86ms->below_4g_mem_size = machine->ram_size;
     }
 
-    if (xen_enabled()) {
-        xen_hvm_init(pcms, &ram_memory);
-    }
-
     x86_cpus_init(x86ms, pcmc->default_cpu_version);
 
     kvmclock_create();
@@ -208,10 +202,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* allocate ram and load rom/bios */
-    if (!xen_enabled()) {
-        pc_memory_init(pcms, get_system_memory(),
-                       rom_memory, &ram_memory);
-    }
+    pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory);
 
     /* create pci host bus */
     q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
@@ -271,7 +262,7 @@ static void pc_q35_init(MachineState *machine)
 
     assert(pcms->vmport != ON_OFF_AUTO__MAX);
     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
-        pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
+        pcms->vmport = ON_OFF_AUTO_ON;
     }
 
     /* init basic PC hardware */
-- 
2.26.2

[PATCH v2 2/6] hw/i386/xen: Rename X86/PC specific function as xen_hvm_init_pc()

[Philippe Mathieu-Daudé]

xen_hvm_init() is only meanful to initialize a X86/PC machine,
rename it as xen_hvm_init_pc().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/xen/xen.h   | 2 +-
 accel/stubs/xen-stub.c | 2 +-
 hw/i386/pc_piix.c      | 6 +++---
 hw/i386/xen/xen-hvm.c  | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 771dd447f2b..b2b459964cb 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -30,7 +30,7 @@ qemu_irq *xen_interrupt_controller_init(void);
 
 void xenstore_store_pv_console_info(int i, struct Chardev *chr);
 
-void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory);
+void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory);
 
 void xen_register_framebuffer(struct MemoryRegion *mr);
 
diff --git a/accel/stubs/xen-stub.c b/accel/stubs/xen-stub.c
index 8ae658acff5..fa3dddbce57 100644
--- a/accel/stubs/xen-stub.c
+++ b/accel/stubs/xen-stub.c
@@ -46,7 +46,7 @@ void xen_register_framebuffer(MemoryRegion *mr)
 {
 }
 
-void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
+void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
 {
 }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 32b1453e6a8..3e008bb0b1a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -117,8 +117,8 @@ static void pc_init1(MachineState *machine,
      *    so legacy non-PAE guests can get as much memory as possible in
      *    the 32bit address space below 4G.
      *
-     *  - Note that Xen has its own ram setp code in xen_ram_init(),
-     *    called via xen_hvm_init().
+     *  - Note that Xen has its own ram setup code in xen_ram_init(),
+     *    called via xen_hvm_init_pc().
      *
      * Examples:
      *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  512M high
@@ -127,7 +127,7 @@ static void pc_init1(MachineState *machine,
      *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
      */
     if (xen_enabled()) {
-        xen_hvm_init(pcms, &ram_memory);
+        xen_hvm_init_pc(pcms, &ram_memory);
     } else {
         if (!pcms->max_ram_below_4g) {
             pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index cde981bad66..49748cda3fb 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1395,7 +1395,7 @@ static int xen_map_ioreq_server(XenIOState *state)
     return 0;
 }
 
-void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
+void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
 {
     MachineState *ms = MACHINE(pcms);
     unsigned int max_cpus = ms->smp.max_cpus;
-- 
2.26.2

[PATCH v2 3/6] sysemu/xen: Add missing 'exec/cpu-common.h' header for ram_addr_t type

[Philippe Mathieu-Daudé]

As this header use the ram_addr_t type, it has to include
"exec/cpu-common.h" to avoid odd errors such:

  include/sysemu/xen.h:35:44: error: unknown type name 'ram_addr_t'; did you mean 'in_addr_t'?
   35 | static inline void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
      |                                            ^~~~~~~~~~
      |                                            in_addr_t

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/sysemu/xen.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index 2c2c429ea8b..0ca25697e4f 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -8,6 +8,8 @@
 #ifndef SYSEMU_XEN_H
 #define SYSEMU_XEN_H
 
+#include "exec/cpu-common.h"
+
 #ifdef NEED_CPU_H
 # ifdef CONFIG_XEN
 #  define CONFIG_XEN_IS_POSSIBLE
-- 
2.26.2

[PATCH v2 4/6] stubs: Split accelerator / hardware related stubs

[Philippe Mathieu-Daudé]

Move hardware stubs unrelated from the accelerator to xen-hw-stub.c.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 accel/stubs/xen-stub.c | 41 +----------------------------------
 stubs/xen-hw-stub.c    | 49 ++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS            |  1 +
 stubs/meson.build      |  1 +
 4 files changed, 52 insertions(+), 40 deletions(-)
 create mode 100644 stubs/xen-hw-stub.c

diff --git a/accel/stubs/xen-stub.c b/accel/stubs/xen-stub.c
index fa3dddbce57..7ba0b697f49 100644
--- a/accel/stubs/xen-stub.c
+++ b/accel/stubs/xen-stub.c
@@ -6,50 +6,11 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/xen/xen.h"
+#include "sysemu/xen.h"
 #include "qapi/qapi-commands-misc.h"
 
 bool xen_allowed;
 
-void xenstore_store_pv_console_info(int i, Chardev *chr)
-{
-}
-
-int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
-{
-    return -1;
-}
-
-void xen_piix3_set_irq(void *opaque, int irq_num, int level)
-{
-}
-
-void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
-{
-}
-
-void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
-{
-}
-
-int xen_is_pirq_msi(uint32_t msi_data)
-{
-    return 0;
-}
-
-qemu_irq *xen_interrupt_controller_init(void)
-{
-    return NULL;
-}
-
-void xen_register_framebuffer(MemoryRegion *mr)
-{
-}
-
-void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
-{
-}
-
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
diff --git a/stubs/xen-hw-stub.c b/stubs/xen-hw-stub.c
new file mode 100644
index 00000000000..d14efef49e9
--- /dev/null
+++ b/stubs/xen-hw-stub.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2014       Citrix Systems UK Ltd.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/xen/xen.h"
+
+void xenstore_store_pv_console_info(int i, Chardev *chr)
+{
+}
+
+int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
+{
+    return -1;
+}
+
+void xen_piix3_set_irq(void *opaque, int irq_num, int level)
+{
+}
+
+void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
+{
+}
+
+void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
+{
+}
+
+int xen_is_pirq_msi(uint32_t msi_data)
+{
+    return 0;
+}
+
+qemu_irq *xen_interrupt_controller_init(void)
+{
+    return NULL;
+}
+
+void xen_register_framebuffer(MemoryRegion *mr)
+{
+}
+
+void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
+{
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 7d0a5e91e4f..b3366713903 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -480,6 +480,7 @@ F: include/hw/block/dataplane/xen*
 F: include/hw/xen/
 F: include/sysemu/xen.h
 F: include/sysemu/xen-mapcache.h
+F: stubs/xen-hw-stub.c
 
 Guest CPU Cores (HAXM)
 ---------------------
diff --git a/stubs/meson.build b/stubs/meson.build
index e0b322bc282..c3e318a57f3 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -46,4 +46,5 @@ stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
 if have_system
   stub_ss.add(files('semihost.c'))
+  stub_ss.add(when: 'CONFIG_XEN', if_false: files('xen-hw-stub.c'))
 endif
-- 
2.26.2

Re: [PATCH v2 4/6] stubs: Split accelerator / hardware related stubs

[Philippe Mathieu-Daudé]

On 9/8/20 5:55 PM, Philippe Mathieu-Daudé wrote:
> Move hardware stubs unrelated from the accelerator to xen-hw-stub.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
...
>  Guest CPU Cores (HAXM)
>  ---------------------
> diff --git a/stubs/meson.build b/stubs/meson.build
> index e0b322bc282..c3e318a57f3 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -46,4 +46,5 @@ stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
>  if have_system
>    stub_ss.add(files('semihost.c'))
> +  stub_ss.add(when: 'CONFIG_XEN', if_false: files('xen-hw-stub.c'))

Bah wrong place :/ not my day.

>  endif
> 

[PATCH v2 5/6] hw/xen: Split x86-specific declaration from generic hardware ones

[Philippe Mathieu-Daudé]

xen_hvm_init() is restricted to the X86 architecture.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/xen/xen-x86.h | 15 +++++++++++++++
 include/hw/xen/xen.h     |  2 --
 hw/i386/pc_piix.c        |  2 +-
 hw/i386/xen/xen-hvm.c    |  1 +
 stubs/xen-hw-stub.c      |  1 +
 5 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 include/hw/xen/xen-x86.h

diff --git a/include/hw/xen/xen-x86.h b/include/hw/xen/xen-x86.h
new file mode 100644
index 00000000000..85e3db1b8de
--- /dev/null
+++ b/include/hw/xen/xen-x86.h
@@ -0,0 +1,15 @@
+/*
+ * Xen X86-specific
+ *
+ * Copyright 2020 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef QEMU_HW_XEN_X86_H
+#define QEMU_HW_XEN_X86_H
+
+#include "hw/i386/pc.h"
+
+void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory);
+
+#endif /* QEMU_HW_XEN_X86_H */
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index b2b459964cb..1406648ca58 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -30,8 +30,6 @@ qemu_irq *xen_interrupt_controller_init(void);
 
 void xenstore_store_pv_console_info(int i, struct Chardev *chr);
 
-void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory);
-
 void xen_register_framebuffer(struct MemoryRegion *mr);
 
 #endif /* QEMU_HW_XEN_H */
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3e008bb0b1a..43205f48188 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -46,7 +46,7 @@
 #include "hw/sysbus.h"
 #include "sysemu/arch_init.h"
 #include "hw/i2c/smbus_eeprom.h"
-#include "hw/xen/xen.h"
+#include "hw/xen/xen-x86.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 #include "hw/acpi/acpi.h"
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 49748cda3fb..e03c59e53da 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -22,6 +22,7 @@
 #include "hw/xen/xen_common.h"
 #include "hw/xen/xen-legacy-backend.h"
 #include "hw/xen/xen-bus.h"
+#include "hw/xen/xen-x86.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qemu/error-report.h"
diff --git a/stubs/xen-hw-stub.c b/stubs/xen-hw-stub.c
index d14efef49e9..2ea81909217 100644
--- a/stubs/xen-hw-stub.c
+++ b/stubs/xen-hw-stub.c
@@ -8,6 +8,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/xen/xen.h"
+#include "hw/xen/xen-x86.h"
 
 void xenstore_store_pv_console_info(int i, Chardev *chr)
 {
-- 
2.26.2

[PATCH v2 6/6] typedefs: Restrict PCMachineState to 'hw/i386/pc.h'

[Philippe Mathieu-Daudé]

The PCMachineState type is only used under hw/i386/.
We don't need to forward-declare it for all architectures,
restrict it to the X86 one.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/pc.h    | 4 ++--
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fe52e165b27..d4ebefcf2b4 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -19,7 +19,7 @@
  * @boot_cpus: number of present VCPUs
  * @smp_dies: number of dies per one package
  */
-struct PCMachineState {
+typedef struct PCMachineState {
     /*< private >*/
     X86MachineState parent_obj;
 
@@ -50,7 +50,7 @@ struct PCMachineState {
 
     /* ACPI Memory hotplug IO base address */
     hwaddr memhp_io_base;
-};
+} PCMachineState;
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
 #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 427027a9707..6281eae3b55 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -90,7 +90,6 @@ typedef struct PCIExpressDevice PCIExpressDevice;
 typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
 typedef struct PCIHostState PCIHostState;
-typedef struct PCMachineState PCMachineState;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
 typedef struct PropertyInfo PropertyInfo;
-- 
2.26.2

Re: [PATCH v2 0/6] hw/xen: Housekeeping

[Paolo Bonzini]

On 08/09/20 17:55, Philippe Mathieu-Daudé wrote:
> Hard to make an exciting cover of this series.
> 
> Basically:
> - Make better separation between Xen accel and Xen hardware,
> - Move stuff around to restrict PCMachineState to hw/i386/.
> 
> Since v1:
> - added missing include in stubs/xen-hw-stub.c
> - added missing 'exec/cpu-common.h' for ram_addr_t
> 
> (Due to a bug in 'make', while rebasing the archives
>  might not be updated... so I missed this missing
>  hunk as hw/core/libhwcore.fa didn't rebuilt stubs/)
> 
> Philippe Mathieu-Daudé (6):
>   hw/i386/q35: Remove unreachable Xen code on Q35 machine
>   hw/i386/xen: Rename X86/PC specific function as xen_hvm_init_pc()
>   sysemu/xen: Add missing 'exec/cpu-common.h' header for ram_addr_t type
>   stubs: Split accelerator / hardware related stubs
>   hw/xen: Split x86-specific declaration from generic hardware ones
>   typedefs: Restrict PCMachineState to 'hw/i386/pc.h'
> 
>  include/hw/i386/pc.h     |  4 ++--
>  include/hw/xen/xen-x86.h | 15 ++++++++++++
>  include/hw/xen/xen.h     |  2 --
>  include/qemu/typedefs.h  |  1 -
>  include/sysemu/xen.h     |  2 ++
>  accel/stubs/xen-stub.c   | 41 +-------------------------------
>  hw/i386/pc_piix.c        |  8 +++----
>  hw/i386/pc_q35.c         | 13 ++---------
>  hw/i386/xen/xen-hvm.c    |  3 ++-
>  stubs/xen-hw-stub.c      | 50 ++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS              |  1 +
>  stubs/meson.build        |  1 +
>  12 files changed, 80 insertions(+), 61 deletions(-)
>  create mode 100644 include/hw/xen/xen-x86.h
>  create mode 100644 stubs/xen-hw-stub.c
> 

Queued, thanks.

Paolo