All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 00/12] Introduce xenpv machine for arm architecture
@ 2022-10-15  5:07 Vikram Garhwal
  2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
                   ` (13 more replies)
  0 siblings, 14 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: vikram.garhwal, stefano.stabellini

Hi,
This series add xenpv machine for aarch64. Motivation behind creating xenpv
machine with IOREQ and TPM was to enable each guest on Xen aarch64 to have it's
own unique and emulated TPM.

This series does following:
    1. Moved common xen functionalities from hw/i386/xen to hw/xen/ so those can
       be used for aarch64.
    2. We added a minimal xenpv arm machine which creates an IOREQ server and
       support TPM.

Please note that patch 05/12 breaks the build. Patch 06/12 fixes the build
issue. If needed we can merge patch 05/12 and 06/12. For now we kept these
separate to make changes easy to review.

Also, checkpatch.pl fails for 03/12 and 06/12. These fails are due to
moving old code to new place which was not QEMU code style compatible.
No new add code was added.

Regards,
Vikram

Stefano Stabellini (5):
  hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory
  include/hw/xen/xen_common: return error from xen_create_ioreq_server
  hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration
    failure
  meson.build: do not set have_xen_pci_passthrough for aarch64 targets

Vikram Garhwal (7):
  hw/xen: Correct build config for xen_pt_stub
  hw/i386/xen/: move xen-mapcache.c to hw/xen/
  hw/i386/xen: rearrange xen_hvm_init_pc
  xen-hvm: move common functions to hw/xen/xen-hvm-common.c
  accel/xen/xen-all: export xenstore_record_dm_state
  hw/arm: introduce xenpv machine
  meson.build: enable xenpv machine build for ARM

 accel/xen/xen-all.c              |    2 +-
 hw/arm/meson.build               |    2 +
 hw/arm/xen_arm.c                 |  163 +++++
 hw/i386/meson.build              |    1 +
 hw/i386/xen/meson.build          |    1 -
 hw/i386/xen/trace-events         |   19 -
 hw/i386/xen/xen-hvm.c            | 1084 +++---------------------------
 hw/xen/meson.build               |    9 +-
 hw/xen/trace-events              |   19 +
 hw/xen/xen-hvm-common.c          |  863 ++++++++++++++++++++++++
 hw/{i386 => }/xen/xen-mapcache.c |    0
 include/hw/arm/xen_arch_hvm.h    |   12 +
 include/hw/i386/xen_arch_hvm.h   |   11 +
 include/hw/xen/arch_hvm.h        |    5 +
 include/hw/xen/xen-hvm-common.h  |   97 +++
 include/hw/xen/xen.h             |    2 +
 include/hw/xen/xen_common.h      |   12 +-
 meson.build                      |    4 +-
 18 files changed, 1287 insertions(+), 1019 deletions(-)
 create mode 100644 hw/arm/xen_arm.c
 create mode 100644 hw/xen/xen-hvm-common.c
 rename hw/{i386 => }/xen/xen-mapcache.c (100%)
 create mode 100644 include/hw/arm/xen_arch_hvm.h
 create mode 100644 include/hw/i386/xen_arch_hvm.h
 create mode 100644 include/hw/xen/arch_hvm.h
 create mode 100644 include/hw/xen/xen-hvm-common.h

-- 
2.17.0



^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 14:46   ` Paul Durrant
  2022-10-26 19:01   ` Alex Bennée
  2022-10-15  5:07 ` [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/ Vikram Garhwal
                   ` (12 subsequent siblings)
  13 siblings, 2 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:X86 Xen CPUs

Build fails when have_xen_pci_passthrough is disabled. This is because of
incorrect build configuration for xen_pt_stub.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 hw/xen/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xen/meson.build b/hw/xen/meson.build
index 08dc1f6857..ae0ace3046 100644
--- a/hw/xen/meson.build
+++ b/hw/xen/meson.build
@@ -18,7 +18,7 @@ if have_xen_pci_passthrough
     'xen_pt_msi.c',
   ))
 else
-  xen_specific_ss.add('xen_pt_stub.c')
+  xen_specific_ss.add(files('xen_pt_stub.c'))
 endif
 
 specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
  2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 14:53   ` Paul Durrant
  2022-10-15  5:07 ` [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc Vikram Garhwal
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, Stefano Stabellini, Anthony Perard,
	Paul Durrant, open list:X86 Xen CPUs

xen-mapcache.c contains common functions which can be used for enabling Xen on
aarch64 with IOREQ handling. Moving it out from hw/i386/xen to hw/xen to make it
accessible for both aarch64 and x86.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/i386/meson.build              | 1 +
 hw/i386/xen/meson.build          | 1 -
 hw/i386/xen/trace-events         | 5 -----
 hw/xen/meson.build               | 4 ++++
 hw/xen/trace-events              | 5 +++++
 hw/{i386 => }/xen/xen-mapcache.c | 0
 6 files changed, 10 insertions(+), 6 deletions(-)
 rename hw/{i386 => }/xen/xen-mapcache.c (100%)

diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index 213e2e82b3..cfdbfdcbcb 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -33,5 +33,6 @@ subdir('kvm')
 subdir('xen')
 
 i386_ss.add_all(xenpv_ss)
+i386_ss.add_all(xen_ss)
 
 hw_arch += {'i386': i386_ss}
diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
index be84130300..2fcc46e6ca 100644
--- a/hw/i386/xen/meson.build
+++ b/hw/i386/xen/meson.build
@@ -1,6 +1,5 @@
 i386_ss.add(when: 'CONFIG_XEN', if_true: files(
   'xen-hvm.c',
-  'xen-mapcache.c',
   'xen_apic.c',
   'xen_platform.c',
   'xen_pvdevice.c',
diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
index 5d6be61090..a0c89d91c4 100644
--- a/hw/i386/xen/trace-events
+++ b/hw/i386/xen/trace-events
@@ -21,8 +21,3 @@ xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
 cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
 cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
 
-# xen-mapcache.c
-xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
-xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
-xen_map_cache_return(void* ptr) "%p"
-
diff --git a/hw/xen/meson.build b/hw/xen/meson.build
index ae0ace3046..19d0637c46 100644
--- a/hw/xen/meson.build
+++ b/hw/xen/meson.build
@@ -22,3 +22,7 @@ else
 endif
 
 specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
+
+xen_ss = ss.source_set()
+
+xen_ss.add(when: 'CONFIG_XEN', if_true: files('xen-mapcache.c'))
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 3da3fd8348..2c8f238f42 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -41,3 +41,8 @@ xs_node_vprintf(char *path, char *value) "%s %s"
 xs_node_vscanf(char *path, char *value) "%s %s"
 xs_node_watch(char *path) "%s"
 xs_node_unwatch(char *path) "%s"
+
+# xen-mapcache.c
+xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
+xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
+xen_map_cache_return(void* ptr) "%p"
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
similarity index 100%
rename from hw/i386/xen/xen-mapcache.c
rename to hw/xen/xen-mapcache.c
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
  2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
  2022-10-15  5:07 ` [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/ Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 14:59   ` Paul Durrant
  2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum,
	open list:X86 Xen CPUs

In preparation to moving most of xen-hvm code to an arch-neutral location,
move non IOREQ references to:
- xen_get_vmport_regs_pfn
- xen_suspend_notifier
- xen_wakeup_notifier
- xen_ram_init

towards the end of the xen_hvm_init_pc() function.

This is done to keep the common ioreq functions in one place which will be
moved to new function in next patch in order to make it common to both x86 and
aarch64 machines.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/i386/xen/xen-hvm.c | 49 ++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index e4293d6d66..b27484ad22 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1416,12 +1416,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
     state->exit.notify = xen_exit_notifier;
     qemu_add_exit_notifier(&state->exit);
 
-    state->suspend.notify = xen_suspend_notifier;
-    qemu_register_suspend_notifier(&state->suspend);
-
-    state->wakeup.notify = xen_wakeup_notifier;
-    qemu_register_wakeup_notifier(&state->wakeup);
-
     /*
      * Register wake-up support in QMP query-current-machine API
      */
@@ -1432,23 +1426,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    rc = xen_get_vmport_regs_pfn(xen_xc, xen_domid, &ioreq_pfn);
-    if (!rc) {
-        DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
-        state->shared_vmport_page =
-            xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
-                                 1, &ioreq_pfn, NULL);
-        if (state->shared_vmport_page == NULL) {
-            error_report("map shared vmport IO page returned error %d handle=%p",
-                         errno, xen_xc);
-            goto err;
-        }
-    } else if (rc != -ENOSYS) {
-        error_report("get vmport regs pfn returned error %d, rc=%d",
-                     errno, rc);
-        goto err;
-    }
-
     /* Note: cpus is empty at this point in init */
     state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
 
@@ -1486,7 +1463,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
 #else
     xen_map_cache_init(NULL, state);
 #endif
-    xen_ram_init(pcms, ms->ram_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
@@ -1513,6 +1489,31 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
     QLIST_INIT(&xen_physmap);
     xen_read_physmap(state);
 
+    state->suspend.notify = xen_suspend_notifier;
+    qemu_register_suspend_notifier(&state->suspend);
+
+    state->wakeup.notify = xen_wakeup_notifier;
+    qemu_register_wakeup_notifier(&state->wakeup);
+
+    rc = xen_get_vmport_regs_pfn(xen_xc, xen_domid, &ioreq_pfn);
+    if (!rc) {
+        DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
+        state->shared_vmport_page =
+            xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
+                                 1, &ioreq_pfn, NULL);
+        if (state->shared_vmport_page == NULL) {
+            error_report("map shared vmport IO page returned error %d handle=%p",
+                         errno, xen_xc);
+            goto err;
+        }
+    } else if (rc != -ENOSYS) {
+        error_report("get vmport regs pfn returned error %d, rc=%d",
+                     errno, rc);
+        goto err;
+    }
+
+    xen_ram_init(pcms, ms->ram_size, ram_memory);
+
     /* Disable ACPI build because Xen handles it */
     pcms->acpi_build_enabled = false;
 
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (2 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 15:56   ` Paul Durrant
                     ` (2 more replies)
  2022-10-15  5:07 ` [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory Vikram Garhwal
                   ` (9 subsequent siblings)
  13 siblings, 3 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, open list:X86 Xen CPUs

From: Stefano Stabellini <stefano.stabellini@amd.com>

In preparation to moving most of xen-hvm code to an arch-neutral location, move:
- shared_vmport_page
- log_for_dirtybit
- dirty_bitmap
- suspend
- wakeup

out of XenIOState struct as these are only used on x86, especially the ones
related to dirty logging.
Updated XenIOState can be used for both aarch64 and x86.

Also, remove free_phys_offset as it was unused.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 hw/i386/xen/xen-hvm.c | 58 ++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index b27484ad22..e169de16c4 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -73,6 +73,7 @@ struct shared_vmport_iopage {
 };
 typedef struct shared_vmport_iopage shared_vmport_iopage_t;
 #endif
+static shared_vmport_iopage_t *shared_vmport_page;
 
 static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
 {
@@ -95,6 +96,11 @@ typedef struct XenPhysmap {
 } XenPhysmap;
 
 static QLIST_HEAD(, XenPhysmap) xen_physmap;
+static const XenPhysmap *log_for_dirtybit;
+/* Buffer used by xen_sync_dirty_bitmap */
+static unsigned long *dirty_bitmap;
+static Notifier suspend;
+static Notifier wakeup;
 
 typedef struct XenPciDevice {
     PCIDevice *pci_dev;
@@ -105,7 +111,6 @@ typedef struct XenPciDevice {
 typedef struct XenIOState {
     ioservid_t ioservid;
     shared_iopage_t *shared_page;
-    shared_vmport_iopage_t *shared_vmport_page;
     buffered_iopage_t *buffered_io_page;
     xenforeignmemory_resource_handle *fres;
     QEMUTimer *buffered_io_timer;
@@ -125,14 +130,8 @@ typedef struct XenIOState {
     MemoryListener io_listener;
     QLIST_HEAD(, XenPciDevice) dev_list;
     DeviceListener device_listener;
-    hwaddr free_phys_offset;
-    const XenPhysmap *log_for_dirtybit;
-    /* Buffer used by xen_sync_dirty_bitmap */
-    unsigned long *dirty_bitmap;
 
     Notifier exit;
-    Notifier suspend;
-    Notifier wakeup;
 } XenIOState;
 
 /* Xen specific function for piix pci */
@@ -462,10 +461,10 @@ static int xen_remove_from_physmap(XenIOState *state,
     }
 
     QLIST_REMOVE(physmap, list);
-    if (state->log_for_dirtybit == physmap) {
-        state->log_for_dirtybit = NULL;
-        g_free(state->dirty_bitmap);
-        state->dirty_bitmap = NULL;
+    if (log_for_dirtybit == physmap) {
+        log_for_dirtybit = NULL;
+        g_free(dirty_bitmap);
+        dirty_bitmap = NULL;
     }
     g_free(physmap);
 
@@ -626,16 +625,16 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
         return;
     }
 
-    if (state->log_for_dirtybit == NULL) {
-        state->log_for_dirtybit = physmap;
-        state->dirty_bitmap = g_new(unsigned long, bitmap_size);
-    } else if (state->log_for_dirtybit != physmap) {
+    if (log_for_dirtybit == NULL) {
+        log_for_dirtybit = physmap;
+        dirty_bitmap = g_new(unsigned long, bitmap_size);
+    } else if (log_for_dirtybit != physmap) {
         /* Only one range for dirty bitmap can be tracked. */
         return;
     }
 
     rc = xen_track_dirty_vram(xen_domid, start_addr >> TARGET_PAGE_BITS,
-                              npages, state->dirty_bitmap);
+                              npages, dirty_bitmap);
     if (rc < 0) {
 #ifndef ENODATA
 #define ENODATA  ENOENT
@@ -650,7 +649,7 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
     }
 
     for (i = 0; i < bitmap_size; i++) {
-        unsigned long map = state->dirty_bitmap[i];
+        unsigned long map = dirty_bitmap[i];
         while (map != 0) {
             j = ctzl(map);
             map &= ~(1ul << j);
@@ -676,12 +675,10 @@ static void xen_log_start(MemoryListener *listener,
 static void xen_log_stop(MemoryListener *listener, MemoryRegionSection *section,
                          int old, int new)
 {
-    XenIOState *state = container_of(listener, XenIOState, memory_listener);
-
     if (old & ~new & (1 << DIRTY_MEMORY_VGA)) {
-        state->log_for_dirtybit = NULL;
-        g_free(state->dirty_bitmap);
-        state->dirty_bitmap = NULL;
+        log_for_dirtybit = NULL;
+        g_free(dirty_bitmap);
+        dirty_bitmap = NULL;
         /* Disable dirty bit tracking */
         xen_track_dirty_vram(xen_domid, 0, 0, NULL);
     }
@@ -1021,9 +1018,9 @@ static void handle_vmport_ioreq(XenIOState *state, ioreq_t *req)
 {
     vmware_regs_t *vmport_regs;
 
-    assert(state->shared_vmport_page);
+    assert(shared_vmport_page);
     vmport_regs =
-        &state->shared_vmport_page->vcpu_vmport_regs[state->send_vcpu];
+        &shared_vmport_page->vcpu_vmport_regs[state->send_vcpu];
     QEMU_BUILD_BUG_ON(sizeof(*req) < sizeof(*vmport_regs));
 
     current_cpu = state->cpu_by_vcpu_id[state->send_vcpu];
@@ -1468,7 +1465,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     state->memory_listener = xen_memory_listener;
     memory_listener_register(&state->memory_listener, &address_space_memory);
-    state->log_for_dirtybit = NULL;
 
     state->io_listener = xen_io_listener;
     memory_listener_register(&state->io_listener, &address_space_io);
@@ -1489,19 +1485,19 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
     QLIST_INIT(&xen_physmap);
     xen_read_physmap(state);
 
-    state->suspend.notify = xen_suspend_notifier;
-    qemu_register_suspend_notifier(&state->suspend);
+    suspend.notify = xen_suspend_notifier;
+    qemu_register_suspend_notifier(&suspend);
 
-    state->wakeup.notify = xen_wakeup_notifier;
-    qemu_register_wakeup_notifier(&state->wakeup);
+    wakeup.notify = xen_wakeup_notifier;
+    qemu_register_wakeup_notifier(&wakeup);
 
     rc = xen_get_vmport_regs_pfn(xen_xc, xen_domid, &ioreq_pfn);
     if (!rc) {
         DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
-        state->shared_vmport_page =
+        shared_vmport_page =
             xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
                                  1, &ioreq_pfn, NULL);
-        if (state->shared_vmport_page == NULL) {
+        if (shared_vmport_page == NULL) {
             error_report("map shared vmport IO page returned error %d handle=%p",
                          errno, xen_xc);
             goto err;
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (3 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 16:09   ` Paul Durrant
  2022-10-27  9:02   ` Alex Bennée
  2022-10-15  5:07 ` [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c Vikram Garhwal
                   ` (8 subsequent siblings)
  13 siblings, 2 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, open list:X86 Xen CPUs

From: Stefano Stabellini <stefano.stabellini@amd.com>

In preparation to moving most of xen-hvm code to an arch-neutral location,
move the x86-specific portion of xen_set_memory to arch_xen_set_memory.

Also move handle_vmport_ioreq to arch_handle_ioreq.

NOTE: This patch breaks the build. Next patch fixes the build issue.
Reason behind creating this patch is because there is lot of new code addition
and pure code movement done for enabling Xen on ARM. Keeping the this patch
separate is done to make it easier to review.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 hw/i386/xen/xen-hvm.c          | 97 ++++++++++++++++++++--------------
 include/hw/i386/xen_arch_hvm.h | 10 ++++
 include/hw/xen/arch_hvm.h      |  3 ++
 3 files changed, 70 insertions(+), 40 deletions(-)
 create mode 100644 include/hw/i386/xen_arch_hvm.h
 create mode 100644 include/hw/xen/arch_hvm.h

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index e169de16c4..3cd1808f9d 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -34,6 +34,7 @@
 #include "trace.h"
 
 #include <xen/hvm/ioreq.h>
+#include "hw/xen/arch_hvm.h"
 #include <xen/hvm/e820.h>
 
 //#define DEBUG_XEN_HVM
@@ -476,10 +477,6 @@ static void xen_set_memory(struct MemoryListener *listener,
                            bool add)
 {
     XenIOState *state = container_of(listener, XenIOState, memory_listener);
-    hwaddr start_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
-    hvmmem_type_t mem_type;
 
     if (section->mr == &ram_memory) {
         return;
@@ -492,38 +489,7 @@ static void xen_set_memory(struct MemoryListener *listener,
                                      section);
         }
     }
-
-    if (!memory_region_is_ram(section->mr)) {
-        return;
-    }
-
-    if (log_dirty != add) {
-        return;
-    }
-
-    trace_xen_client_set_memory(start_addr, size, log_dirty);
-
-    start_addr &= TARGET_PAGE_MASK;
-    size = TARGET_PAGE_ALIGN(size);
-
-    if (add) {
-        if (!memory_region_is_rom(section->mr)) {
-            xen_add_to_physmap(state, start_addr, size,
-                               section->mr, section->offset_within_region);
-        } else {
-            mem_type = HVMMEM_ram_ro;
-            if (xen_set_mem_type(xen_domid, mem_type,
-                                 start_addr >> TARGET_PAGE_BITS,
-                                 size >> TARGET_PAGE_BITS)) {
-                DPRINTF("xen_set_mem_type error, addr: "TARGET_FMT_plx"\n",
-                        start_addr);
-            }
-        }
-    } else {
-        if (xen_remove_from_physmap(state, start_addr, size) < 0) {
-            DPRINTF("physmapping does not exist at "TARGET_FMT_plx"\n", start_addr);
-        }
-    }
+    arch_xen_set_memory(state, section, add);
 }
 
 static void xen_region_add(MemoryListener *listener,
@@ -1051,9 +1017,6 @@ static void handle_ioreq(XenIOState *state, ioreq_t *req)
         case IOREQ_TYPE_COPY:
             cpu_ioreq_move(req);
             break;
-        case IOREQ_TYPE_VMWARE_PORT:
-            handle_vmport_ioreq(state, req);
-            break;
         case IOREQ_TYPE_TIMEOFFSET:
             break;
         case IOREQ_TYPE_INVALIDATE:
@@ -1063,7 +1026,7 @@ static void handle_ioreq(XenIOState *state, ioreq_t *req)
             cpu_ioreq_config(state, req);
             break;
         default:
-            hw_error("Invalid ioreq type 0x%x\n", req->type);
+            arch_handle_ioreq(state, req);
     }
     if (req->dir == IOREQ_READ) {
         trace_handle_ioreq_read(req, req->type, req->df, req->data_is_ptr,
@@ -1604,3 +1567,57 @@ void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
         memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
     }
 }
+
+void arch_xen_set_memory(XenIOState *state, MemoryRegionSection *section,
+                                bool add)
+{
+    hwaddr start_addr = section->offset_within_address_space;
+    ram_addr_t size = int128_get64(section->size);
+    bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
+    hvmmem_type_t mem_type;
+
+    if (!memory_region_is_ram(section->mr)) {
+        return;
+    }
+
+    if (log_dirty != add) {
+        return;
+    }
+
+    trace_xen_client_set_memory(start_addr, size, log_dirty);
+
+    start_addr &= TARGET_PAGE_MASK;
+    size = TARGET_PAGE_ALIGN(size);
+
+    if (add) {
+        if (!memory_region_is_rom(section->mr)) {
+            xen_add_to_physmap(state, start_addr, size,
+                               section->mr, section->offset_within_region);
+        } else {
+            mem_type = HVMMEM_ram_ro;
+            if (xen_set_mem_type(xen_domid, mem_type,
+                                 start_addr >> TARGET_PAGE_BITS,
+                                 size >> TARGET_PAGE_BITS)) {
+                DPRINTF("xen_set_mem_type error, addr: "TARGET_FMT_plx"\n",
+                        start_addr);
+            }
+        }
+    } else {
+        if (xen_remove_from_physmap(state, start_addr, size) < 0) {
+            DPRINTF("physmapping does not exist at "TARGET_FMT_plx"\n", start_addr);
+        }
+    }
+}
+
+void arch_handle_ioreq(XenIOState *state, ioreq_t *req)
+{
+    switch (req->type) {
+    case IOREQ_TYPE_VMWARE_PORT:
+            handle_vmport_ioreq(state, req);
+        break;
+    default:
+        hw_error("Invalid ioreq type 0x%x\n", req->type);
+    }
+
+    return;
+}
diff --git a/include/hw/i386/xen_arch_hvm.h b/include/hw/i386/xen_arch_hvm.h
new file mode 100644
index 0000000000..1b2c71ba4f
--- /dev/null
+++ b/include/hw/i386/xen_arch_hvm.h
@@ -0,0 +1,10 @@
+#ifndef HW_XEN_ARCH_I386_HVM_H
+#define HW_XEN_ARCH_I386_HVM_H
+
+#include <xen/hvm/ioreq.h>
+
+void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
+void arch_xen_set_memory(XenIOState *state,
+                         MemoryRegionSection *section,
+                         bool add);
+#endif
diff --git a/include/hw/xen/arch_hvm.h b/include/hw/xen/arch_hvm.h
new file mode 100644
index 0000000000..26674648d8
--- /dev/null
+++ b/include/hw/xen/arch_hvm.h
@@ -0,0 +1,3 @@
+#if defined(TARGET_I386) || defined(TARGET_X86_64)
+#include "hw/i386/xen_arch_hvm.h"
+#endif
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (4 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-16 18:07   ` Julien Grall
  2022-10-19 16:16   ` Paul Durrant
  2022-10-15  5:07 ` [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server Vikram Garhwal
                   ` (7 subsequent siblings)
  13 siblings, 2 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, open list:X86 Xen CPUs

Extract common functionalities from hw/i386/xen/xen-hvm.c and move them to
hw/xen/xen-hvm-common.c. These common functions are useful for creating
an IOREQ server.

xen_hvm_init_pc() contains the arch independent code for creating and mapping
a IOREQ server, connecting memory and IO listeners, initializing a xen bus and
registering backends. Moved this common xen code to a new function
xen_register_ioreq() which can be used by both x86 and ARM machines.

Following functions are moved to hw/xen/xen-hvm-common.c:
    xen_vcpu_eport(), xen_vcpu_ioreq(), xen_ram_alloc(), xen_set_memory(),
    xen_region_add(), xen_region_del(), xen_io_add(), xen_io_del(),
    xen_device_realize(), xen_device_unrealize(),
    cpu_get_ioreq_from_shared_memory(), cpu_get_ioreq(), do_inp(), do_outp(),
    rw_phys_req_item(), read_phys_req_item(), write_phys_req_item(),
    cpu_ioreq_pio(), cpu_ioreq_move(), cpu_ioreq_config(), handle_ioreq(),
    handle_buffered_iopage(), handle_buffered_io(), cpu_handle_ioreq(),
    xen_main_loop_prepare(), xen_hvm_change_state_handler(),
    xen_exit_notifier(), xen_map_ioreq_server(), destroy_hvm_domain() and
    xen_shutdown_fatal_error()

Removed static type from below functions:
1. xen_region_add()
2. xen_region_del()
3. xen_io_add()
4. xen_io_del()
5. xen_device_realize()
6. xen_device_unrealize()
7. xen_hvm_change_state_handler()
8. cpu_ioreq_pio()
9. xen_exit_notifier()

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/i386/xen/trace-events        |  14 -
 hw/i386/xen/xen-hvm.c           | 930 +-------------------------------
 hw/xen/meson.build              |   5 +-
 hw/xen/trace-events             |  14 +
 hw/xen/xen-hvm-common.c         | 858 +++++++++++++++++++++++++++++
 include/hw/i386/xen_arch_hvm.h  |   1 +
 include/hw/xen/xen-hvm-common.h |  97 ++++
 7 files changed, 983 insertions(+), 936 deletions(-)
 create mode 100644 hw/xen/xen-hvm-common.c
 create mode 100644 include/hw/xen/xen-hvm-common.h

diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
index a0c89d91c4..5d0a8d6dcf 100644
--- a/hw/i386/xen/trace-events
+++ b/hw/i386/xen/trace-events
@@ -7,17 +7,3 @@ xen_platform_log(char *s) "xen platform: %s"
 xen_pv_mmio_read(uint64_t addr) "WARNING: read from Xen PV Device MMIO space (address 0x%"PRIx64")"
 xen_pv_mmio_write(uint64_t addr) "WARNING: write to Xen PV Device MMIO space (address 0x%"PRIx64")"
 
-# xen-hvm.c
-xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: 0x%lx, size 0x%lx"
-xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) "0x%"PRIx64" size 0x%lx, log_dirty %i"
-handle_ioreq(void *req, uint32_t type, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p type=%d dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-handle_ioreq_read(void *req, uint32_t type, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p read type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-handle_ioreq_write(void *req, uint32_t type, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p write type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-cpu_ioreq_pio(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p pio dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-cpu_ioreq_pio_read_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio read reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
-cpu_ioreq_pio_write_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio write reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
-cpu_ioreq_move(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p copy dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
-cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
-cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
-
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 3cd1808f9d..0406f13b35 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -10,47 +10,20 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+#include "trace.h"
 
-#include "cpu.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pci_host.h"
 #include "hw/i386/pc.h"
 #include "hw/irq.h"
-#include "hw/hw.h"
 #include "hw/i386/apic-msidef.h"
-#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-migration.h"
-#include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qemu/range.h"
-#include "sysemu/runstate.h"
-#include "sysemu/sysemu.h"
-#include "sysemu/xen.h"
-#include "sysemu/xen-mapcache.h"
-#include "trace.h"
 
-#include <xen/hvm/ioreq.h>
+#include "hw/xen/xen-hvm-common.h"
 #include "hw/xen/arch_hvm.h"
 #include <xen/hvm/e820.h>
 
-//#define DEBUG_XEN_HVM
-
-#ifdef DEBUG_XEN_HVM
-#define DPRINTF(fmt, ...) \
-    do { fprintf(stderr, "xen: " fmt, ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-    do { } while (0)
-#endif
-
-static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
-static MemoryRegion *framebuffer;
-static bool xen_in_migration;
-
 /* Compatibility with older version */
 
 /* This allows QEMU to build on a system that has Xen 4.5 or earlier
@@ -76,26 +49,9 @@ typedef struct shared_vmport_iopage shared_vmport_iopage_t;
 #endif
 static shared_vmport_iopage_t *shared_vmport_page;
 
-static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
-{
-    return shared_page->vcpu_ioreq[i].vp_eport;
-}
-static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
-{
-    return &shared_page->vcpu_ioreq[vcpu];
-}
-
-#define BUFFER_IO_MAX_DELAY  100
-
-typedef struct XenPhysmap {
-    hwaddr start_addr;
-    ram_addr_t size;
-    const char *name;
-    hwaddr phys_offset;
-
-    QLIST_ENTRY(XenPhysmap) list;
-} XenPhysmap;
-
+static MemoryRegion ram_640k, ram_lo, ram_hi;
+static MemoryRegion *framebuffer;
+static bool xen_in_migration;
 static QLIST_HEAD(, XenPhysmap) xen_physmap;
 static const XenPhysmap *log_for_dirtybit;
 /* Buffer used by xen_sync_dirty_bitmap */
@@ -103,38 +59,6 @@ static unsigned long *dirty_bitmap;
 static Notifier suspend;
 static Notifier wakeup;
 
-typedef struct XenPciDevice {
-    PCIDevice *pci_dev;
-    uint32_t sbdf;
-    QLIST_ENTRY(XenPciDevice) entry;
-} XenPciDevice;
-
-typedef struct XenIOState {
-    ioservid_t ioservid;
-    shared_iopage_t *shared_page;
-    buffered_iopage_t *buffered_io_page;
-    xenforeignmemory_resource_handle *fres;
-    QEMUTimer *buffered_io_timer;
-    CPUState **cpu_by_vcpu_id;
-    /* the evtchn port for polling the notification, */
-    evtchn_port_t *ioreq_local_port;
-    /* evtchn remote and local ports for buffered io */
-    evtchn_port_t bufioreq_remote_port;
-    evtchn_port_t bufioreq_local_port;
-    /* the evtchn fd for polling */
-    xenevtchn_handle *xce_handle;
-    /* which vcpu we are serving */
-    int send_vcpu;
-
-    struct xs_handle *xenstore;
-    MemoryListener memory_listener;
-    MemoryListener io_listener;
-    QLIST_HEAD(, XenPciDevice) dev_list;
-    DeviceListener device_listener;
-
-    Notifier exit;
-} XenIOState;
-
 /* Xen specific function for piix pci */
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
@@ -247,42 +171,6 @@ static void xen_ram_init(PCMachineState *pcms,
     }
 }
 
-void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
-                   Error **errp)
-{
-    unsigned long nr_pfn;
-    xen_pfn_t *pfn_list;
-    int i;
-
-    if (runstate_check(RUN_STATE_INMIGRATE)) {
-        /* RAM already populated in Xen */
-        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
-                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
-                __func__, size, ram_addr);
-        return;
-    }
-
-    if (mr == &ram_memory) {
-        return;
-    }
-
-    trace_xen_ram_alloc(ram_addr, size);
-
-    nr_pfn = size >> TARGET_PAGE_BITS;
-    pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
-
-    for (i = 0; i < nr_pfn; i++) {
-        pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i;
-    }
-
-    if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
-        error_setg(errp, "xen: failed to populate ram at " RAM_ADDR_FMT,
-                   ram_addr);
-    }
-
-    g_free(pfn_list);
-}
-
 static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size)
 {
     XenPhysmap *physmap = NULL;
@@ -472,109 +360,6 @@ static int xen_remove_from_physmap(XenIOState *state,
     return 0;
 }
 
-static void xen_set_memory(struct MemoryListener *listener,
-                           MemoryRegionSection *section,
-                           bool add)
-{
-    XenIOState *state = container_of(listener, XenIOState, memory_listener);
-
-    if (section->mr == &ram_memory) {
-        return;
-    } else {
-        if (add) {
-            xen_map_memory_section(xen_domid, state->ioservid,
-                                   section);
-        } else {
-            xen_unmap_memory_section(xen_domid, state->ioservid,
-                                     section);
-        }
-    }
-    arch_xen_set_memory(state, section, add);
-}
-
-static void xen_region_add(MemoryListener *listener,
-                           MemoryRegionSection *section)
-{
-    memory_region_ref(section->mr);
-    xen_set_memory(listener, section, true);
-}
-
-static void xen_region_del(MemoryListener *listener,
-                           MemoryRegionSection *section)
-{
-    xen_set_memory(listener, section, false);
-    memory_region_unref(section->mr);
-}
-
-static void xen_io_add(MemoryListener *listener,
-                       MemoryRegionSection *section)
-{
-    XenIOState *state = container_of(listener, XenIOState, io_listener);
-    MemoryRegion *mr = section->mr;
-
-    if (mr->ops == &unassigned_io_ops) {
-        return;
-    }
-
-    memory_region_ref(mr);
-
-    xen_map_io_section(xen_domid, state->ioservid, section);
-}
-
-static void xen_io_del(MemoryListener *listener,
-                       MemoryRegionSection *section)
-{
-    XenIOState *state = container_of(listener, XenIOState, io_listener);
-    MemoryRegion *mr = section->mr;
-
-    if (mr->ops == &unassigned_io_ops) {
-        return;
-    }
-
-    xen_unmap_io_section(xen_domid, state->ioservid, section);
-
-    memory_region_unref(mr);
-}
-
-static void xen_device_realize(DeviceListener *listener,
-                               DeviceState *dev)
-{
-    XenIOState *state = container_of(listener, XenIOState, device_listener);
-
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        PCIDevice *pci_dev = PCI_DEVICE(dev);
-        XenPciDevice *xendev = g_new(XenPciDevice, 1);
-
-        xendev->pci_dev = pci_dev;
-        xendev->sbdf = PCI_BUILD_BDF(pci_dev_bus_num(pci_dev),
-                                     pci_dev->devfn);
-        QLIST_INSERT_HEAD(&state->dev_list, xendev, entry);
-
-        xen_map_pcidev(xen_domid, state->ioservid, pci_dev);
-    }
-}
-
-static void xen_device_unrealize(DeviceListener *listener,
-                                 DeviceState *dev)
-{
-    XenIOState *state = container_of(listener, XenIOState, device_listener);
-
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        PCIDevice *pci_dev = PCI_DEVICE(dev);
-        XenPciDevice *xendev, *next;
-
-        xen_unmap_pcidev(xen_domid, state->ioservid, pci_dev);
-
-        QLIST_FOREACH_SAFE(xendev, &state->dev_list, entry, next) {
-            if (xendev->pci_dev == pci_dev) {
-                QLIST_REMOVE(xendev, entry);
-                g_free(xendev);
-                break;
-            }
-        }
-    }
-}
-
 static void xen_sync_dirty_bitmap(XenIOState *state,
                                   hwaddr start_addr,
                                   ram_addr_t size)
@@ -682,277 +467,6 @@ static MemoryListener xen_memory_listener = {
     .priority = 10,
 };
 
-static MemoryListener xen_io_listener = {
-    .name = "xen-io",
-    .region_add = xen_io_add,
-    .region_del = xen_io_del,
-    .priority = 10,
-};
-
-static DeviceListener xen_device_listener = {
-    .realize = xen_device_realize,
-    .unrealize = xen_device_unrealize,
-};
-
-/* get the ioreq packets from share mem */
-static ioreq_t *cpu_get_ioreq_from_shared_memory(XenIOState *state, int vcpu)
-{
-    ioreq_t *req = xen_vcpu_ioreq(state->shared_page, vcpu);
-
-    if (req->state != STATE_IOREQ_READY) {
-        DPRINTF("I/O request not ready: "
-                "%x, ptr: %x, port: %"PRIx64", "
-                "data: %"PRIx64", count: %u, size: %u\n",
-                req->state, req->data_is_ptr, req->addr,
-                req->data, req->count, req->size);
-        return NULL;
-    }
-
-    xen_rmb(); /* see IOREQ_READY /then/ read contents of ioreq */
-
-    req->state = STATE_IOREQ_INPROCESS;
-    return req;
-}
-
-/* use poll to get the port notification */
-/* ioreq_vec--out,the */
-/* retval--the number of ioreq packet */
-static ioreq_t *cpu_get_ioreq(XenIOState *state)
-{
-    MachineState *ms = MACHINE(qdev_get_machine());
-    unsigned int max_cpus = ms->smp.max_cpus;
-    int i;
-    evtchn_port_t port;
-
-    port = xenevtchn_pending(state->xce_handle);
-    if (port == state->bufioreq_local_port) {
-        timer_mod(state->buffered_io_timer,
-                BUFFER_IO_MAX_DELAY + qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
-        return NULL;
-    }
-
-    if (port != -1) {
-        for (i = 0; i < max_cpus; i++) {
-            if (state->ioreq_local_port[i] == port) {
-                break;
-            }
-        }
-
-        if (i == max_cpus) {
-            hw_error("Fatal error while trying to get io event!\n");
-        }
-
-        /* unmask the wanted port again */
-        xenevtchn_unmask(state->xce_handle, port);
-
-        /* get the io packet from shared memory */
-        state->send_vcpu = i;
-        return cpu_get_ioreq_from_shared_memory(state, i);
-    }
-
-    /* read error or read nothing */
-    return NULL;
-}
-
-static uint32_t do_inp(uint32_t addr, unsigned long size)
-{
-    switch (size) {
-        case 1:
-            return cpu_inb(addr);
-        case 2:
-            return cpu_inw(addr);
-        case 4:
-            return cpu_inl(addr);
-        default:
-            hw_error("inp: bad size: %04x %lx", addr, size);
-    }
-}
-
-static void do_outp(uint32_t addr,
-        unsigned long size, uint32_t val)
-{
-    switch (size) {
-        case 1:
-            return cpu_outb(addr, val);
-        case 2:
-            return cpu_outw(addr, val);
-        case 4:
-            return cpu_outl(addr, val);
-        default:
-            hw_error("outp: bad size: %04x %lx", addr, size);
-    }
-}
-
-/*
- * Helper functions which read/write an object from/to physical guest
- * memory, as part of the implementation of an ioreq.
- *
- * Equivalent to
- *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
- *                          val, req->size, 0/1)
- * except without the integer overflow problems.
- */
-static void rw_phys_req_item(hwaddr addr,
-                             ioreq_t *req, uint32_t i, void *val, int rw)
-{
-    /* Do everything unsigned so overflow just results in a truncated result
-     * and accesses to undesired parts of guest memory, which is up
-     * to the guest */
-    hwaddr offset = (hwaddr)req->size * i;
-    if (req->df) {
-        addr -= offset;
-    } else {
-        addr += offset;
-    }
-    cpu_physical_memory_rw(addr, val, req->size, rw);
-}
-
-static inline void read_phys_req_item(hwaddr addr,
-                                      ioreq_t *req, uint32_t i, void *val)
-{
-    rw_phys_req_item(addr, req, i, val, 0);
-}
-static inline void write_phys_req_item(hwaddr addr,
-                                       ioreq_t *req, uint32_t i, void *val)
-{
-    rw_phys_req_item(addr, req, i, val, 1);
-}
-
-
-static void cpu_ioreq_pio(ioreq_t *req)
-{
-    uint32_t i;
-
-    trace_cpu_ioreq_pio(req, req->dir, req->df, req->data_is_ptr, req->addr,
-                         req->data, req->count, req->size);
-
-    if (req->size > sizeof(uint32_t)) {
-        hw_error("PIO: bad size (%u)", req->size);
-    }
-
-    if (req->dir == IOREQ_READ) {
-        if (!req->data_is_ptr) {
-            req->data = do_inp(req->addr, req->size);
-            trace_cpu_ioreq_pio_read_reg(req, req->data, req->addr,
-                                         req->size);
-        } else {
-            uint32_t tmp;
-
-            for (i = 0; i < req->count; i++) {
-                tmp = do_inp(req->addr, req->size);
-                write_phys_req_item(req->data, req, i, &tmp);
-            }
-        }
-    } else if (req->dir == IOREQ_WRITE) {
-        if (!req->data_is_ptr) {
-            trace_cpu_ioreq_pio_write_reg(req, req->data, req->addr,
-                                          req->size);
-            do_outp(req->addr, req->size, req->data);
-        } else {
-            for (i = 0; i < req->count; i++) {
-                uint32_t tmp = 0;
-
-                read_phys_req_item(req->data, req, i, &tmp);
-                do_outp(req->addr, req->size, tmp);
-            }
-        }
-    }
-}
-
-static void cpu_ioreq_move(ioreq_t *req)
-{
-    uint32_t i;
-
-    trace_cpu_ioreq_move(req, req->dir, req->df, req->data_is_ptr, req->addr,
-                         req->data, req->count, req->size);
-
-    if (req->size > sizeof(req->data)) {
-        hw_error("MMIO: bad size (%u)", req->size);
-    }
-
-    if (!req->data_is_ptr) {
-        if (req->dir == IOREQ_READ) {
-            for (i = 0; i < req->count; i++) {
-                read_phys_req_item(req->addr, req, i, &req->data);
-            }
-        } else if (req->dir == IOREQ_WRITE) {
-            for (i = 0; i < req->count; i++) {
-                write_phys_req_item(req->addr, req, i, &req->data);
-            }
-        }
-    } else {
-        uint64_t tmp;
-
-        if (req->dir == IOREQ_READ) {
-            for (i = 0; i < req->count; i++) {
-                read_phys_req_item(req->addr, req, i, &tmp);
-                write_phys_req_item(req->data, req, i, &tmp);
-            }
-        } else if (req->dir == IOREQ_WRITE) {
-            for (i = 0; i < req->count; i++) {
-                read_phys_req_item(req->data, req, i, &tmp);
-                write_phys_req_item(req->addr, req, i, &tmp);
-            }
-        }
-    }
-}
-
-static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
-{
-    uint32_t sbdf = req->addr >> 32;
-    uint32_t reg = req->addr;
-    XenPciDevice *xendev;
-
-    if (req->size != sizeof(uint8_t) && req->size != sizeof(uint16_t) &&
-        req->size != sizeof(uint32_t)) {
-        hw_error("PCI config access: bad size (%u)", req->size);
-    }
-
-    if (req->count != 1) {
-        hw_error("PCI config access: bad count (%u)", req->count);
-    }
-
-    QLIST_FOREACH(xendev, &state->dev_list, entry) {
-        if (xendev->sbdf != sbdf) {
-            continue;
-        }
-
-        if (!req->data_is_ptr) {
-            if (req->dir == IOREQ_READ) {
-                req->data = pci_host_config_read_common(
-                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
-                    req->size);
-                trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
-                                            req->size, req->data);
-            } else if (req->dir == IOREQ_WRITE) {
-                trace_cpu_ioreq_config_write(req, xendev->sbdf, reg,
-                                             req->size, req->data);
-                pci_host_config_write_common(
-                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
-                    req->data, req->size);
-            }
-        } else {
-            uint32_t tmp;
-
-            if (req->dir == IOREQ_READ) {
-                tmp = pci_host_config_read_common(
-                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
-                    req->size);
-                trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
-                                            req->size, tmp);
-                write_phys_req_item(req->data, req, 0, &tmp);
-            } else if (req->dir == IOREQ_WRITE) {
-                read_phys_req_item(req->data, req, 0, &tmp);
-                trace_cpu_ioreq_config_write(req, xendev->sbdf, reg,
-                                             req->size, tmp);
-                pci_host_config_write_common(
-                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
-                    tmp, req->size);
-            }
-        }
-    }
-}
-
 static void regs_to_cpu(vmware_regs_t *vmport_regs, ioreq_t *req)
 {
     X86CPU *cpu;
@@ -996,223 +510,6 @@ static void handle_vmport_ioreq(XenIOState *state, ioreq_t *req)
     current_cpu = NULL;
 }
 
-static void handle_ioreq(XenIOState *state, ioreq_t *req)
-{
-    trace_handle_ioreq(req, req->type, req->dir, req->df, req->data_is_ptr,
-                       req->addr, req->data, req->count, req->size);
-
-    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
-            (req->size < sizeof (target_ulong))) {
-        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
-    }
-
-    if (req->dir == IOREQ_WRITE)
-        trace_handle_ioreq_write(req, req->type, req->df, req->data_is_ptr,
-                                 req->addr, req->data, req->count, req->size);
-
-    switch (req->type) {
-        case IOREQ_TYPE_PIO:
-            cpu_ioreq_pio(req);
-            break;
-        case IOREQ_TYPE_COPY:
-            cpu_ioreq_move(req);
-            break;
-        case IOREQ_TYPE_TIMEOFFSET:
-            break;
-        case IOREQ_TYPE_INVALIDATE:
-            xen_invalidate_map_cache();
-            break;
-        case IOREQ_TYPE_PCI_CONFIG:
-            cpu_ioreq_config(state, req);
-            break;
-        default:
-            arch_handle_ioreq(state, req);
-    }
-    if (req->dir == IOREQ_READ) {
-        trace_handle_ioreq_read(req, req->type, req->df, req->data_is_ptr,
-                                req->addr, req->data, req->count, req->size);
-    }
-}
-
-static bool handle_buffered_iopage(XenIOState *state)
-{
-    buffered_iopage_t *buf_page = state->buffered_io_page;
-    buf_ioreq_t *buf_req = NULL;
-    bool handled_ioreq = false;
-    ioreq_t req;
-    int qw;
-
-    if (!buf_page) {
-        return 0;
-    }
-
-    memset(&req, 0x00, sizeof(req));
-    req.state = STATE_IOREQ_READY;
-    req.count = 1;
-    req.dir = IOREQ_WRITE;
-
-    for (;;) {
-        uint32_t rdptr = buf_page->read_pointer, wrptr;
-
-        xen_rmb();
-        wrptr = buf_page->write_pointer;
-        xen_rmb();
-        if (rdptr != buf_page->read_pointer) {
-            continue;
-        }
-        if (rdptr == wrptr) {
-            break;
-        }
-        buf_req = &buf_page->buf_ioreq[rdptr % IOREQ_BUFFER_SLOT_NUM];
-        req.size = 1U << buf_req->size;
-        req.addr = buf_req->addr;
-        req.data = buf_req->data;
-        req.type = buf_req->type;
-        xen_rmb();
-        qw = (req.size == 8);
-        if (qw) {
-            if (rdptr + 1 == wrptr) {
-                hw_error("Incomplete quad word buffered ioreq");
-            }
-            buf_req = &buf_page->buf_ioreq[(rdptr + 1) %
-                                           IOREQ_BUFFER_SLOT_NUM];
-            req.data |= ((uint64_t)buf_req->data) << 32;
-            xen_rmb();
-        }
-
-        handle_ioreq(state, &req);
-
-        /* Only req.data may get updated by handle_ioreq(), albeit even that
-         * should not happen as such data would never make it to the guest (we
-         * can only usefully see writes here after all).
-         */
-        assert(req.state == STATE_IOREQ_READY);
-        assert(req.count == 1);
-        assert(req.dir == IOREQ_WRITE);
-        assert(!req.data_is_ptr);
-
-        qatomic_add(&buf_page->read_pointer, qw + 1);
-        handled_ioreq = true;
-    }
-
-    return handled_ioreq;
-}
-
-static void handle_buffered_io(void *opaque)
-{
-    XenIOState *state = opaque;
-
-    if (handle_buffered_iopage(state)) {
-        timer_mod(state->buffered_io_timer,
-                BUFFER_IO_MAX_DELAY + qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
-    } else {
-        timer_del(state->buffered_io_timer);
-        xenevtchn_unmask(state->xce_handle, state->bufioreq_local_port);
-    }
-}
-
-static void cpu_handle_ioreq(void *opaque)
-{
-    XenIOState *state = opaque;
-    ioreq_t *req = cpu_get_ioreq(state);
-
-    handle_buffered_iopage(state);
-    if (req) {
-        ioreq_t copy = *req;
-
-        xen_rmb();
-        handle_ioreq(state, &copy);
-        req->data = copy.data;
-
-        if (req->state != STATE_IOREQ_INPROCESS) {
-            fprintf(stderr, "Badness in I/O request ... not in service?!: "
-                    "%x, ptr: %x, port: %"PRIx64", "
-                    "data: %"PRIx64", count: %u, size: %u, type: %u\n",
-                    req->state, req->data_is_ptr, req->addr,
-                    req->data, req->count, req->size, req->type);
-            destroy_hvm_domain(false);
-            return;
-        }
-
-        xen_wmb(); /* Update ioreq contents /then/ update state. */
-
-        /*
-         * We do this before we send the response so that the tools
-         * have the opportunity to pick up on the reset before the
-         * guest resumes and does a hlt with interrupts disabled which
-         * causes Xen to powerdown the domain.
-         */
-        if (runstate_is_running()) {
-            ShutdownCause request;
-
-            if (qemu_shutdown_requested_get()) {
-                destroy_hvm_domain(false);
-            }
-            request = qemu_reset_requested_get();
-            if (request) {
-                qemu_system_reset(request);
-                destroy_hvm_domain(true);
-            }
-        }
-
-        req->state = STATE_IORESP_READY;
-        xenevtchn_notify(state->xce_handle,
-                         state->ioreq_local_port[state->send_vcpu]);
-    }
-}
-
-static void xen_main_loop_prepare(XenIOState *state)
-{
-    int evtchn_fd = -1;
-
-    if (state->xce_handle != NULL) {
-        evtchn_fd = xenevtchn_fd(state->xce_handle);
-    }
-
-    state->buffered_io_timer = timer_new_ms(QEMU_CLOCK_REALTIME, handle_buffered_io,
-                                                 state);
-
-    if (evtchn_fd != -1) {
-        CPUState *cpu_state;
-
-        DPRINTF("%s: Init cpu_by_vcpu_id\n", __func__);
-        CPU_FOREACH(cpu_state) {
-            DPRINTF("%s: cpu_by_vcpu_id[%d]=%p\n",
-                    __func__, cpu_state->cpu_index, cpu_state);
-            state->cpu_by_vcpu_id[cpu_state->cpu_index] = cpu_state;
-        }
-        qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
-    }
-}
-
-
-static void xen_hvm_change_state_handler(void *opaque, bool running,
-                                         RunState rstate)
-{
-    XenIOState *state = opaque;
-
-    if (running) {
-        xen_main_loop_prepare(state);
-    }
-
-    xen_set_ioreq_server_state(xen_domid,
-                               state->ioservid,
-                               (rstate == RUN_STATE_RUNNING));
-}
-
-static void xen_exit_notifier(Notifier *n, void *data)
-{
-    XenIOState *state = container_of(n, XenIOState, exit);
-
-    xen_destroy_ioreq_server(xen_domid, state->ioservid);
-    if (state->fres != NULL) {
-        xenforeignmemory_unmap_resource(xen_fmem, state->fres);
-    }
-
-    xenevtchn_close(state->xce_handle);
-    xs_daemon_close(state->xenstore);
-}
-
 #ifdef XEN_COMPAT_PHYSMAP
 static void xen_read_physmap(XenIOState *state)
 {
@@ -1272,178 +569,17 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-static int xen_map_ioreq_server(XenIOState *state)
-{
-    void *addr = NULL;
-    xen_pfn_t ioreq_pfn;
-    xen_pfn_t bufioreq_pfn;
-    evtchn_port_t bufioreq_evtchn;
-    int rc;
-
-    /*
-     * Attempt to map using the resource API and fall back to normal
-     * foreign mapping if this is not supported.
-     */
-    QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq != 0);
-    QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1);
-    state->fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
-                                         XENMEM_resource_ioreq_server,
-                                         state->ioservid, 0, 2,
-                                         &addr,
-                                         PROT_READ | PROT_WRITE, 0);
-    if (state->fres != NULL) {
-        trace_xen_map_resource_ioreq(state->ioservid, addr);
-        state->buffered_io_page = addr;
-        state->shared_page = addr + TARGET_PAGE_SIZE;
-    } else if (errno != EOPNOTSUPP) {
-        error_report("failed to map ioreq server resources: error %d handle=%p",
-                     errno, xen_xc);
-        return -1;
-    }
-
-    rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
-                                   (state->shared_page == NULL) ?
-                                   &ioreq_pfn : NULL,
-                                   (state->buffered_io_page == NULL) ?
-                                   &bufioreq_pfn : NULL,
-                                   &bufioreq_evtchn);
-    if (rc < 0) {
-        error_report("failed to get ioreq server info: error %d handle=%p",
-                     errno, xen_xc);
-        return rc;
-    }
-
-    if (state->shared_page == NULL) {
-        DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
-
-        state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
-                                                  PROT_READ | PROT_WRITE,
-                                                  1, &ioreq_pfn, NULL);
-        if (state->shared_page == NULL) {
-            error_report("map shared IO page returned error %d handle=%p",
-                         errno, xen_xc);
-        }
-    }
-
-    if (state->buffered_io_page == NULL) {
-        DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn);
-
-        state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid,
-                                                       PROT_READ | PROT_WRITE,
-                                                       1, &bufioreq_pfn,
-                                                       NULL);
-        if (state->buffered_io_page == NULL) {
-            error_report("map buffered IO page returned error %d", errno);
-            return -1;
-        }
-    }
-
-    if (state->shared_page == NULL || state->buffered_io_page == NULL) {
-        return -1;
-    }
-
-    DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn);
-
-    state->bufioreq_remote_port = bufioreq_evtchn;
-
-    return 0;
-}
-
 void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory)
 {
     MachineState *ms = MACHINE(pcms);
     unsigned int max_cpus = ms->smp.max_cpus;
-    int i, rc;
+    int rc;
     xen_pfn_t ioreq_pfn;
     XenIOState *state;
 
     state = g_new0(XenIOState, 1);
 
-    state->xce_handle = xenevtchn_open(NULL, 0);
-    if (state->xce_handle == NULL) {
-        perror("xen: event channel open");
-        goto err;
-    }
-
-    state->xenstore = xs_daemon_open();
-    if (state->xenstore == NULL) {
-        perror("xen: xenstore open");
-        goto err;
-    }
-
-    xen_create_ioreq_server(xen_domid, &state->ioservid);
-
-    state->exit.notify = xen_exit_notifier;
-    qemu_add_exit_notifier(&state->exit);
-
-    /*
-     * Register wake-up support in QMP query-current-machine API
-     */
-    qemu_register_wakeup_support();
-
-    rc = xen_map_ioreq_server(state);
-    if (rc < 0) {
-        goto err;
-    }
-
-    /* Note: cpus is empty at this point in init */
-    state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
-
-    rc = xen_set_ioreq_server_state(xen_domid, state->ioservid, true);
-    if (rc < 0) {
-        error_report("failed to enable ioreq server info: error %d handle=%p",
-                     errno, xen_xc);
-        goto err;
-    }
-
-    state->ioreq_local_port = g_new0(evtchn_port_t, max_cpus);
-
-    /* FIXME: how about if we overflow the page here? */
-    for (i = 0; i < max_cpus; i++) {
-        rc = xenevtchn_bind_interdomain(state->xce_handle, xen_domid,
-                                        xen_vcpu_eport(state->shared_page, i));
-        if (rc == -1) {
-            error_report("shared evtchn %d bind error %d", i, errno);
-            goto err;
-        }
-        state->ioreq_local_port[i] = rc;
-    }
-
-    rc = xenevtchn_bind_interdomain(state->xce_handle, xen_domid,
-                                    state->bufioreq_remote_port);
-    if (rc == -1) {
-        error_report("buffered evtchn bind error %d", errno);
-        goto err;
-    }
-    state->bufioreq_local_port = rc;
-
-    /* Init RAM management */
-#ifdef XEN_COMPAT_PHYSMAP
-    xen_map_cache_init(xen_phys_offset_to_gaddr, state);
-#else
-    xen_map_cache_init(NULL, state);
-#endif
-
-    qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
-
-    state->memory_listener = xen_memory_listener;
-    memory_listener_register(&state->memory_listener, &address_space_memory);
-
-    state->io_listener = xen_io_listener;
-    memory_listener_register(&state->io_listener, &address_space_io);
-
-    state->device_listener = xen_device_listener;
-    QLIST_INIT(&state->dev_list);
-    device_listener_register(&state->device_listener);
-
-    xen_bus_init();
-
-    /* Initialize backend core & drivers */
-    if (xen_be_init() != 0) {
-        error_report("xen backend core setup failed");
-        goto err;
-    }
-    xen_be_register_common();
+    xen_register_ioreq(state, max_cpus, xen_memory_listener);
 
     QLIST_INIT(&xen_physmap);
     xen_read_physmap(state);
@@ -1483,59 +619,11 @@ err:
     exit(1);
 }
 
-void destroy_hvm_domain(bool reboot)
-{
-    xc_interface *xc_handle;
-    int sts;
-    int rc;
-
-    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
-
-    if (xen_dmod) {
-        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
-        if (!rc) {
-            return;
-        }
-        if (errno != ENOTTY /* old Xen */) {
-            perror("xendevicemodel_shutdown failed");
-        }
-        /* well, try the old thing then */
-    }
-
-    xc_handle = xc_interface_open(0, 0, 0);
-    if (xc_handle == NULL) {
-        fprintf(stderr, "Cannot acquire xenctrl handle\n");
-    } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
-        if (sts != 0) {
-            fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
-                    "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-                    sts, strerror(errno));
-        } else {
-            fprintf(stderr, "Issued domain %d %s\n", xen_domid,
-                    reboot ? "reboot" : "poweroff");
-        }
-        xc_interface_close(xc_handle);
-    }
-}
-
 void xen_register_framebuffer(MemoryRegion *mr)
 {
     framebuffer = mr;
 }
 
-void xen_shutdown_fatal_error(const char *fmt, ...)
-{
-    va_list ap;
-
-    va_start(ap, fmt);
-    vfprintf(stderr, fmt, ap);
-    va_end(ap);
-    fprintf(stderr, "Will destroy the domain.\n");
-    /* destroy the domain */
-    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
-}
-
 void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
 {
     if (unlikely(xen_in_migration)) {
diff --git a/hw/xen/meson.build b/hw/xen/meson.build
index 19d0637c46..008e036d63 100644
--- a/hw/xen/meson.build
+++ b/hw/xen/meson.build
@@ -25,4 +25,7 @@ specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
 
 xen_ss = ss.source_set()
 
-xen_ss.add(when: 'CONFIG_XEN', if_true: files('xen-mapcache.c'))
+xen_ss.add(when: 'CONFIG_XEN', if_true: files(
+  'xen-mapcache.c',
+  'xen-hvm-common.c',
+))
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 2c8f238f42..02ca1183da 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -42,6 +42,20 @@ xs_node_vscanf(char *path, char *value) "%s %s"
 xs_node_watch(char *path) "%s"
 xs_node_unwatch(char *path) "%s"
 
+# xen-hvm.c
+xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: 0x%lx, size 0x%lx"
+xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) "0x%"PRIx64" size 0x%lx, log_dirty %i"
+handle_ioreq(void *req, uint32_t type, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p type=%d dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
+handle_ioreq_read(void *req, uint32_t type, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p read type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
+handle_ioreq_write(void *req, uint32_t type, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p write type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
+cpu_ioreq_pio(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p pio dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
+cpu_ioreq_pio_read_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio read reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
+cpu_ioreq_pio_write_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio write reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
+cpu_ioreq_move(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p copy dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
+xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
+cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
+cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
+
 # xen-mapcache.c
 xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
 xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
new file mode 100644
index 0000000000..f848f9e625
--- /dev/null
+++ b/hw/xen/xen-hvm-common.c
@@ -0,0 +1,858 @@
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/pci/pci_host.h"
+#include "hw/xen/xen-hvm-common.h"
+#include "hw/xen/xen-legacy-backend.h"
+#include "hw/xen/xen-bus.h"
+#include "hw/boards.h"
+#include "hw/xen/arch_hvm.h"
+
+MemoryRegion ram_memory;
+
+void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
+                   Error **errp)
+{
+    unsigned long nr_pfn;
+    xen_pfn_t *pfn_list;
+    int i;
+
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* RAM already populated in Xen */
+        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
+                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
+                __func__, size, ram_addr);
+        return;
+    }
+
+    if (mr == &ram_memory) {
+        return;
+    }
+
+    trace_xen_ram_alloc(ram_addr, size);
+
+    nr_pfn = size >> TARGET_PAGE_BITS;
+    pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
+
+    for (i = 0; i < nr_pfn; i++) {
+        pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i;
+    }
+
+    if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
+        error_setg(errp, "xen: failed to populate ram at " RAM_ADDR_FMT,
+                   ram_addr);
+    }
+
+    g_free(pfn_list);
+}
+
+
+static void xen_set_memory(struct MemoryListener *listener,
+                           MemoryRegionSection *section,
+                           bool add)
+{
+    XenIOState *state = container_of(listener, XenIOState, memory_listener);
+
+    if (section->mr == &ram_memory) {
+        return;
+    } else {
+        if (add) {
+            xen_map_memory_section(xen_domid, state->ioservid,
+                                   section);
+        } else {
+            xen_unmap_memory_section(xen_domid, state->ioservid,
+                                     section);
+        }
+    }
+    arch_xen_set_memory(state, section, add);
+}
+
+void xen_region_add(MemoryListener *listener,
+                           MemoryRegionSection *section)
+{
+    memory_region_ref(section->mr);
+    xen_set_memory(listener, section, true);
+}
+
+void xen_region_del(MemoryListener *listener,
+                           MemoryRegionSection *section)
+{
+    xen_set_memory(listener, section, false);
+    memory_region_unref(section->mr);
+}
+
+void xen_io_add(MemoryListener *listener,
+                       MemoryRegionSection *section)
+{
+    XenIOState *state = container_of(listener, XenIOState, io_listener);
+    MemoryRegion *mr = section->mr;
+
+    if (mr->ops == &unassigned_io_ops) {
+        return;
+    }
+
+    memory_region_ref(mr);
+
+    xen_map_io_section(xen_domid, state->ioservid, section);
+}
+
+void xen_io_del(MemoryListener *listener,
+                       MemoryRegionSection *section)
+{
+    XenIOState *state = container_of(listener, XenIOState, io_listener);
+    MemoryRegion *mr = section->mr;
+
+    if (mr->ops == &unassigned_io_ops) {
+        return;
+    }
+
+    xen_unmap_io_section(xen_domid, state->ioservid, section);
+
+    memory_region_unref(mr);
+}
+
+void xen_device_realize(DeviceListener *listener,
+                               DeviceState *dev)
+{
+    XenIOState *state = container_of(listener, XenIOState, device_listener);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pci_dev = PCI_DEVICE(dev);
+        XenPciDevice *xendev = g_new(XenPciDevice, 1);
+
+        xendev->pci_dev = pci_dev;
+        xendev->sbdf = PCI_BUILD_BDF(pci_dev_bus_num(pci_dev),
+                                     pci_dev->devfn);
+        QLIST_INSERT_HEAD(&state->dev_list, xendev, entry);
+
+        xen_map_pcidev(xen_domid, state->ioservid, pci_dev);
+    }
+}
+
+void xen_device_unrealize(DeviceListener *listener,
+                                 DeviceState *dev)
+{
+    XenIOState *state = container_of(listener, XenIOState, device_listener);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pci_dev = PCI_DEVICE(dev);
+        XenPciDevice *xendev, *next;
+
+        xen_unmap_pcidev(xen_domid, state->ioservid, pci_dev);
+
+        QLIST_FOREACH_SAFE(xendev, &state->dev_list, entry, next) {
+            if (xendev->pci_dev == pci_dev) {
+                QLIST_REMOVE(xendev, entry);
+                g_free(xendev);
+                break;
+            }
+        }
+    }
+}
+
+MemoryListener xen_io_listener = {
+    .region_add = xen_io_add,
+    .region_del = xen_io_del,
+    .priority = 10,
+};
+
+DeviceListener xen_device_listener = {
+    .realize = xen_device_realize,
+    .unrealize = xen_device_unrealize,
+};
+
+/* get the ioreq packets from share mem */
+static ioreq_t *cpu_get_ioreq_from_shared_memory(XenIOState *state, int vcpu)
+{
+    ioreq_t *req = xen_vcpu_ioreq(state->shared_page, vcpu);
+
+    if (req->state != STATE_IOREQ_READY) {
+        DPRINTF("I/O request not ready: "
+                "%x, ptr: %x, port: %"PRIx64", "
+                "data: %"PRIx64", count: %u, size: %u\n",
+                req->state, req->data_is_ptr, req->addr,
+                req->data, req->count, req->size);
+        return NULL;
+    }
+
+    xen_rmb(); /* see IOREQ_READY /then/ read contents of ioreq */
+
+    req->state = STATE_IOREQ_INPROCESS;
+    return req;
+}
+
+/* use poll to get the port notification */
+/* ioreq_vec--out,the */
+/* retval--the number of ioreq packet */
+static ioreq_t *cpu_get_ioreq(XenIOState *state)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    unsigned int max_cpus = ms->smp.max_cpus;
+    int i;
+    evtchn_port_t port;
+
+    port = xenevtchn_pending(state->xce_handle);
+    if (port == state->bufioreq_local_port) {
+        timer_mod(state->buffered_io_timer,
+                BUFFER_IO_MAX_DELAY + qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
+        return NULL;
+    }
+
+    if (port != -1) {
+        for (i = 0; i < max_cpus; i++) {
+            if (state->ioreq_local_port[i] == port) {
+                break;
+            }
+        }
+
+        if (i == max_cpus) {
+            hw_error("Fatal error while trying to get io event!\n");
+        }
+
+        /* unmask the wanted port again */
+        xenevtchn_unmask(state->xce_handle, port);
+
+        /* get the io packet from shared memory */
+        state->send_vcpu = i;
+        return cpu_get_ioreq_from_shared_memory(state, i);
+    }
+
+    /* read error or read nothing */
+    return NULL;
+}
+
+static uint32_t do_inp(uint32_t addr, unsigned long size)
+{
+    switch (size) {
+        case 1:
+            return cpu_inb(addr);
+        case 2:
+            return cpu_inw(addr);
+        case 4:
+            return cpu_inl(addr);
+        default:
+            hw_error("inp: bad size: %04x %lx", addr, size);
+    }
+}
+
+static void do_outp(uint32_t addr,
+        unsigned long size, uint32_t val)
+{
+    switch (size) {
+        case 1:
+            return cpu_outb(addr, val);
+        case 2:
+            return cpu_outw(addr, val);
+        case 4:
+            return cpu_outl(addr, val);
+        default:
+            hw_error("outp: bad size: %04x %lx", addr, size);
+    }
+}
+
+/*
+ * Helper functions which read/write an object from/to physical guest
+ * memory, as part of the implementation of an ioreq.
+ *
+ * Equivalent to
+ *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
+ *                          val, req->size, 0/1)
+ * except without the integer overflow problems.
+ */
+static void rw_phys_req_item(hwaddr addr,
+                             ioreq_t *req, uint32_t i, void *val, int rw)
+{
+    /* Do everything unsigned so overflow just results in a truncated result
+     * and accesses to undesired parts of guest memory, which is up
+     * to the guest */
+    hwaddr offset = (hwaddr)req->size * i;
+    if (req->df) {
+        addr -= offset;
+    } else {
+        addr += offset;
+    }
+    cpu_physical_memory_rw(addr, val, req->size, rw);
+}
+
+static inline void read_phys_req_item(hwaddr addr,
+                                      ioreq_t *req, uint32_t i, void *val)
+{
+    rw_phys_req_item(addr, req, i, val, 0);
+}
+static inline void write_phys_req_item(hwaddr addr,
+                                       ioreq_t *req, uint32_t i, void *val)
+{
+    rw_phys_req_item(addr, req, i, val, 1);
+}
+
+
+void cpu_ioreq_pio(ioreq_t *req)
+{
+    uint32_t i;
+
+    trace_cpu_ioreq_pio(req, req->dir, req->df, req->data_is_ptr, req->addr,
+                         req->data, req->count, req->size);
+
+    if (req->size > sizeof(uint32_t)) {
+        hw_error("PIO: bad size (%u)", req->size);
+    }
+
+    if (req->dir == IOREQ_READ) {
+        if (!req->data_is_ptr) {
+            req->data = do_inp(req->addr, req->size);
+            trace_cpu_ioreq_pio_read_reg(req, req->data, req->addr,
+                                         req->size);
+        } else {
+            uint32_t tmp;
+
+            for (i = 0; i < req->count; i++) {
+                tmp = do_inp(req->addr, req->size);
+                write_phys_req_item(req->data, req, i, &tmp);
+            }
+        }
+    } else if (req->dir == IOREQ_WRITE) {
+        if (!req->data_is_ptr) {
+            trace_cpu_ioreq_pio_write_reg(req, req->data, req->addr,
+                                          req->size);
+            do_outp(req->addr, req->size, req->data);
+        } else {
+            for (i = 0; i < req->count; i++) {
+                uint32_t tmp = 0;
+
+                read_phys_req_item(req->data, req, i, &tmp);
+                do_outp(req->addr, req->size, tmp);
+            }
+        }
+    }
+}
+
+static void cpu_ioreq_move(ioreq_t *req)
+{
+    uint32_t i;
+
+    trace_cpu_ioreq_move(req, req->dir, req->df, req->data_is_ptr, req->addr,
+                         req->data, req->count, req->size);
+
+    if (req->size > sizeof(req->data)) {
+        hw_error("MMIO: bad size (%u)", req->size);
+    }
+
+    if (!req->data_is_ptr) {
+        if (req->dir == IOREQ_READ) {
+            for (i = 0; i < req->count; i++) {
+                read_phys_req_item(req->addr, req, i, &req->data);
+            }
+        } else if (req->dir == IOREQ_WRITE) {
+            for (i = 0; i < req->count; i++) {
+                write_phys_req_item(req->addr, req, i, &req->data);
+            }
+        }
+    } else {
+        uint64_t tmp;
+
+        if (req->dir == IOREQ_READ) {
+            for (i = 0; i < req->count; i++) {
+                read_phys_req_item(req->addr, req, i, &tmp);
+                write_phys_req_item(req->data, req, i, &tmp);
+            }
+        } else if (req->dir == IOREQ_WRITE) {
+            for (i = 0; i < req->count; i++) {
+                read_phys_req_item(req->data, req, i, &tmp);
+                write_phys_req_item(req->addr, req, i, &tmp);
+            }
+        }
+    }
+}
+
+static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
+{
+    uint32_t sbdf = req->addr >> 32;
+    uint32_t reg = req->addr;
+    XenPciDevice *xendev;
+
+    if (req->size != sizeof(uint8_t) && req->size != sizeof(uint16_t) &&
+        req->size != sizeof(uint32_t)) {
+        hw_error("PCI config access: bad size (%u)", req->size);
+    }
+
+    if (req->count != 1) {
+        hw_error("PCI config access: bad count (%u)", req->count);
+    }
+
+    QLIST_FOREACH(xendev, &state->dev_list, entry) {
+        if (xendev->sbdf != sbdf) {
+            continue;
+        }
+
+        if (!req->data_is_ptr) {
+            if (req->dir == IOREQ_READ) {
+                req->data = pci_host_config_read_common(
+                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
+                    req->size);
+                trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
+                                            req->size, req->data);
+            } else if (req->dir == IOREQ_WRITE) {
+                trace_cpu_ioreq_config_write(req, xendev->sbdf, reg,
+                                             req->size, req->data);
+                pci_host_config_write_common(
+                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
+                    req->data, req->size);
+            }
+        } else {
+            uint32_t tmp;
+
+            if (req->dir == IOREQ_READ) {
+                tmp = pci_host_config_read_common(
+                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
+                    req->size);
+                trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
+                                            req->size, tmp);
+                write_phys_req_item(req->data, req, 0, &tmp);
+            } else if (req->dir == IOREQ_WRITE) {
+                read_phys_req_item(req->data, req, 0, &tmp);
+                trace_cpu_ioreq_config_write(req, xendev->sbdf, reg,
+                                             req->size, tmp);
+                pci_host_config_write_common(
+                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
+                    tmp, req->size);
+            }
+        }
+    }
+}
+
+static void handle_ioreq(XenIOState *state, ioreq_t *req)
+{
+    trace_handle_ioreq(req, req->type, req->dir, req->df, req->data_is_ptr,
+                       req->addr, req->data, req->count, req->size);
+
+    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
+            (req->size < sizeof (target_ulong))) {
+        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
+    }
+
+    if (req->dir == IOREQ_WRITE)
+        trace_handle_ioreq_write(req, req->type, req->df, req->data_is_ptr,
+                                 req->addr, req->data, req->count, req->size);
+
+    switch (req->type) {
+        case IOREQ_TYPE_PIO:
+            cpu_ioreq_pio(req);
+            break;
+        case IOREQ_TYPE_COPY:
+            cpu_ioreq_move(req);
+            break;
+        case IOREQ_TYPE_TIMEOFFSET:
+            break;
+        case IOREQ_TYPE_INVALIDATE:
+            xen_invalidate_map_cache();
+            break;
+        case IOREQ_TYPE_PCI_CONFIG:
+            cpu_ioreq_config(state, req);
+            break;
+        default:
+            arch_handle_ioreq(state, req);
+    }
+    if (req->dir == IOREQ_READ) {
+        trace_handle_ioreq_read(req, req->type, req->df, req->data_is_ptr,
+                                req->addr, req->data, req->count, req->size);
+    }
+}
+
+static int handle_buffered_iopage(XenIOState *state)
+{
+    buffered_iopage_t *buf_page = state->buffered_io_page;
+    buf_ioreq_t *buf_req = NULL;
+    ioreq_t req;
+    int qw;
+
+    if (!buf_page) {
+        return 0;
+    }
+
+    memset(&req, 0x00, sizeof(req));
+    req.state = STATE_IOREQ_READY;
+    req.count = 1;
+    req.dir = IOREQ_WRITE;
+
+    for (;;) {
+        uint32_t rdptr = buf_page->read_pointer, wrptr;
+
+        xen_rmb();
+        wrptr = buf_page->write_pointer;
+        xen_rmb();
+        if (rdptr != buf_page->read_pointer) {
+            continue;
+        }
+        if (rdptr == wrptr) {
+            break;
+        }
+        buf_req = &buf_page->buf_ioreq[rdptr % IOREQ_BUFFER_SLOT_NUM];
+        req.size = 1U << buf_req->size;
+        req.addr = buf_req->addr;
+        req.data = buf_req->data;
+        req.type = buf_req->type;
+        xen_rmb();
+        qw = (req.size == 8);
+        if (qw) {
+            if (rdptr + 1 == wrptr) {
+                hw_error("Incomplete quad word buffered ioreq");
+            }
+            buf_req = &buf_page->buf_ioreq[(rdptr + 1) %
+                                           IOREQ_BUFFER_SLOT_NUM];
+            req.data |= ((uint64_t)buf_req->data) << 32;
+            xen_rmb();
+        }
+
+        handle_ioreq(state, &req);
+
+        /* Only req.data may get updated by handle_ioreq(), albeit even that
+         * should not happen as such data would never make it to the guest (we
+         * can only usefully see writes here after all).
+         */
+        assert(req.state == STATE_IOREQ_READY);
+        assert(req.count == 1);
+        assert(req.dir == IOREQ_WRITE);
+        assert(!req.data_is_ptr);
+
+        qatomic_add(&buf_page->read_pointer, qw + 1);
+    }
+
+    return req.count;
+}
+
+static void handle_buffered_io(void *opaque)
+{
+    XenIOState *state = opaque;
+
+    if (handle_buffered_iopage(state)) {
+        timer_mod(state->buffered_io_timer,
+                BUFFER_IO_MAX_DELAY + qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
+    } else {
+        timer_del(state->buffered_io_timer);
+        xenevtchn_unmask(state->xce_handle, state->bufioreq_local_port);
+    }
+}
+
+static void cpu_handle_ioreq(void *opaque)
+{
+    XenIOState *state = opaque;
+    ioreq_t *req = cpu_get_ioreq(state);
+
+    handle_buffered_iopage(state);
+    if (req) {
+        ioreq_t copy = *req;
+
+        xen_rmb();
+        handle_ioreq(state, &copy);
+        req->data = copy.data;
+
+        if (req->state != STATE_IOREQ_INPROCESS) {
+            fprintf(stderr, "Badness in I/O request ... not in service?!: "
+                    "%x, ptr: %x, port: %"PRIx64", "
+                    "data: %"PRIx64", count: %u, size: %u, type: %u\n",
+                    req->state, req->data_is_ptr, req->addr,
+                    req->data, req->count, req->size, req->type);
+            destroy_hvm_domain(false);
+            return;
+        }
+
+        xen_wmb(); /* Update ioreq contents /then/ update state. */
+
+        /*
+         * We do this before we send the response so that the tools
+         * have the opportunity to pick up on the reset before the
+         * guest resumes and does a hlt with interrupts disabled which
+         * causes Xen to powerdown the domain.
+         */
+        if (runstate_is_running()) {
+            ShutdownCause request;
+
+            if (qemu_shutdown_requested_get()) {
+                destroy_hvm_domain(false);
+            }
+            request = qemu_reset_requested_get();
+            if (request) {
+                qemu_system_reset(request);
+                destroy_hvm_domain(true);
+            }
+        }
+
+        req->state = STATE_IORESP_READY;
+        xenevtchn_notify(state->xce_handle,
+                         state->ioreq_local_port[state->send_vcpu]);
+    }
+}
+
+static void xen_main_loop_prepare(XenIOState *state)
+{
+    int evtchn_fd = -1;
+
+    if (state->xce_handle != NULL) {
+        evtchn_fd = xenevtchn_fd(state->xce_handle);
+    }
+
+    state->buffered_io_timer = timer_new_ms(QEMU_CLOCK_REALTIME, handle_buffered_io,
+                                                 state);
+
+    if (evtchn_fd != -1) {
+        CPUState *cpu_state;
+
+        DPRINTF("%s: Init cpu_by_vcpu_id\n", __func__);
+        CPU_FOREACH(cpu_state) {
+            DPRINTF("%s: cpu_by_vcpu_id[%d]=%p\n",
+                    __func__, cpu_state->cpu_index, cpu_state);
+            state->cpu_by_vcpu_id[cpu_state->cpu_index] = cpu_state;
+        }
+        qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
+    }
+}
+
+
+void xen_hvm_change_state_handler(void *opaque, bool running,
+                                         RunState rstate)
+{
+    XenIOState *state = opaque;
+
+    if (running) {
+        xen_main_loop_prepare(state);
+    }
+
+    xen_set_ioreq_server_state(xen_domid,
+                               state->ioservid,
+                               (rstate == RUN_STATE_RUNNING));
+}
+
+void xen_exit_notifier(Notifier *n, void *data)
+{
+    XenIOState *state = container_of(n, XenIOState, exit);
+
+    xen_destroy_ioreq_server(xen_domid, state->ioservid);
+
+    xenevtchn_close(state->xce_handle);
+    xs_daemon_close(state->xenstore);
+}
+
+static int xen_map_ioreq_server(XenIOState *state)
+{
+    void *addr = NULL;
+    xenforeignmemory_resource_handle *fres;
+    xen_pfn_t ioreq_pfn;
+    xen_pfn_t bufioreq_pfn;
+    evtchn_port_t bufioreq_evtchn;
+    int rc;
+
+    /*
+     * Attempt to map using the resource API and fall back to normal
+     * foreign mapping if this is not supported.
+     */
+    QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq != 0);
+    QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1);
+    fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
+                                         XENMEM_resource_ioreq_server,
+                                         state->ioservid, 0, 2,
+                                         &addr,
+                                         PROT_READ | PROT_WRITE, 0);
+    if (fres != NULL) {
+        trace_xen_map_resource_ioreq(state->ioservid, addr);
+        state->buffered_io_page = addr;
+        state->shared_page = addr + TARGET_PAGE_SIZE;
+    } else if (errno != EOPNOTSUPP) {
+        error_report("failed to map ioreq server resources: error %d handle=%p",
+                     errno, xen_xc);
+        return -1;
+    }
+
+    rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
+                                   (state->shared_page == NULL) ?
+                                   &ioreq_pfn : NULL,
+                                   (state->buffered_io_page == NULL) ?
+                                   &bufioreq_pfn : NULL,
+                                   &bufioreq_evtchn);
+    if (rc < 0) {
+        error_report("failed to get ioreq server info: error %d handle=%p",
+                     errno, xen_xc);
+        return rc;
+    }
+
+    if (state->shared_page == NULL) {
+        DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
+
+        state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
+                                                  PROT_READ | PROT_WRITE,
+                                                  1, &ioreq_pfn, NULL);
+        if (state->shared_page == NULL) {
+            error_report("map shared IO page returned error %d handle=%p",
+                         errno, xen_xc);
+        }
+    }
+
+    if (state->buffered_io_page == NULL) {
+        DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn);
+
+        state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid,
+                                                       PROT_READ | PROT_WRITE,
+                                                       1, &bufioreq_pfn,
+                                                       NULL);
+        if (state->buffered_io_page == NULL) {
+            error_report("map buffered IO page returned error %d", errno);
+            return -1;
+        }
+    }
+
+    if (state->shared_page == NULL || state->buffered_io_page == NULL) {
+        return -1;
+    }
+
+    DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn);
+
+    state->bufioreq_remote_port = bufioreq_evtchn;
+
+    return 0;
+}
+
+void destroy_hvm_domain(bool reboot)
+{
+    xc_interface *xc_handle;
+    int sts;
+    int rc;
+
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        if (errno != ENOTTY /* old Xen */) {
+            perror("xendevicemodel_shutdown failed");
+        }
+        /* well, try the old thing then */
+    }
+
+    xc_handle = xc_interface_open(0, 0, 0);
+    if (xc_handle == NULL) {
+        fprintf(stderr, "Cannot acquire xenctrl handle\n");
+    } else {
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
+        if (sts != 0) {
+            fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
+                    "sts %d, %s\n", reboot ? "reboot" : "poweroff",
+                    sts, strerror(errno));
+        } else {
+            fprintf(stderr, "Issued domain %d %s\n", xen_domid,
+                    reboot ? "reboot" : "poweroff");
+        }
+        xc_interface_close(xc_handle);
+    }
+}
+
+void xen_shutdown_fatal_error(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    vfprintf(stderr, fmt, ap);
+    va_end(ap);
+    fprintf(stderr, "Will destroy the domain.\n");
+    /* destroy the domain */
+    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
+}
+
+void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
+                        MemoryListener xen_memory_listener)
+{
+    int i, rc;
+
+    state->xce_handle = xenevtchn_open(NULL, 0);
+    if (state->xce_handle == NULL) {
+        perror("xen: event channel open");
+        goto err;
+    }
+
+    state->xenstore = xs_daemon_open();
+    if (state->xenstore == NULL) {
+        perror("xen: xenstore open");
+        goto err;
+    }
+
+    xen_create_ioreq_server(xen_domid, &state->ioservid);
+
+    state->exit.notify = xen_exit_notifier;
+    qemu_add_exit_notifier(&state->exit);
+
+    /*
+     * Register wake-up support in QMP query-current-machine API
+     */
+    qemu_register_wakeup_support();
+
+    rc = xen_map_ioreq_server(state);
+    if (rc < 0) {
+        goto err;
+    }
+
+    /* Note: cpus is empty at this point in init */
+    state->cpu_by_vcpu_id = g_malloc0(max_cpus * sizeof(CPUState *));
+
+    rc = xen_set_ioreq_server_state(xen_domid, state->ioservid, true);
+    if (rc < 0) {
+        error_report("failed to enable ioreq server info: error %d handle=%p",
+                     errno, xen_xc);
+        goto err;
+    }
+
+    state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));
+
+    /* FIXME: how about if we overflow the page here? */
+    for (i = 0; i < max_cpus; i++) {
+        rc = xenevtchn_bind_interdomain(state->xce_handle, xen_domid,
+                                        xen_vcpu_eport(state->shared_page, i));
+        if (rc == -1) {
+            error_report("shared evtchn %d bind error %d", i, errno);
+            goto err;
+        }
+        state->ioreq_local_port[i] = rc;
+    }
+
+    rc = xenevtchn_bind_interdomain(state->xce_handle, xen_domid,
+                                    state->bufioreq_remote_port);
+    if (rc == -1) {
+        error_report("buffered evtchn bind error %d", errno);
+        goto err;
+    }
+    state->bufioreq_local_port = rc;
+
+    /* Init RAM management */
+#ifdef XEN_COMPAT_PHYSMAP
+    xen_map_cache_init(xen_phys_offset_to_gaddr, state);
+#else
+    xen_map_cache_init(NULL, state);
+#endif
+
+    qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
+
+    state->memory_listener = xen_memory_listener;
+    memory_listener_register(&state->memory_listener, &address_space_memory);
+
+    state->io_listener = xen_io_listener;
+    memory_listener_register(&state->io_listener, &address_space_io);
+
+    state->device_listener = xen_device_listener;
+    QLIST_INIT(&state->dev_list);
+    device_listener_register(&state->device_listener);
+
+    xen_bus_init();
+
+    /* Initialize backend core & drivers */
+    if (xen_be_init() != 0) {
+        error_report("xen backend core setup failed");
+        goto err;
+    }
+    xen_be_register_common();
+
+    return;
+err:
+    error_report("xen hardware virtual machine initialisation failed");
+    exit(1);
+}
diff --git a/include/hw/i386/xen_arch_hvm.h b/include/hw/i386/xen_arch_hvm.h
index 1b2c71ba4f..1000f8f543 100644
--- a/include/hw/i386/xen_arch_hvm.h
+++ b/include/hw/i386/xen_arch_hvm.h
@@ -2,6 +2,7 @@
 #define HW_XEN_ARCH_I386_HVM_H
 
 #include <xen/hvm/ioreq.h>
+#include "hw/xen/xen-hvm-common.h"
 
 void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
 void arch_xen_set_memory(XenIOState *state,
diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
new file mode 100644
index 0000000000..c16057835f
--- /dev/null
+++ b/include/hw/xen/xen-hvm-common.h
@@ -0,0 +1,97 @@
+#ifndef HW_XEN_HVM_COMMON_H
+#define HW_XEN_HVM_COMMON_H
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+
+#include "cpu.h"
+#include "hw/pci/pci.h"
+#include "hw/hw.h"
+#include "hw/xen/xen_common.h"
+#include "sysemu/runstate.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/xen.h"
+#include "sysemu/xen-mapcache.h"
+
+#include <xen/hvm/ioreq.h>
+
+extern MemoryRegion ram_memory;
+extern MemoryListener xen_io_listener;
+extern DeviceListener xen_device_listener;
+
+//#define DEBUG_XEN_HVM
+
+#ifdef DEBUG_XEN_HVM
+#define DPRINTF(fmt, ...) \
+    do { fprintf(stderr, "xen: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+    do { } while (0)
+#endif
+
+static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
+{
+    return shared_page->vcpu_ioreq[i].vp_eport;
+}
+static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
+{
+    return &shared_page->vcpu_ioreq[vcpu];
+}
+
+#define BUFFER_IO_MAX_DELAY  100
+
+typedef struct XenPhysmap {
+    hwaddr start_addr;
+    ram_addr_t size;
+    const char *name;
+    hwaddr phys_offset;
+
+    QLIST_ENTRY(XenPhysmap) list;
+} XenPhysmap;
+
+typedef struct XenPciDevice {
+    PCIDevice *pci_dev;
+    uint32_t sbdf;
+    QLIST_ENTRY(XenPciDevice) entry;
+} XenPciDevice;
+
+typedef struct XenIOState {
+    ioservid_t ioservid;
+    shared_iopage_t *shared_page;
+    buffered_iopage_t *buffered_io_page;
+    QEMUTimer *buffered_io_timer;
+    CPUState **cpu_by_vcpu_id;
+    /* the evtchn port for polling the notification, */
+    evtchn_port_t *ioreq_local_port;
+    /* evtchn remote and local ports for buffered io */
+    evtchn_port_t bufioreq_remote_port;
+    evtchn_port_t bufioreq_local_port;
+    /* the evtchn fd for polling */
+    xenevtchn_handle *xce_handle;
+    /* which vcpu we are serving */
+    int send_vcpu;
+
+    struct xs_handle *xenstore;
+    MemoryListener memory_listener;
+    MemoryListener io_listener;
+    QLIST_HEAD(, XenPciDevice) dev_list;
+    DeviceListener device_listener;
+
+    Notifier exit;
+} XenIOState;
+
+void xen_exit_notifier(Notifier *n, void *data);
+
+void xen_region_add(MemoryListener *listener, MemoryRegionSection *section);
+void xen_region_del(MemoryListener *listener, MemoryRegionSection *section);
+void xen_io_add(MemoryListener *listener, MemoryRegionSection *section);
+void xen_io_del(MemoryListener *listener, MemoryRegionSection *section);
+void xen_device_realize(DeviceListener *listener, DeviceState *dev);
+void xen_device_unrealize(DeviceListener *listener, DeviceState *dev);
+
+void xen_hvm_change_state_handler(void *opaque, bool running, RunState rstate);
+void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
+                        MemoryListener xen_memory_listener);
+
+void cpu_ioreq_pio(ioreq_t *req);
+#endif /* HW_XEN_HVM_COMMON_H */
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (5 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-16 17:53   ` Julien Grall
  2022-10-15  5:07 ` [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure Vikram Garhwal
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:X86 Xen CPUs

From: Stefano Stabellini <stefano.stabellini@amd.com>

This is done to prepare for enabling xenpv support for ARM architecture.
On ARM it is possible to have a functioning xenpv machine with only the
PV backends and no IOREQ server. If the IOREQ server creation fails,
continue to the PV backends initialization.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 include/hw/xen/xen_common.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 77ce17d8a4..c2d2f36bde 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -467,8 +467,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
 {
 }
 
-static inline void xen_create_ioreq_server(domid_t dom,
-                                           ioservid_t *ioservid)
+static inline int xen_create_ioreq_server(domid_t dom,
+                                          ioservid_t *ioservid)
 {
 }
 
@@ -600,8 +600,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
                                                   PCI_FUNC(pci_dev->devfn));
 }
 
-static inline void xen_create_ioreq_server(domid_t dom,
-                                           ioservid_t *ioservid)
+static inline int xen_create_ioreq_server(domid_t dom,
+                                          ioservid_t *ioservid)
 {
     int rc = xendevicemodel_create_ioreq_server(xen_dmod, dom,
                                                 HVM_IOREQSRV_BUFIOREQ_ATOMIC,
@@ -609,12 +609,14 @@ static inline void xen_create_ioreq_server(domid_t dom,
 
     if (rc == 0) {
         trace_xen_ioreq_server_create(*ioservid);
-        return;
+        return rc;
     }
 
     *ioservid = 0;
     use_default_ioreq_server = true;
     trace_xen_default_ioreq_server();
+
+    return rc;
 }
 
 static inline void xen_destroy_ioreq_server(domid_t dom,
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (6 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-27  8:46   ` Alex Bennée
  2022-10-15  5:07 ` [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state Vikram Garhwal
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:X86 Xen CPUs

From: Stefano Stabellini <stefano.stabellini@amd.com>

On ARM it is possible to have a functioning xenpv machine with only the
PV backends and no IOREQ server. If the IOREQ server creation fails continue
to the PV backends initialization.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/xen/xen-hvm-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index f848f9e625..7bccf595fc 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -777,7 +777,11 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
         goto err;
     }
 
-    xen_create_ioreq_server(xen_domid, &state->ioservid);
+    rc = xen_create_ioreq_server(xen_domid, &state->ioservid);
+    if (rc) {
+        DPRINTF("xen: failed to create ioreq server\n");
+        goto no_ioreq;
+    }
 
     state->exit.notify = xen_exit_notifier;
     qemu_add_exit_notifier(&state->exit);
@@ -842,6 +846,7 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
     QLIST_INIT(&state->dev_list);
     device_listener_register(&state->device_listener);
 
+no_ioreq:
     xen_bus_init();
 
     /* Initialize backend core & drivers */
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (7 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-19 16:20   ` Paul Durrant
  2022-10-27  9:14   ` Alex Bennée
  2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
                   ` (4 subsequent siblings)
  13 siblings, 2 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:X86 Xen CPUs

xenstore_record_dm_state() will also be used in aarch64 xenpv machine.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 accel/xen/xen-all.c  | 2 +-
 include/hw/xen/xen.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b..276625b78b 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
 }
 
 
-static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
     char path[50];
 
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index afdf9c436a..31e9538a5c 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -9,6 +9,7 @@
  */
 
 #include "exec/cpu-common.h"
+#include <xenstore.h>
 
 /* xen-machine.c */
 enum xen_mode {
@@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
 void xenstore_store_pv_console_info(int i, Chardev *chr);
 
 void xen_register_framebuffer(struct MemoryRegion *mr);
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
 
 #endif /* QEMU_HW_XEN_H */
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (8 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-16 17:47   ` Julien Grall
                     ` (2 more replies)
  2022-10-15  5:07 ` [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM Vikram Garhwal
                   ` (3 subsequent siblings)
  13 siblings, 3 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: vikram.garhwal, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, open list:X86 Xen CPUs

Add a new machine xenpv which creates a IOREQ server to register/connect with
Xen Hypervisor.

Xen IOREQ connection expect the TARGET_PAGE_SIZE to 4096, and the xenpv machine
on ARM will have no CPU definitions. We need to define TARGET_PAGE_SIZE
appropriately ourselves.

Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
TPM emulator and connects to swtpm running on host machine via chardev socket
and support TPM functionalities for a guest domain.

Extra command line for aarch64 xenpv QEMU to connect to swtpm:
    -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
    -tpmdev emulator,id=tpm0,chardev=chrtpm \

swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
provides access to TPM functionality over socket, chardev and CUSE interface.
Github repo: https://github.com/stefanberger/swtpm
Example for starting swtpm on host machine:
    mkdir /tmp/vtpm2
    swtpm socket --tpmstate dir=/tmp/vtpm2 \
    --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/arm/meson.build            |   2 +
 hw/arm/xen_arm.c              | 163 ++++++++++++++++++++++++++++++++++
 include/hw/arm/xen_arch_hvm.h |  12 +++
 include/hw/xen/arch_hvm.h     |   2 +
 4 files changed, 179 insertions(+)
 create mode 100644 hw/arm/xen_arm.c
 create mode 100644 include/hw/arm/xen_arch_hvm.h

diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index 92f9f6e000..0cae024374 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -62,5 +62,7 @@ arm_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.
 arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c', 'smmuv3.c'))
 arm_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-imx6ul.c', 'mcimx6ul-evk.c'))
 arm_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_soc.c'))
+arm_ss.add(when: 'CONFIG_XEN', if_true: files('xen_arm.c'))
+arm_ss.add_all(xen_ss)
 
 hw_arch += {'arm': arm_ss}
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
new file mode 100644
index 0000000000..0b900314cc
--- /dev/null
+++ b/hw/arm/xen_arm.c
@@ -0,0 +1,163 @@
+/*
+ * QEMU ARM Xen PV Machine
+ *
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qapi/qapi-commands-migration.h"
+#include "hw/boards.h"
+#include "hw/sysbus.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/tpm_backend.h"
+#include "sysemu/sysemu.h"
+#include "hw/xen/xen-legacy-backend.h"
+#include "hw/xen/xen-hvm-common.h"
+#include "sysemu/tpm.h"
+#include "hw/xen/arch_hvm.h"
+
+#define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpv")
+OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
+
+static MemoryListener xen_memory_listener = {
+    .region_add = xen_region_add,
+    .region_del = xen_region_del,
+    .log_start = NULL,
+    .log_stop = NULL,
+    .log_sync = NULL,
+    .log_global_start = NULL,
+    .log_global_stop = NULL,
+    .priority = 10,
+};
+
+struct XenArmState {
+    /*< private >*/
+    MachineState parent;
+
+    XenIOState *state;
+};
+
+void arch_handle_ioreq(XenIOState *state, ioreq_t *req)
+{
+    hw_error("Invalid ioreq type 0x%x\n", req->type);
+
+    return;
+}
+
+void arch_xen_set_memory(XenIOState *state, MemoryRegionSection *section,
+                         bool add)
+{
+}
+
+void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
+{
+}
+
+void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
+{
+}
+
+static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
+{
+    xen_dmod = xendevicemodel_open(0, 0);
+    xen_xc = xc_interface_open(0, 0, 0);
+
+    if (xen_xc == NULL) {
+        perror("xen: can't open xen interface\n");
+        return -1;
+    }
+
+    xen_fmem = xenforeignmemory_open(0, 0);
+    if (xen_fmem == NULL) {
+        perror("xen: can't open xen fmem interface\n");
+        xc_interface_close(xen_xc);
+        return -1;
+    }
+
+    xen_register_ioreq(state, max_cpus, xen_memory_listener);
+
+    xenstore_record_dm_state(xenstore, "running");
+
+    return 0;
+}
+
+static void xen_enable_tpm(void)
+{
+/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
+#ifdef CONFIG_TPM
+    Error *errp = NULL;
+    DeviceState *dev;
+    SysBusDevice *busdev;
+
+    TPMBackend *be = qemu_find_tpm_be("tpm0");
+    if (be == NULL) {
+        DPRINTF("Couldn't fine the backend for tpm0\n");
+        return;
+    }
+    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
+    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
+    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(busdev, &error_fatal);
+    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
+
+    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
+#endif
+}
+
+static void xen_arm_init(MachineState *machine)
+{
+    XenArmState *xam = XEN_ARM(machine);
+
+    xam->state =  g_new0(XenIOState, 1);
+
+    if (xen_init_ioreq(xam->state, machine->smp.cpus)) {
+        return;
+    }
+
+    xen_enable_tpm();
+
+    return;
+}
+
+static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
+{
+
+    MachineClass *mc = MACHINE_CLASS(oc);
+    mc->desc = "Xen Para-virtualized PC";
+    mc->init = xen_arm_init;
+    mc->max_cpus = 1;
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+}
+
+static const TypeInfo xen_arm_machine_type = {
+    .name = TYPE_XEN_ARM,
+    .parent = TYPE_MACHINE,
+    .class_init = xen_arm_machine_class_init,
+    .instance_size = sizeof(XenArmState),
+};
+
+static void xen_arm_machine_register_types(void)
+{
+    type_register_static(&xen_arm_machine_type);
+}
+
+type_init(xen_arm_machine_register_types)
diff --git a/include/hw/arm/xen_arch_hvm.h b/include/hw/arm/xen_arch_hvm.h
new file mode 100644
index 0000000000..f645dfec28
--- /dev/null
+++ b/include/hw/arm/xen_arch_hvm.h
@@ -0,0 +1,12 @@
+#ifndef HW_XEN_ARCH_ARM_HVM_H
+#define HW_XEN_ARCH_ARM_HVM_H
+
+#include <xen/hvm/ioreq.h>
+void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
+void arch_xen_set_memory(XenIOState *state,
+                         MemoryRegionSection *section,
+                         bool add);
+
+#undef TARGET_PAGE_SIZE
+#define TARGET_PAGE_SIZE 4096
+#endif
diff --git a/include/hw/xen/arch_hvm.h b/include/hw/xen/arch_hvm.h
index 26674648d8..c7c515220d 100644
--- a/include/hw/xen/arch_hvm.h
+++ b/include/hw/xen/arch_hvm.h
@@ -1,3 +1,5 @@
 #if defined(TARGET_I386) || defined(TARGET_X86_64)
 #include "hw/i386/xen_arch_hvm.h"
+#elif defined(TARGET_ARM) || defined(TARGET_ARM_64)
+#include "hw/arm/xen_arch_hvm.h"
 #endif
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (9 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-27  9:11   ` Alex Bennée
  2022-10-15  5:07 ` [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets Vikram Garhwal
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: vikram.garhwal, stefano.stabellini

Add CONFIG_XEN for aarch64 device to support build for ARM targets.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index b686dfef75..0027d7d195 100644
--- a/meson.build
+++ b/meson.build
@@ -125,7 +125,7 @@ endif
 if cpu in ['x86', 'x86_64', 'arm', 'aarch64']
   # i386 emulator provides xenpv machine type for multiple architectures
   accelerator_targets += {
-    'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu'],
+    'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu', 'aarch64-softmmu'],
   }
 endif
 if cpu in ['x86', 'x86_64']
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (10 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM Vikram Garhwal
@ 2022-10-15  5:07 ` Vikram Garhwal
  2022-10-27  9:14   ` Alex Bennée
  2022-10-27  8:26 ` [PATCH v1 00/12] Introduce xenpv machine for arm architecture Alex Bennée
  2022-10-27  9:24 ` Alex Bennée
  13 siblings, 1 reply; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-15  5:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: vikram.garhwal, stefano.stabellini

From: Stefano Stabellini <stefano.stabellini@amd.com>

have_xen_pci_passthrough is only used for Xen x86 VMs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 meson.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meson.build b/meson.build
index 0027d7d195..43e70936ee 100644
--- a/meson.build
+++ b/meson.build
@@ -1454,6 +1454,8 @@ have_xen_pci_passthrough = get_option('xen_pci_passthrough') \
            error_message: 'Xen PCI passthrough requested but Xen not enabled') \
   .require(targetos == 'linux',
            error_message: 'Xen PCI passthrough not available on this platform') \
+  .require(cpu == 'x86'  or cpu == 'x86_64',
+           error_message: 'Xen PCI passthrough not available on this platform') \
   .allowed()
 
 
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
@ 2022-10-16 17:47   ` Julien Grall
  2022-10-18  1:26     ` Stefano Stabellini
                       ` (2 more replies)
  2022-10-27  8:02   ` Alex Bennée
  2022-10-27  8:40   ` Alex Bennée
  2 siblings, 3 replies; 50+ messages in thread
From: Julien Grall @ 2022-10-16 17:47 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Peter Maydell, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:ARM TCG CPUs,
	open list:X86 Xen CPUs

Hi,

There seem to be some missing patches on xen-devel (including the cover 
letter). Is that expected?

On 15/10/2022 06:07, Vikram Garhwal wrote:
> Add a new machine xenpv which creates a IOREQ server to register/connect with
> Xen Hypervisor.

I don't like the name 'xenpv' because it doesn't convey the fact that 
some of the HW may be emulated rather than para-virtualized. In fact one 
may only want to use for emulating devices.

Potential name would be 'xen-arm' or re-using 'virt' but with 
'accel=xen' to select a Xen layout.

> 
> Xen IOREQ connection expect the TARGET_PAGE_SIZE to 4096, and the xenpv machine
> on ARM will have no CPU definitions. We need to define TARGET_PAGE_SIZE
> appropriately ourselves.
> 
> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
> TPM emulator and connects to swtpm running on host machine via chardev socket
> and support TPM functionalities for a guest domain.
> 
> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
> 
> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
> provides access to TPM functionality over socket, chardev and CUSE interface.
> Github repo: https://github.com/stefanberger/swtpm
> Example for starting swtpm on host machine:
>      mkdir /tmp/vtpm2
>      swtpm socket --tpmstate dir=/tmp/vtpm2 \
>      --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &

I see patches for QEMU but not Xen. How can this be tested with existing 
Xen? Will libxl ever create QEMU?

[...]

> +static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
> +{
> +    xen_dmod = xendevicemodel_open(0, 0);
> +    xen_xc = xc_interface_open(0, 0, 0);
> +
> +    if (xen_xc == NULL) {

You are checking xen_xc but not xen_dmod. Why?

> +        perror("xen: can't open xen interface\n");
> +        return -1;
> +    }
> +
> +    xen_fmem = xenforeignmemory_open(0, 0);
> +    if (xen_fmem == NULL) {
> +        perror("xen: can't open xen fmem interface\n");
> +        xc_interface_close(xen_xc);
> +        return -1;
> +    }
> +
> +    xen_register_ioreq(state, max_cpus, xen_memory_listener);
> +
> +    xenstore_record_dm_state(xenstore, "running");
> +
> +    return 0;
> +}
> +
> +static void xen_enable_tpm(void)
> +{
> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> +#ifdef CONFIG_TPM
> +    Error *errp = NULL;
> +    DeviceState *dev;
> +    SysBusDevice *busdev;
> +
> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
> +    if (be == NULL) {
> +        DPRINTF("Couldn't fine the backend for tpm0\n");
> +        return;
> +    }
> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);

I can't find where GUEST_TPM_BASE is defined. But then the guest memory 
layout is not expected to be stable. With your current approach, it 
means QEMU would need to be rebuilt for every Xen version. Is it what we 
want?

> +
> +    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
> +#endif
> +}
> +
> +static void xen_arm_init(MachineState *machine)
> +{
> +    XenArmState *xam = XEN_ARM(machine);
> +
> +    xam->state =  g_new0(XenIOState, 1);
> +
> +    if (xen_init_ioreq(xam->state, machine->smp.cpus)) {
> +        return;

In another patch, you said the IOREQ would be optional. IHMO, I think 
this is a bad idea to register it by default because one may only want 
to use PV drivers. Registering IOREQ will add unnecessary overhead in Xen.

Furthermore, it means that someone selecting TPM but Xen is not built 
with CONFIG_IOREQ=y (BTW This is still a tech preview but there are 
security holes on Arm...) will not get an error. Instead, the OS will 
until it crashes when trying to access the TPM.

Overall I think it would be better if IOREQ is only registered when a 
device requires (like TPM) it *and* throw an error if there is a problem 
during the initialization.

> +    } > +
> +    xen_enable_tpm();
> +
> +    return;
> +}
> +
> +static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
> +{
> +
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    mc->desc = "Xen Para-virtualized PC";
> +    mc->init = xen_arm_init;
> +    mc->max_cpus = 1;
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);

Shouldn't this be protected with #ifdef CONFIG_TPM?

> +}
> +
> +static const TypeInfo xen_arm_machine_type = {
> +    .name = TYPE_XEN_ARM,
> +    .parent = TYPE_MACHINE,
> +    .class_init = xen_arm_machine_class_init,
> +    .instance_size = sizeof(XenArmState),
> +};
> +
> +static void xen_arm_machine_register_types(void)
> +{
> +    type_register_static(&xen_arm_machine_type);
> +}
> +
> +type_init(xen_arm_machine_register_types)
> diff --git a/include/hw/arm/xen_arch_hvm.h b/include/hw/arm/xen_arch_hvm.h
> new file mode 100644
> index 0000000000..f645dfec28
> --- /dev/null
> +++ b/include/hw/arm/xen_arch_hvm.h
> @@ -0,0 +1,12 @@
> +#ifndef HW_XEN_ARCH_ARM_HVM_H
> +#define HW_XEN_ARCH_ARM_HVM_H
> +
> +#include <xen/hvm/ioreq.h>
> +void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
> +void arch_xen_set_memory(XenIOState *state,
> +                         MemoryRegionSection *section,
> +                         bool add);
> +
> +#undef TARGET_PAGE_SIZE

I am a bit puzzled with this #undef. In the commit message you said that 
there will be no CPU definition. So the implications is this should not 
be defined.

If it is defined, then what guarantees that all the source will use the 
correct value?


> +#define TARGET_PAGE_SIZE 4096

It would be better to use XC_PAGE_SIZE (or similar) rather than 
hardcoding it.

> +#endif
> diff --git a/include/hw/xen/arch_hvm.h b/include/hw/xen/arch_hvm.h
> index 26674648d8..c7c515220d 100644
> --- a/include/hw/xen/arch_hvm.h
> +++ b/include/hw/xen/arch_hvm.h
> @@ -1,3 +1,5 @@
>   #if defined(TARGET_I386) || defined(TARGET_X86_64)
>   #include "hw/i386/xen_arch_hvm.h"
> +#elif defined(TARGET_ARM) || defined(TARGET_ARM_64)
> +#include "hw/arm/xen_arch_hvm.h"
>   #endif

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server
  2022-10-15  5:07 ` [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server Vikram Garhwal
@ 2022-10-16 17:53   ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2022-10-16 17:53 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Paul Durrant, open list:X86 Xen CPUs

Hi Vikram,

On 15/10/2022 06:07, Vikram Garhwal wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> This is done to prepare for enabling xenpv support for ARM architecture.
> On ARM it is possible to have a functioning xenpv machine with only the
> PV backends and no IOREQ server. If the IOREQ server creation fails,
> continue to the PV backends initialization.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>   include/hw/xen/xen_common.h | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 77ce17d8a4..c2d2f36bde 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -467,8 +467,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
>   {
>   }
>   
> -static inline void xen_create_ioreq_server(domid_t dom,
> -                                           ioservid_t *ioservid)
> +static inline int xen_create_ioreq_server(domid_t dom,
> +                                          ioservid_t *ioservid)
>   {

I think there is a return missing here.

>   }
>   
> @@ -600,8 +600,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
>                                                     PCI_FUNC(pci_dev->devfn));
>   }
>   
> -static inline void xen_create_ioreq_server(domid_t dom,
> -                                           ioservid_t *ioservid)
> +static inline int xen_create_ioreq_server(domid_t dom,
> +                                          ioservid_t *ioservid)
>   {
>       int rc = xendevicemodel_create_ioreq_server(xen_dmod, dom,
>                                                   HVM_IOREQSRV_BUFIOREQ_ATOMIC,
> @@ -609,12 +609,14 @@ static inline void xen_create_ioreq_server(domid_t dom,
>   
>       if (rc == 0) {
>           trace_xen_ioreq_server_create(*ioservid);
> -        return;
> +        return rc;
>       }
>   
>       *ioservid = 0;
>       use_default_ioreq_server = true;
>       trace_xen_default_ioreq_server();
> +
> +    return rc;
>   }
>   
>   static inline void xen_destroy_ioreq_server(domid_t dom,

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c
  2022-10-15  5:07 ` [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c Vikram Garhwal
@ 2022-10-16 18:07   ` Julien Grall
  2022-10-19 16:16   ` Paul Durrant
  1 sibling, 0 replies; 50+ messages in thread
From: Julien Grall @ 2022-10-16 18:07 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Paul Durrant, Michael S. Tsirkin, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	open list:X86 Xen CPUs

Hi Vikram,

On 15/10/2022 06:07, Vikram Garhwal wrote:
> +void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
> +                        MemoryListener xen_memory_listener)
> +{


[...]

> +
> +    xen_bus_init();
> +
> +    /* Initialize backend core & drivers */
> +    if (xen_be_init() != 0) {
> +        error_report("xen backend core setup failed");
> +        goto err;
> +    }
> +    xen_be_register_common();

Calling xen_be_init() and xen_be_register_common() from 
xen_register_ioreq() sounds wrong to me. There are no dependency between 
the two. I think it would be better to create a new function to register 
backends.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-16 17:47   ` Julien Grall
@ 2022-10-18  1:26     ` Stefano Stabellini
  2022-10-18  8:24       ` Julien Grall
  2022-10-27  8:37     ` Alex Bennée
  2022-12-02  3:24     ` Garhwal, Vikram
  2 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2022-10-18  1:26 UTC (permalink / raw)
  To: Julien Grall
  Cc: Vikram Garhwal, qemu-devel, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, open list:X86 Xen CPUs

On Sun, 16 Oct 2022, Julien Grall wrote:
> Hi,
> 
> There seem to be some missing patches on xen-devel (including the cover
> letter). Is that expected?
> 
> On 15/10/2022 06:07, Vikram Garhwal wrote:
> > Add a new machine xenpv which creates a IOREQ server to register/connect
> > with
> > Xen Hypervisor.
> 
> I don't like the name 'xenpv' because it doesn't convey the fact that some of
> the HW may be emulated rather than para-virtualized. In fact one may only want
> to use for emulating devices.
> 
> Potential name would be 'xen-arm' or re-using 'virt' but with 'accel=xen' to
> select a Xen layout.

The benefit of 'xenpv' is that it doesn't require any changes to libxl.
It is even backward compatible so it could be used with an older version
of Xen/libxl. Backward compatibility aside, if we come up with a
different name then we'll need changes to libxl and to manage those
changes. For instance, if we use 'xen-arm' that would mean we would need
to handle per-arch QEMU machine names.


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-18  1:26     ` Stefano Stabellini
@ 2022-10-18  8:24       ` Julien Grall
  2022-10-19  0:15         ` Stefano Stabellini
  0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2022-10-18  8:24 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Vikram Garhwal, qemu-devel, stefano.stabellini, Peter Maydell,
	Anthony Perard, Paul Durrant, open list:ARM TCG CPUs,
	open list:X86 Xen CPUs

Hi Stefano,

On 18/10/2022 02:26, Stefano Stabellini wrote:
> On Sun, 16 Oct 2022, Julien Grall wrote:
>> Hi,
>>
>> There seem to be some missing patches on xen-devel (including the cover
>> letter). Is that expected?
>>
>> On 15/10/2022 06:07, Vikram Garhwal wrote:
>>> Add a new machine xenpv which creates a IOREQ server to register/connect
>>> with
>>> Xen Hypervisor.
>>
>> I don't like the name 'xenpv' because it doesn't convey the fact that some of
>> the HW may be emulated rather than para-virtualized. In fact one may only want
>> to use for emulating devices.
>>
>> Potential name would be 'xen-arm' or re-using 'virt' but with 'accel=xen' to
>> select a Xen layout.
> 
> The benefit of 'xenpv' is that it doesn't require any changes to libxl.

I am quite surprised. Looking at the code, it seems to work more by 
chance than it is intentional as the code is gated by 
libxl__need_xenpv_qemu(). So it would not start if there were no 
emulated devices.

> It is even backward compatible so it could be used with an older version
> of Xen/libxl.
We don't really gain much here. IOREQ is a tech preview and anyone that 
wants to try it should really use the latest Xen.

> Backward compatibility aside, if we come up with a
> different name then we'll need changes to libxl and to manage those
> changes. For instance, if we use 'xen-arm' that would mean we would need
> to handle per-arch QEMU machine names.

Right, so the main argument here is for simplicity in libxl
Looking at how 'xenpv' is built, this is really expected to deal with PV 
backend rather than emulated device. I do expect some changes as we go 
along to be able to add emulated device.

Furthermore, libxl is not the only toolstack out. So I am not convinced 
this is a good argument to keep the name the same.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-18  8:24       ` Julien Grall
@ 2022-10-19  0:15         ` Stefano Stabellini
  2022-10-19  9:49           ` Julien Grall
  0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2022-10-19  0:15 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, Vikram Garhwal, qemu-devel,
	stefano.stabellini, Peter Maydell, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, open list:X86 Xen CPUs

On Tue, 18 Oct 2022, Julien Grall wrote:
> On 18/10/2022 02:26, Stefano Stabellini wrote:
> > On Sun, 16 Oct 2022, Julien Grall wrote:
> > > Hi,
> > > 
> > > There seem to be some missing patches on xen-devel (including the cover
> > > letter). Is that expected?
> > > 
> > > On 15/10/2022 06:07, Vikram Garhwal wrote:
> > > > Add a new machine xenpv which creates a IOREQ server to register/connect
> > > > with
> > > > Xen Hypervisor.
> > > 
> > > I don't like the name 'xenpv' because it doesn't convey the fact that some
> > > of
> > > the HW may be emulated rather than para-virtualized. In fact one may only
> > > want
> > > to use for emulating devices.
> > > 
> > > Potential name would be 'xen-arm' or re-using 'virt' but with 'accel=xen'
> > > to
> > > select a Xen layout.
> > 
> > The benefit of 'xenpv' is that it doesn't require any changes to libxl.
> 
> I am quite surprised. Looking at the code, it seems to work more by chance
> than it is intentional as the code is gated by libxl__need_xenpv_qemu(). So it
> would not start if there were no emulated devices.
> 
> > It is even backward compatible so it could be used with an older version
> > of Xen/libxl.
> We don't really gain much here. IOREQ is a tech preview and anyone that wants
> to try it should really use the latest Xen.

I think that's fair.


> > Backward compatibility aside, if we come up with a
> > different name then we'll need changes to libxl and to manage those
> > changes. For instance, if we use 'xen-arm' that would mean we would need
> > to handle per-arch QEMU machine names.
> 
> Right, so the main argument here is for simplicity in libxl

Yeah


> Looking at how 'xenpv' is built, this is really expected to deal with PV
> backend rather than emulated device. I do expect some changes as we go along
> to be able to add emulated device.
> 
> Furthermore, libxl is not the only toolstack out. So I am not convinced this
> is a good argument to keep the name the same.

Let's think some more about the naming strategy. From a QEMU point of
view we could choose any name we like (Vikram please confirm), the issue
is really on the libxl side.

Today libxl understands two QEMU "machines": xenpv and xenfv
(pc,accel=xen is the same as xenfv, I'll use xenfv in this email for
simplicity).

xenpv is for PV guests and only provides PV backends, no emulation. It
is used on both ARM and x86.

xenfv is only used on x86, and it is for HVM guests, with a full set of
emulated hardware (PIIX3, etc.).

The purpose of this series is to introduce a QEMU machine that:
- works on ARM (but could maybe work on other archs as a stretch goal)
- provides PV backends
- no emulated devices by default
- also can emulate selected devices on request

Certainly it is not xenfv or pc,accel=xen because they would with more
emulation by default. This is more like "xenpvh": an extension to PV
with also the capability of emulating one device as requested. It is not
intended to emulate a full PC and doesn't do that by default like xenfv.

If/When x86 PVH gains the ability to use QEMU as IOREQ server, I would
imagine it would run a QEMU machine similar to this one.

This is also how I would imagine it would get integrated in libxl: as a
xenpv + individual emulated devices. Not as HVM for ARM. The other QEMU
command line arguments are inline with the xenpv command line arguments
rather than xenfv command line arguments. This is why the libxl changes
are small to zero to make it work today.

So, I think the following options work:

a) call it "xenpv" because it is an extension of the old xenpv machine
b) call it "xenpvh" because it is PV + few individual emulated devices

If we call it xenpv there are fewer changes in libxl. If we call it
xenpvh there are more changes in libxl but we can distinguish xenpv from
xenpvh (I don't see why we need it right now, but I could imagine it
could turn out useful in the future.)

I would stay away from arch-specific machine names because it will make
it harder on the libxl side without immediate benefits.


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-19  0:15         ` Stefano Stabellini
@ 2022-10-19  9:49           ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2022-10-19  9:49 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Vikram Garhwal, qemu-devel, stefano.stabellini, Peter Maydell,
	Anthony Perard, Paul Durrant, open list:ARM TCG CPUs,
	open list:X86 Xen CPUs

Hi Stefano,

On 19/10/2022 01:15, Stefano Stabellini wrote:
> On Tue, 18 Oct 2022, Julien Grall wrote:
>> On 18/10/2022 02:26, Stefano Stabellini wrote:
>>> On Sun, 16 Oct 2022, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> There seem to be some missing patches on xen-devel (including the cover
>>>> letter). Is that expected?
>>>>
>>>> On 15/10/2022 06:07, Vikram Garhwal wrote:
>>>>> Add a new machine xenpv which creates a IOREQ server to register/connect
>>>>> with
>>>>> Xen Hypervisor.
>>>>
>>>> I don't like the name 'xenpv' because it doesn't convey the fact that some
>>>> of
>>>> the HW may be emulated rather than para-virtualized. In fact one may only
>>>> want
>>>> to use for emulating devices.
>>>>
>>>> Potential name would be 'xen-arm' or re-using 'virt' but with 'accel=xen'
>>>> to
>>>> select a Xen layout.
>>>
>>> The benefit of 'xenpv' is that it doesn't require any changes to libxl.
>>
>> I am quite surprised. Looking at the code, it seems to work more by chance
>> than it is intentional as the code is gated by libxl__need_xenpv_qemu(). So it
>> would not start if there were no emulated devices.
>>
>>> It is even backward compatible so it could be used with an older version
>>> of Xen/libxl.
>> We don't really gain much here. IOREQ is a tech preview and anyone that wants
>> to try it should really use the latest Xen.
> 
> I think that's fair.
> 
> 
>>> Backward compatibility aside, if we come up with a
>>> different name then we'll need changes to libxl and to manage those
>>> changes. For instance, if we use 'xen-arm' that would mean we would need
>>> to handle per-arch QEMU machine names.
>>
>> Right, so the main argument here is for simplicity in libxl
> 
> Yeah
> 
> 
>> Looking at how 'xenpv' is built, this is really expected to deal with PV
>> backend rather than emulated device. I do expect some changes as we go along
>> to be able to add emulated device.
>>
>> Furthermore, libxl is not the only toolstack out. So I am not convinced this
>> is a good argument to keep the name the same.
> 
> Let's think some more about the naming strategy. From a QEMU point of
> view we could choose any name we like (Vikram please confirm), the issue
> is really on the libxl side.
> 
> Today libxl understands two QEMU "machines": xenpv and xenfv
> (pc,accel=xen is the same as xenfv, I'll use xenfv in this email for
> simplicity).
> 
> xenpv is for PV guests and only provides PV backends, no emulation. It
> is used on both ARM and x86.
> 
> xenfv is only used on x86, and it is for HVM guests, with a full set of
> emulated hardware (PIIX3, etc.).
> 
> The purpose of this series is to introduce a QEMU machine that:
> - works on ARM (but could maybe work on other archs as a stretch goal)
> - provides PV backends
> - no emulated devices by default
> - also can emulate selected devices on request
> 
> Certainly it is not xenfv or pc,accel=xen because they would with more
> emulation by default. This is more like "xenpvh": an extension to PV
> with also the capability of emulating one device as requested. It is not
> intended to emulate a full PC and doesn't do that by default like xenfv.

The definition of "full PC" is not very clear for me. Unlike x86, Arm 
doesn't have legacy devices that needs to be emulated. So technically, 
if we emulated one network card and one block device, then we would be 
able potentially be able to boot an unaware OS on Xen on Arm. This would 
be the same as if you passthrough-ed the two devices.

In the past, I have seen interest to boot OS like Windows OS/iOS on Arm. 
I do expect that the addition of a Xen platform in QEMU will lead to 
another increase of the interest because we could expose anything to the 
VM. Although, it might need some tweak in Xen to allow more dynamic 
layout just in case an OS doesn't discover dynamically devices.

> 
> If/When x86 PVH gains the ability to use QEMU as IOREQ server, I would
> imagine it would run a QEMU machine similar to this one.
To me it would sounds odd to add emulated devices in the 'xenpv' because 
they would only work for PVH domain. AFAIK, QEMU doesn't know whether a 
domain is PV or PVH. So we would solely rely on IOREQ to return an error.

> 
> This is also how I would imagine it would get integrated in libxl: as a
> xenpv + individual emulated devices. Not as HVM for ARM. The other QEMU
> command line arguments are inline with the xenpv command line arguments
> rather than xenfv command line arguments. This is why the libxl changes
> are small to zero to make it work today.
> 
> So, I think the following options work:
> 
> a) call it "xenpv" because it is an extension of the old xenpv machine
> b) call it "xenpvh" because it is PV + few individual emulated devices
> 
> If we call it xenpv there are fewer changes in libxl. If we call it
> xenpvh there are more changes in libxl but we can distinguish xenpv from
> xenpvh (I don't see why we need it right now, but I could imagine it
> could turn out useful in the future.)

IMHO, we need to plan for the future.

> 
> I would stay away from arch-specific machine names because it will make
> it harder on the libxl side without immediate benefits.

If the name is the only change, then I would expect this could be done 
with a per-arch define. So that would be a few lines change maximum.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub
  2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
@ 2022-10-19 14:46   ` Paul Durrant
  2022-10-26 19:01   ` Alex Bennée
  1 sibling, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 14:46 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> Build fails when have_xen_pci_passthrough is disabled. This is because of
> incorrect build configuration for xen_pt_stub.c.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/
  2022-10-15  5:07 ` [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/ Vikram Garhwal
@ 2022-10-19 14:53   ` Paul Durrant
  2022-12-01 20:25     ` Garhwal, Vikram
  0 siblings, 1 reply; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 14:53 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Michael S. Tsirkin, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Stefano Stabellini, Anthony Perard, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> xen-mapcache.c contains common functions which can be used for enabling Xen on
> aarch64 with IOREQ handling. Moving it out from hw/i386/xen to hw/xen to make it
> accessible for both aarch64 and x86.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>   hw/i386/meson.build              | 1 +
>   hw/i386/xen/meson.build          | 1 -
>   hw/i386/xen/trace-events         | 5 -----
>   hw/xen/meson.build               | 4 ++++
>   hw/xen/trace-events              | 5 +++++
>   hw/{i386 => }/xen/xen-mapcache.c | 0
>   6 files changed, 10 insertions(+), 6 deletions(-)
>   rename hw/{i386 => }/xen/xen-mapcache.c (100%)
> 
> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
> index 213e2e82b3..cfdbfdcbcb 100644
> --- a/hw/i386/meson.build
> +++ b/hw/i386/meson.build
> @@ -33,5 +33,6 @@ subdir('kvm')
>   subdir('xen')
>   
>   i386_ss.add_all(xenpv_ss)
> +i386_ss.add_all(xen_ss)
>   
>   hw_arch += {'i386': i386_ss}
> diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
> index be84130300..2fcc46e6ca 100644
> --- a/hw/i386/xen/meson.build
> +++ b/hw/i386/xen/meson.build
> @@ -1,6 +1,5 @@
>   i386_ss.add(when: 'CONFIG_XEN', if_true: files(
>     'xen-hvm.c',
> -  'xen-mapcache.c',
>     'xen_apic.c',
>     'xen_platform.c',
>     'xen_pvdevice.c',
> diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
> index 5d6be61090..a0c89d91c4 100644
> --- a/hw/i386/xen/trace-events
> +++ b/hw/i386/xen/trace-events
> @@ -21,8 +21,3 @@ xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
>   cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
>   cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
>   
> -# xen-mapcache.c
> -xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
> -xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
> -xen_map_cache_return(void* ptr) "%p"
> -
> diff --git a/hw/xen/meson.build b/hw/xen/meson.build
> index ae0ace3046..19d0637c46 100644
> --- a/hw/xen/meson.build
> +++ b/hw/xen/meson.build
> @@ -22,3 +22,7 @@ else
>   endif
>   
>   specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
> +
> +xen_ss = ss.source_set()
> +
> +xen_ss.add(when: 'CONFIG_XEN', if_true: files('xen-mapcache.c'))

Curious as to why you couldn't just add this to the softmmu_ss list above?

   Paul

> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index 3da3fd8348..2c8f238f42 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -41,3 +41,8 @@ xs_node_vprintf(char *path, char *value) "%s %s"
>   xs_node_vscanf(char *path, char *value) "%s %s"
>   xs_node_watch(char *path) "%s"
>   xs_node_unwatch(char *path) "%s"
> +
> +# xen-mapcache.c
> +xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
> +xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
> +xen_map_cache_return(void* ptr) "%p"
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> similarity index 100%
> rename from hw/i386/xen/xen-mapcache.c
> rename to hw/xen/xen-mapcache.c



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc
  2022-10-15  5:07 ` [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc Vikram Garhwal
@ 2022-10-19 14:59   ` Paul Durrant
  0 siblings, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 14:59 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Michael S. Tsirkin, Marcel Apfelbaum, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> In preparation to moving most of xen-hvm code to an arch-neutral location,
> move non IOREQ references to:
> - xen_get_vmport_regs_pfn
> - xen_suspend_notifier
> - xen_wakeup_notifier
> - xen_ram_init
> 
> towards the end of the xen_hvm_init_pc() function.
> 
> This is done to keep the common ioreq functions in one place which will be
> moved to new function in next patch in order to make it common to both x86 and
> aarch64 machines.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
@ 2022-10-19 15:56   ` Paul Durrant
  2022-10-19 16:00   ` Paul Durrant
  2022-10-27  8:59   ` Alex Bennée
  2 siblings, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 15:56 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
	Richard Henderson, Eduardo Habkost, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> In preparation to moving most of xen-hvm code to an arch-neutral location, move:
> - shared_vmport_page
> - log_for_dirtybit
> - dirty_bitmap
> - suspend
> - wakeup
> 
> out of XenIOState struct as these are only used on x86, especially the ones
> related to dirty logging.
> Updated XenIOState can be used for both aarch64 and x86.
> 
> Also, remove free_phys_offset as it was unused.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>




^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
  2022-10-19 15:56   ` Paul Durrant
@ 2022-10-19 16:00   ` Paul Durrant
  2022-10-27  8:59   ` Alex Bennée
  2 siblings, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 16:00 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
	Richard Henderson, Eduardo Habkost, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> In preparation to moving most of xen-hvm code to an arch-neutral location, move:
> - shared_vmport_page
> - log_for_dirtybit
> - dirty_bitmap
> - suspend
> - wakeup
> 
> out of XenIOState struct as these are only used on x86, especially the ones
> related to dirty logging.
> Updated XenIOState can be used for both aarch64 and x86.
> 
> Also, remove free_phys_offset as it was unused.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory
  2022-10-15  5:07 ` [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory Vikram Garhwal
@ 2022-10-19 16:09   ` Paul Durrant
  2022-10-27  9:02   ` Alex Bennée
  1 sibling, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 16:09 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
	Richard Henderson, Eduardo Habkost, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> In preparation to moving most of xen-hvm code to an arch-neutral location,
> move the x86-specific portion of xen_set_memory to arch_xen_set_memory.
> 
> Also move handle_vmport_ioreq to arch_handle_ioreq.
> 
> NOTE: This patch breaks the build. Next patch fixes the build issue.
> Reason behind creating this patch is because there is lot of new code addition
> and pure code movement done for enabling Xen on ARM. Keeping the this patch
> separate is done to make it easier to review.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c
  2022-10-15  5:07 ` [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c Vikram Garhwal
  2022-10-16 18:07   ` Julien Grall
@ 2022-10-19 16:16   ` Paul Durrant
  2022-10-19 23:12     ` Garhwal, Vikram
  1 sibling, 1 reply; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 16:16 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
	Richard Henderson, Eduardo Habkost, open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
[snip]
> +    qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
> +
> +    state->memory_listener = xen_memory_listener;
> +    memory_listener_register(&state->memory_listener, &address_space_memory);
> +
> +    state->io_listener = xen_io_listener;
> +    memory_listener_register(&state->io_listener, &address_space_io);
> +
> +    state->device_listener = xen_device_listener;
> +    QLIST_INIT(&state->dev_list);
> +    device_listener_register(&state->device_listener);
> +

As Julien said, these do not belong here. These are the (current and 
legacy) PV backend setup functions; they most certainly have nothing to 
do with device emulation.

   Paul



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-15  5:07 ` [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state Vikram Garhwal
@ 2022-10-19 16:20   ` Paul Durrant
  2022-10-27  9:14   ` Alex Bennée
  1 sibling, 0 replies; 50+ messages in thread
From: Paul Durrant @ 2022-10-19 16:20 UTC (permalink / raw)
  To: Vikram Garhwal, qemu-devel
  Cc: stefano.stabellini, Stefano Stabellini, Anthony Perard,
	open list:X86 Xen CPUs

On 15/10/2022 06:07, Vikram Garhwal wrote:
> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

Reviewed-by: Paul Durrant <paul@xen.org>



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c
  2022-10-19 16:16   ` Paul Durrant
@ 2022-10-19 23:12     ` Garhwal, Vikram
  0 siblings, 0 replies; 50+ messages in thread
From: Garhwal, Vikram @ 2022-10-19 23:12 UTC (permalink / raw)
  To: Paul Durrant, qemu-devel
  Cc: Stabellini, Stefano, Stefano Stabellini, Anthony Perard,
	Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
	Richard Henderson, Eduardo Habkost, open list:X86 Xen CPUs

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Thanks Paul & Julien for reviewing. I will update this in v2.

Regards,
Vikram

From: Paul Durrant <xadimgnik@gmail.com>
Date: Wednesday, October 19, 2022 at 9:16 AM
To: Garhwal, Vikram <vikram.garhwal@amd.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: Stabellini, Stefano <stefano.stabellini@amd.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Michael S. Tsirkin <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, open list:X86 Xen CPUs <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c
On 15/10/2022 06:07, Vikram Garhwal wrote:
[snip]
> +    qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
> +
> +    state->memory_listener = xen_memory_listener;
> +    memory_listener_register(&state->memory_listener, &address_space_memory);
> +
> +    state->io_listener = xen_io_listener;
> +    memory_listener_register(&state->io_listener, &address_space_io);
> +
> +    state->device_listener = xen_device_listener;
> +    QLIST_INIT(&state->dev_list);
> +    device_listener_register(&state->device_listener);
> +

As Julien said, these do not belong here. These are the (current and
legacy) PV backend setup functions; they most certainly have nothing to
do with device emulation.

   Paul

[-- Attachment #2: Type: text/html, Size: 3930 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub
  2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
  2022-10-19 14:46   ` Paul Durrant
@ 2022-10-26 19:01   ` Alex Bennée
  1 sibling, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-26 19:01 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Build fails when have_xen_pci_passthrough is disabled. This is because of
> incorrect build configuration for xen_pt_stub.c.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

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

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
  2022-10-16 17:47   ` Julien Grall
@ 2022-10-27  8:02   ` Alex Bennée
  2022-10-28 17:57     ` Julien Grall
  2022-10-27  8:40   ` Alex Bennée
  2 siblings, 1 reply; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:02 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

<snip>
> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
> TPM emulator and connects to swtpm running on host machine via chardev socket
> and support TPM functionalities for a guest domain.
>
> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
>     -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>     -tpmdev emulator,id=tpm0,chardev=chrtpm \
>
> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
> provides access to TPM functionality over socket, chardev and CUSE interface.
> Github repo: https://github.com/stefanberger/swtpm
> Example for starting swtpm on host machine:
>     mkdir /tmp/vtpm2
>     swtpm socket --tpmstate dir=/tmp/vtpm2 \
>     --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &

<snip>
> +static void xen_enable_tpm(void)
> +{
> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> +#ifdef CONFIG_TPM
> +    Error *errp = NULL;
> +    DeviceState *dev;
> +    SysBusDevice *busdev;
> +
> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
> +    if (be == NULL) {
> +        DPRINTF("Couldn't fine the backend for tpm0\n");
> +        return;
> +    }
> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);

I'm not sure what has gone wrong here but I'm getting:

  ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
  ../../hw/arm/xen_arm.c:120:32: error: ‘GUEST_TPM_BASE’ undeclared (first use in this function); did you mean ‘GUEST_RAM_BASE’?
    120 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
        |                                ^~~~~~~~~~~~~~
        |                                GUEST_RAM_BASE
  ../../hw/arm/xen_arm.c:120:32: note: each undeclared identifier is reported only once for each function it appears in

In my cross build:

  # Configured with: '../../configure' '--disable-docs' '--target-list=aarch64-softmmu' '--disable-kvm' '--enable-xen' '--disable-opengl' '--disable-libudev' '--enable-tpm' '--disable-xen-pci-passthrough' '--cross-prefix=aarch64-linux-gnu-' '--skip-meson'

which makes me wonder if this is a configure failure or a confusion
about being able to have host swtpm implementations during emulation but
needing target tpm for Xen?

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 00/12] Introduce xenpv machine for arm architecture
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (11 preceding siblings ...)
  2022-10-15  5:07 ` [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets Vikram Garhwal
@ 2022-10-27  8:26 ` Alex Bennée
  2022-10-27  9:24 ` Alex Bennée
  13 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:26 UTC (permalink / raw)
  To: Vikram Garhwal; +Cc: stefano.stabellini, qemu-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Hi,
> This series add xenpv machine for aarch64. Motivation behind creating xenpv
> machine with IOREQ and TPM was to enable each guest on Xen aarch64 to have it's
> own unique and emulated TPM.
>
> This series does following:
>     1. Moved common xen functionalities from hw/i386/xen to hw/xen/ so those can
>        be used for aarch64.
>     2. We added a minimal xenpv arm machine which creates an IOREQ server and
>        support TPM.

Now I have some CI minutes again:

  https://gitlab.com/stsquad/qemu/-/pipelines/677956972/failures

which broadly break down into:

  * GUEST_TPM_BASE define missing
  * #include <xenstore.h> failure on builds that don't enable Xen
  * CPUTLBEntryFull f; breakage (tcg bits in a non-tcg build?)

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-16 17:47   ` Julien Grall
  2022-10-18  1:26     ` Stefano Stabellini
@ 2022-10-27  8:37     ` Alex Bennée
  2022-12-02  3:24     ` Garhwal, Vikram
  2 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:37 UTC (permalink / raw)
  To: Julien Grall
  Cc: Vikram Garhwal, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, open list:X86 Xen CPUs, qemu-devel


Julien Grall <julien@xen.org> writes:

> Hi,
>
> There seem to be some missing patches on xen-devel (including the
> cover letter). Is that expected?
>
> On 15/10/2022 06:07, Vikram Garhwal wrote:
>> Add a new machine xenpv which creates a IOREQ server to register/connect with
>> Xen Hypervisor.
>
> I don't like the name 'xenpv' because it doesn't convey the fact that
> some of the HW may be emulated rather than para-virtualized. In fact
> one may only want to use for emulating devices.
>
> Potential name would be 'xen-arm' or re-using 'virt' but with
> 'accel=xen' to select a Xen layout.

I don't think you can re-use the machine name and select by accelerator
because the virt machine does quite a lot of other stuff this model
doesn't support. However I've been calling this concept "xen-virt" or
maybe the explicit "xen-virtio" because that is what it is targeting.

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
  2022-10-16 17:47   ` Julien Grall
  2022-10-27  8:02   ` Alex Bennée
@ 2022-10-27  8:40   ` Alex Bennée
  2 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:40 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Add a new machine xenpv which creates a IOREQ server to register/connect with
> Xen Hypervisor.
>
<snip>
> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
> TPM emulator and connects to swtpm running on host machine via chardev socket
> and support TPM functionalities for a guest domain.
<snip>
> +
> +static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
> +{
> +
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    mc->desc = "Xen Para-virtualized PC";
> +    mc->init = xen_arm_init;
> +    mc->max_cpus = 1;
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);

This needs #ifdef CONFIG_TPM because while doing --disable-tpm to try
and get the cross build working it then fails with:

../../hw/arm/xen_arm.c: In function ‘xen_arm_machine_class_init’:
../../hw/arm/xen_arm.c:148:48: error: ‘TYPE_TPM_TIS_SYSBUS’ undeclared (first use in this function)
  148 |     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
      |                                                ^~~~~~~~~~~~~~~~~~~
../../hw/arm/xen_arm.c:148:48: note: each undeclared identifier is reported only once for each function it appears in

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure
  2022-10-15  5:07 ` [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure Vikram Garhwal
@ 2022-10-27  8:46   ` Alex Bennée
  0 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:46 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> On ARM it is possible to have a functioning xenpv machine with only the
> PV backends and no IOREQ server. If the IOREQ server creation fails continue
> to the PV backends initialization.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  hw/xen/xen-hvm-common.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index f848f9e625..7bccf595fc 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -777,7 +777,11 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
>          goto err;
>      }
>  
> -    xen_create_ioreq_server(xen_domid, &state->ioservid);
> +    rc = xen_create_ioreq_server(xen_domid, &state->ioservid);
> +    if (rc) {
> +        DPRINTF("xen: failed to create ioreq server\n");

This should be a warn_report to properly inform the user.

> +        goto no_ioreq;

Maybe pushing the rest of this function into a local subroutine would
reduce the amount of goto messing about. Other candidates for cleaning
up/modernising:

  - g_malloc to g_new0
  - perror -> error_setg_errno

> +    }
>  
>      state->exit.notify = xen_exit_notifier;
>      qemu_add_exit_notifier(&state->exit);
> @@ -842,6 +846,7 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
>      QLIST_INIT(&state->dev_list);
>      device_listener_register(&state->device_listener);
>  
> +no_ioreq:
>      xen_bus_init();
>  
>      /* Initialize backend core & drivers */


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
  2022-10-19 15:56   ` Paul Durrant
  2022-10-19 16:00   ` Paul Durrant
@ 2022-10-27  8:59   ` Alex Bennée
  2 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  8:59 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> In preparation to moving most of xen-hvm code to an arch-neutral location, move:
> - shared_vmport_page
> - log_for_dirtybit
> - dirty_bitmap
> - suspend
> - wakeup
>
> out of XenIOState struct as these are only used on x86, especially the ones
> related to dirty logging.
> Updated XenIOState can be used for both aarch64 and x86.
>
> Also, remove free_phys_offset as it was unused.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

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

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory
  2022-10-15  5:07 ` [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory Vikram Garhwal
  2022-10-19 16:09   ` Paul Durrant
@ 2022-10-27  9:02   ` Alex Bennée
  1 sibling, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  9:02 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, Michael S. Tsirkin,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
	Eduardo Habkost, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> In preparation to moving most of xen-hvm code to an arch-neutral location,
> move the x86-specific portion of xen_set_memory to arch_xen_set_memory.
>
> Also move handle_vmport_ioreq to arch_handle_ioreq.
>
> NOTE: This patch breaks the build. Next patch fixes the build issue.
> Reason behind creating this patch is because there is lot of new code addition
> and pure code movement done for enabling Xen on ARM. Keeping the this patch
> separate is done to make it easier to review.

But you do intend to squash the patches for the final version? We don't
want to intentionally break bisection.

Otherwise:

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


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM
  2022-10-15  5:07 ` [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM Vikram Garhwal
@ 2022-10-27  9:11   ` Alex Bennée
  0 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  9:11 UTC (permalink / raw)
  To: Vikram Garhwal; +Cc: stefano.stabellini, qemu-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Add CONFIG_XEN for aarch64 device to support build for ARM targets.

So to be clear a --enable-xen only build for any of these binaries
essentially ends up being the same thing just with a slightly less
discombobulating name?

Maybe given there is no real architecture specific stuff we should just
create a neutral binary for --enable-xen (e.g. qemu-xen-backend)?

Anyway:

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


>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index b686dfef75..0027d7d195 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -125,7 +125,7 @@ endif
>  if cpu in ['x86', 'x86_64', 'arm', 'aarch64']
>    # i386 emulator provides xenpv machine type for multiple architectures
>    accelerator_targets += {
> -    'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu'],
> +    'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu', 'aarch64-softmmu'],
>    }
>  endif
>  if cpu in ['x86', 'x86_64']


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets
  2022-10-15  5:07 ` [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets Vikram Garhwal
@ 2022-10-27  9:14   ` Alex Bennée
  0 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  9:14 UTC (permalink / raw)
  To: Vikram Garhwal; +Cc: stefano.stabellini, qemu-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> have_xen_pci_passthrough is only used for Xen x86 VMs.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

I think this might want to before 11/12. Anyway:

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

> ---
>  meson.build | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index 0027d7d195..43e70936ee 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1454,6 +1454,8 @@ have_xen_pci_passthrough = get_option('xen_pci_passthrough') \
>             error_message: 'Xen PCI passthrough requested but Xen not enabled') \
>    .require(targetos == 'linux',
>             error_message: 'Xen PCI passthrough not available on this platform') \
> +  .require(cpu == 'x86'  or cpu == 'x86_64',
> +           error_message: 'Xen PCI passthrough not available on this platform') \
>    .allowed()


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-15  5:07 ` [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state Vikram Garhwal
  2022-10-19 16:20   ` Paul Durrant
@ 2022-10-27  9:14   ` Alex Bennée
  2022-10-28 20:26     ` Oleksandr Tyshchenko
  2022-10-29  5:22     ` Garhwal, Vikram
  1 sibling, 2 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  9:14 UTC (permalink / raw)
  To: Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  accel/xen/xen-all.c  | 2 +-
>  include/hw/xen/xen.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b..276625b78b 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>  }
>  
>  
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>  {
>      char path[50];
>  
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index afdf9c436a..31e9538a5c 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "exec/cpu-common.h"
> +#include <xenstore.h>

This is breaking a bunch of the builds and generally we try and avoid
adding system includes in headers (apart from osdep.h) for this reason.
In fact there is a comment just above to that fact.

I think you can just add struct xs_handle to typedefs.h (or maybe just
xen.h) and directly include xenstore.h in xen-all.c following the usual
rules:

  https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives

It might be worth doing an audit to see what else is including xen.h
needlessly or should be using sysemu/xen.h. 

>  
>  /* xen-machine.c */
>  enum xen_mode {
> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>  
>  void xen_register_framebuffer(struct MemoryRegion *mr);
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>  
>  #endif /* QEMU_HW_XEN_H */


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 00/12] Introduce xenpv machine for arm architecture
  2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
                   ` (12 preceding siblings ...)
  2022-10-27  8:26 ` [PATCH v1 00/12] Introduce xenpv machine for arm architecture Alex Bennée
@ 2022-10-27  9:24 ` Alex Bennée
  13 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-10-27  9:24 UTC (permalink / raw)
  To: Vikram Garhwal; +Cc: stefano.stabellini, qemu-devel


Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Hi,
> This series add xenpv machine for aarch64. Motivation behind creating xenpv
> machine with IOREQ and TPM was to enable each guest on Xen aarch64 to have it's
> own unique and emulated TPM.
>
> This series does following:
>     1. Moved common xen functionalities from hw/i386/xen to hw/xen/ so those can
>        be used for aarch64.
>     2. We added a minimal xenpv arm machine which creates an IOREQ server and
>        support TPM.
>
> Please note that patch 05/12 breaks the build. Patch 06/12 fixes the build
> issue. If needed we can merge patch 05/12 and 06/12. For now we kept these
> separate to make changes easy to review.
>
> Also, checkpatch.pl fails for 03/12 and 06/12. These fails are due to
> moving old code to new place which was not QEMU code style compatible.
> No new add code was added.

I've finished my review pass. Please CC me on v2 when it's ready ;-)

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-27  8:02   ` Alex Bennée
@ 2022-10-28 17:57     ` Julien Grall
  2022-10-28 19:05       ` Oleksandr Tyshchenko
  0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2022-10-28 17:57 UTC (permalink / raw)
  To: Alex Bennée, Vikram Garhwal
  Cc: qemu-devel, stefano.stabellini, Peter Maydell,
	Stefano Stabellini, Anthony Perard, Paul Durrant,
	open list:ARM TCG CPUs, xen-devel

Hi,

On 27/10/2022 09:02, Alex Bennée wrote:
> 
> Vikram Garhwal <vikram.garhwal@amd.com> writes:
> 
> <snip>
>> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
>> TPM emulator and connects to swtpm running on host machine via chardev socket
>> and support TPM functionalities for a guest domain.
>>
>> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
>>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>
>> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
>> provides access to TPM functionality over socket, chardev and CUSE interface.
>> Github repo: https://github.com/stefanberger/swtpm
>> Example for starting swtpm on host machine:
>>      mkdir /tmp/vtpm2
>>      swtpm socket --tpmstate dir=/tmp/vtpm2 \
>>      --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
> 
> <snip>
>> +static void xen_enable_tpm(void)
>> +{
>> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
>> +#ifdef CONFIG_TPM
>> +    Error *errp = NULL;
>> +    DeviceState *dev;
>> +    SysBusDevice *busdev;
>> +
>> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
>> +    if (be == NULL) {
>> +        DPRINTF("Couldn't fine the backend for tpm0\n");
>> +        return;
>> +    }
>> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
>> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
>> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(busdev, &error_fatal);
>> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
> 
> I'm not sure what has gone wrong here but I'm getting:
> 
>    ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
>    ../../hw/arm/xen_arm.c:120:32: error: ‘GUEST_TPM_BASE’ undeclared (first use in this function); did you mean ‘GUEST_RAM_BASE’?
>      120 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
>          |                                ^~~~~~~~~~~~~~
>          |                                GUEST_RAM_BASE
>    ../../hw/arm/xen_arm.c:120:32: note: each undeclared identifier is reported only once for each function it appears in
> 
> In my cross build:
> 
>    # Configured with: '../../configure' '--disable-docs' '--target-list=aarch64-softmmu' '--disable-kvm' '--enable-xen' '--disable-opengl' '--disable-libudev' '--enable-tpm' '--disable-xen-pci-passthrough' '--cross-prefix=aarch64-linux-gnu-' '--skip-meson'
> 
> which makes me wonder if this is a configure failure or a confusion
> about being able to have host swtpm implementations during emulation but
> needing target tpm for Xen?

I was also wondering where is that value come from. Note that the 
memory/IRQ layout exposed to the guest is not stable.

Are we expecting the user to rebuild QEMU for every Xen versions (or 
possibly every guest if we ever allow dynamic layout in Xen)?

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-28 17:57     ` Julien Grall
@ 2022-10-28 19:05       ` Oleksandr Tyshchenko
  0 siblings, 0 replies; 50+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-28 19:05 UTC (permalink / raw)
  To: Julien Grall, Vikram Garhwal, stefano.stabellini
  Cc: qemu-devel, Peter Maydell, Stefano Stabellini, Anthony Perard,
	Paul Durrant, open list:ARM TCG CPUs, xen-devel,
	Alex Bennée

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

On Fri, Oct 28, 2022 at 8:58 PM Julien Grall <julien@xen.org> wrote:

> Hi,
>

Hello all.

[sorry for the possible format issues]



>
> On 27/10/2022 09:02, Alex Bennée wrote:
> >
> > Vikram Garhwal <vikram.garhwal@amd.com> writes:
> >
> > <snip>
> >> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device,
> adds a
> >> TPM emulator and connects to swtpm running on host machine via chardev
> socket
> >> and support TPM functionalities for a guest domain.
> >>
> >> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
> >>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
> >>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
> >>
> >> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on
> libtpms and
> >> provides access to TPM functionality over socket, chardev and CUSE
> interface.
> >> Github repo: https://github.com/stefanberger/swtpm
> >> Example for starting swtpm on host machine:
> >>      mkdir /tmp/vtpm2
> >>      swtpm socket --tpmstate dir=/tmp/vtpm2 \
> >>      --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
> >
> > <snip>
> >> +static void xen_enable_tpm(void)
> >> +{
> >> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> >> +#ifdef CONFIG_TPM
> >> +    Error *errp = NULL;
> >> +    DeviceState *dev;
> >> +    SysBusDevice *busdev;
> >> +
> >> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
> >> +    if (be == NULL) {
> >> +        DPRINTF("Couldn't fine the backend for tpm0\n");
> >> +        return;
> >> +    }
> >> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> >> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> >> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> >> +    busdev = SYS_BUS_DEVICE(dev);
> >> +    sysbus_realize_and_unref(busdev, &error_fatal);
> >> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
> >
> > I'm not sure what has gone wrong here but I'm getting:
> >
> >    ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
> >    ../../hw/arm/xen_arm.c:120:32: error: ‘GUEST_TPM_BASE’ undeclared
> (first use in this function); did you mean ‘GUEST_RAM_BASE’?
> >      120 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
> >          |                                ^~~~~~~~~~~~~~
> >          |                                GUEST_RAM_BASE
> >    ../../hw/arm/xen_arm.c:120:32: note: each undeclared identifier is
> reported only once for each function it appears in
> >
> > In my cross build:
> >
> >    # Configured with: '../../configure' '--disable-docs'
> '--target-list=aarch64-softmmu' '--disable-kvm' '--enable-xen'
> '--disable-opengl' '--disable-libudev' '--enable-tpm'
> '--disable-xen-pci-passthrough' '--cross-prefix=aarch64-linux-gnu-'
> '--skip-meson'
> >
> > which makes me wonder if this is a configure failure or a confusion
> > about being able to have host swtpm implementations during emulation but
> > needing target tpm for Xen?
>
> I was also wondering where is that value come from. Note that the
> memory/IRQ layout exposed to the guest is not stable.
>
> Are we expecting the user to rebuild QEMU for every Xen versions (or
> possibly every guest if we ever allow dynamic layout in Xen)?
>


This doesn't sound ideal.

I am wondering what would be the correct way here assuming that we would
likely need to have more such information in place for supporting more
use-cases...
For instance, the PCI host bridge emulation in Qemu. Xen toolstack (another
software layer) generates device-tree for the guest, so creates PCI Host
bridge node by using reserved regions from Guest OS interface (arch-arm.h):
- GUEST_VPCI_MEM_ADDR (GUEST_VPCI_MEM_SIZE)
- GUEST_VPCI_ECAM_BASE (GUEST_VPCI_ECAM_SIZE)
- GUEST_VPCI_PREFETCH_MEM_ADDR (GUEST_VPCI_PREFETCH_MEM_SIZE)
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libs/light/libxl_arm.c;h=2a5e93c28403738779863aded31d2df3ba72f8c0;hb=HEAD#l833

Here in Qemu when creating a PCI Host bridge we would need to use exactly
the same reserved regions which toolstack writes in the corresponding
device-tree node. So how to tell Qemu about them?
1. Introduce new cmd line arguments?
2. Using Xenstore?
3. Anything else?

I am afraid this would be related to every device that we want to emulate
in Qemu and for which the toolstack needs to generate device-tree node by
using something defined with GUEST_*, unless I really missed something.



>
> Cheers,
>
> --
> Julien Grall
>
>

-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #2: Type: text/html, Size: 6869 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-27  9:14   ` Alex Bennée
@ 2022-10-28 20:26     ` Oleksandr Tyshchenko
  2022-10-29  5:36       ` Vikram Garhwal
  2022-10-29  5:22     ` Garhwal, Vikram
  1 sibling, 1 reply; 50+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-28 20:26 UTC (permalink / raw)
  To: Vikram Garhwal, stefano.stabellini
  Cc: qemu-devel, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Alex Bennée, xen-devel

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Thu, Oct 27, 2022 at 12:24 PM Alex Bennée <alex.bennee@linaro.org> wrote:

Hello all



> Vikram Garhwal <vikram.garhwal@amd.com> writes:
>
> > xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
> >
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >  accel/xen/xen-all.c  | 2 +-
> >  include/hw/xen/xen.h | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> > index 69aa7d018b..276625b78b 100644
> > --- a/accel/xen/xen-all.c
> > +++ b/accel/xen/xen-all.c
> > @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev
> *chr)
> >  }
> >
> >
> > -static void xenstore_record_dm_state(struct xs_handle *xs, const char
> *state)
> > +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> >  {
> >      char path[50];
> >
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index afdf9c436a..31e9538a5c 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -9,6 +9,7 @@
> >   */
> >
> >  #include "exec/cpu-common.h"
> > +#include <xenstore.h>
>
> This is breaking a bunch of the builds and generally we try and avoid
> adding system includes in headers (apart from osdep.h) for this reason.
> In fact there is a comment just above to that fact.
>
> I think you can just add struct xs_handle to typedefs.h (or maybe just
> xen.h) and directly include xenstore.h in xen-all.c following the usual
> rules:
>
>
> https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
> It might be worth doing an audit to see what else is including xen.h
> needlessly or should be using sysemu/xen.h.
>
> >
> >  /* xen-machine.c */
> >  enum xen_mode {
> > @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
> >  void xenstore_store_pv_console_info(int i, Chardev *chr);
> >
> >  void xen_register_framebuffer(struct MemoryRegion *mr);
> > +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
> >
> >  #endif /* QEMU_HW_XEN_H */
>
>
> --
> Alex Bennée
>
>

For considering:
I think this patch and some other changes done in "[PATCH v1 10/12] hw/arm:
introduce xenpv machine" (the opening of Xen interfaces and
calling xenstore_record_dm_state() in hw/arm/xen_arm.c:xen_init_ioreq())
could be avoided if we enable the Xen accelerator (either by passing "-M
xenpv,accel=xen" or by adding mc->default_machine_opts = "accel=xen";
to hw/arm/xen_arm.c:xen_arm_machine_class_init() or by some other method).
These actions are already done in accel/xen/xen-all.c:xen_init(). Please
note, that I am not too familiar with that code, so there might be nuances.

Besides that, Xen accelerator will be needed for the xen-mapcache to be in
use (this is needed for mapping guest memory), there are a few
xen_enabled() checks spreading around that code to perform Xen specific
actions.

-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #2: Type: text/html, Size: 4698 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-27  9:14   ` Alex Bennée
  2022-10-28 20:26     ` Oleksandr Tyshchenko
@ 2022-10-29  5:22     ` Garhwal, Vikram
  2022-11-01 11:29       ` Alex Bennée
  1 sibling, 1 reply; 50+ messages in thread
From: Garhwal, Vikram @ 2022-10-29  5:22 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, Stabellini, Stefano, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

Thanks, Alex, for reviewing this one. I built for all the archs and it was fine. Can you please share more about what environment builds are breaking? So, I can test the changes for v2.

Regards,
Vikram

From: Alex Bennée <alex.bennee@linaro.org>
Date: Thursday, October 27, 2022 at 2:24 AM
To: Garhwal, Vikram <vikram.garhwal@amd.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, Stabellini, Stefano <stefano.stabellini@amd.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state

Vikram Garhwal <vikram.garhwal@amd.com> writes:

> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  accel/xen/xen-all.c  | 2 +-
>  include/hw/xen/xen.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b..276625b78b 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>  }
>
>
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>  {
>      char path[50];
>
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index afdf9c436a..31e9538a5c 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -9,6 +9,7 @@
>   */
>
>  #include "exec/cpu-common.h"
> +#include <xenstore.h>

This is breaking a bunch of the builds and generally we try and avoid
adding system includes in headers (apart from osdep.h) for this reason.
In fact there is a comment just above to that fact.

I think you can just add struct xs_handle to typedefs.h (or maybe just
xen.h) and directly include xenstore.h in xen-all.c following the usual
rules:

  https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives

It might be worth doing an audit to see what else is including xen.h
needlessly or should be using sysemu/xen.h.

>
>  /* xen-machine.c */
>  enum xen_mode {
> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>
>  void xen_register_framebuffer(struct MemoryRegion *mr);
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>
>  #endif /* QEMU_HW_XEN_H */


--
Alex Bennée

[-- Attachment #2: Type: text/html, Size: 5478 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-28 20:26     ` Oleksandr Tyshchenko
@ 2022-10-29  5:36       ` Vikram Garhwal
  0 siblings, 0 replies; 50+ messages in thread
From: Vikram Garhwal @ 2022-10-29  5:36 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, stefano.stabellini
  Cc: qemu-devel, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Alex Bennée, xen-devel

Hi Oleksandr,

On 10/28/22 1:26 PM, Oleksandr Tyshchenko wrote:
>
>
> On Thu, Oct 27, 2022 at 12:24 PM Alex Bennée <alex.bennee@linaro.org> 
> wrote:
>
> Hello all
>
>
>
>     Vikram Garhwal <vikram.garhwal@amd.com> writes:
>
>     > xenstore_record_dm_state() will also be used in aarch64 xenpv
>     machine.
>     >
>     > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>     > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>     > ---
>     >  accel/xen/xen-all.c  | 2 +-
>     >  include/hw/xen/xen.h | 2 ++
>     >  2 files changed, 3 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>     > index 69aa7d018b..276625b78b 100644
>     > --- a/accel/xen/xen-all.c
>     > +++ b/accel/xen/xen-all.c
>     > @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i,
>     Chardev *chr)
>     >  }
>     >
>     >
>     > -static void xenstore_record_dm_state(struct xs_handle *xs,
>     const char *state)
>     > +void xenstore_record_dm_state(struct xs_handle *xs, const char
>     *state)
>     >  {
>     >      char path[50];
>     >
>     > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>     > index afdf9c436a..31e9538a5c 100644
>     > --- a/include/hw/xen/xen.h
>     > +++ b/include/hw/xen/xen.h
>     > @@ -9,6 +9,7 @@
>     >   */
>     >
>     >  #include "exec/cpu-common.h"
>     > +#include <xenstore.h>
>
>     This is breaking a bunch of the builds and generally we try and avoid
>     adding system includes in headers (apart from osdep.h) for this
>     reason.
>     In fact there is a comment just above to that fact.
>
>     I think you can just add struct xs_handle to typedefs.h (or maybe just
>     xen.h) and directly include xenstore.h in xen-all.c following the
>     usual
>     rules:
>
>     https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
>     It might be worth doing an audit to see what else is including xen.h
>     needlessly or should be using sysemu/xen.h.
>
>     >
>     >  /* xen-machine.c */
>     >  enum xen_mode {
>     > @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>     >  void xenstore_store_pv_console_info(int i, Chardev *chr);
>     >
>     >  void xen_register_framebuffer(struct MemoryRegion *mr);
>     > +void xenstore_record_dm_state(struct xs_handle *xs, const char
>     *state);
>     >
>     >  #endif /* QEMU_HW_XEN_H */
>
>
>     -- 
>     Alex Bennée
>
>
>
> For considering:
> I think this patch and some other changes done in "[PATCH v1 10/12] 
> hw/arm: introduce xenpv machine" (the opening of Xen interfaces and 
> calling xenstore_record_dm_state() in hw/arm/xen_arm.c:xen_init_ioreq())
> could be avoided if we enable the Xen accelerator (either by passing 
> "-M xenpv,accel=xen" or by adding mc->default_machine_opts = 
> "accel=xen"; to hw/arm/xen_arm.c:xen_arm_machine_class_init() or by 
> some other method).
> These actions are already done in accel/xen/xen-all.c:xen_init(). 
> Please note, that I am not too familiar with that code, so there might 
> be nuances.
>
> Besides that, Xen accelerator will be needed for the xen-mapcache to 
> be in use (this is needed for mapping guest memory), there are a few 
> xen_enabled() checks spreading around that code to perform Xen 
> specific actions.
>
Unfortunately, I am not that familiar with xen as accelerator function. 
Let me check and get back to you.
> -- 
> Regards,
>
> Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
  2022-10-29  5:22     ` Garhwal, Vikram
@ 2022-11-01 11:29       ` Alex Bennée
  0 siblings, 0 replies; 50+ messages in thread
From: Alex Bennée @ 2022-11-01 11:29 UTC (permalink / raw)
  To: Garhwal, Vikram
  Cc: qemu-devel, Stabellini, Stefano, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel


"Garhwal, Vikram" <vikram.garhwal@amd.com> writes:

> Thanks, Alex, for reviewing this one. I built for all the archs and it was fine. Can you please share more about what
> environment builds are breaking? So, I can test the changes for v2.

My cross build environment failed:

  ../../configure' '--disable-docs' '--disable-tools' '--cross-prefix=aarch64-linux-gnu-' '--enable-xen' '--target-list=i386-softmmu,x86_64-softmmu,arm-softmmu,aarch64-softmmu' '--disable-tpm'

On a Debian Bullseye with:

  11:30:20 [root@zen:~] # dpkg -l libxen\*
  Desired=Unknown/Install/Remove/Purge/Hold
  | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
  |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
  ||/ Name                       Version                 Architecture Description
  +++-==========================-=======================-============-====================================================
  ii  libxen-dev:arm64           4.14.5+24-g87d90d511c-1 arm64        Public headers and libs for Xen
  ii  libxencall1:amd64          4.14.5+24-g87d90d511c-1 amd64        Xen runtime library - libxencall
  ii  libxencall1:arm64          4.14.5+24-g87d90d511c-1 arm64        Xen runtime library - libxencall
  ii  libxendevicemodel1:amd64   4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxendevicemodel
  ii  libxendevicemodel1:arm64   4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxendevicemodel
  ii  libxenevtchn1:amd64        4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenevtchn
  ii  libxenevtchn1:arm64        4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenevtchn
  ii  libxenforeignmemory1:amd64 4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenforeignmemory
  ii  libxenforeignmemory1:arm64 4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenforeignmemory
  ii  libxengnttab1:amd64        4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxengnttab
  ii  libxengnttab1:arm64        4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxengnttab
  ii  libxenhypfs1:amd64         4.14.5+24-g87d90d511c-1 amd64        Xen runtime library - libxenhypfs
  ii  libxenhypfs1:arm64         4.14.5+24-g87d90d511c-1 arm64        Xen runtime library - libxenhypfs
  ii  libxenmisc4.14:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - miscellaneous, versioned ABI
  ii  libxenmisc4.14:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - miscellaneous, versioned ABI
  ii  libxenstore3.0:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenstore
  ii  libxenstore3.0:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenstore
  ii  libxentoolcore1:amd64      4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxentoolcore
  ii  libxentoolcore1:arm64      4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxentoolcore
  ii  libxentoollog1:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxentoollog
  ii  libxentoollog1:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxentoollog

But also a bunch of cross builds on the CI system:

  https://gitlab.com/stsquad/qemu/-/pipelines/677956972/failures

>
>  
>
> Regards,
>
> Vikram
>
>  
>
> From: Alex Bennée <alex.bennee@linaro.org>
> Date: Thursday, October 27, 2022 at 2:24 AM
> To: Garhwal, Vikram <vikram.garhwal@amd.com>
> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, Stabellini, Stefano <stefano.stabellini@amd.com>, Stefano
> Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>,
> xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
>
> Vikram Garhwal <vikram.garhwal@amd.com> writes:
>
>> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>>
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> ---
>>  accel/xen/xen-all.c  | 2 +-
>>  include/hw/xen/xen.h | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 69aa7d018b..276625b78b 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>>  }
>>  
>>  
>> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>>  {
>>      char path[50];
>>  
>> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>> index afdf9c436a..31e9538a5c 100644
>> --- a/include/hw/xen/xen.h
>> +++ b/include/hw/xen/xen.h
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include "exec/cpu-common.h"
>> +#include <xenstore.h>
>
> This is breaking a bunch of the builds and generally we try and avoid
> adding system includes in headers (apart from osdep.h) for this reason.
> In fact there is a comment just above to that fact.
>
> I think you can just add struct xs_handle to typedefs.h (or maybe just
> xen.h) and directly include xenstore.h in xen-all.c following the usual
> rules:
>
>   https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
> It might be worth doing an audit to see what else is including xen.h
> needlessly or should be using sysemu/xen.h. 
>
>>  
>>  /* xen-machine.c */
>>  enum xen_mode {
>> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>>  
>>  void xen_register_framebuffer(struct MemoryRegion *mr);
>> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>>  
>>  #endif /* QEMU_HW_XEN_H */


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/
  2022-10-19 14:53   ` Paul Durrant
@ 2022-12-01 20:25     ` Garhwal, Vikram
  0 siblings, 0 replies; 50+ messages in thread
From: Garhwal, Vikram @ 2022-12-01 20:25 UTC (permalink / raw)
  To: Paul Durrant, qemu-devel
  Cc: Stabellini, Stefano, Michael S. Tsirkin, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Stefano Stabellini, Anthony Perard, open list:X86 Xen CPUs

[-- Attachment #1: Type: text/plain, Size: 4444 bytes --]

Hi Paul,

From: Paul Durrant <xadimgnik@gmail.com>
Date: Wednesday, October 19, 2022 at 7:54 AM
To: Garhwal, Vikram <vikram.garhwal@amd.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: Stabellini, Stefano <stefano.stabellini@amd.com>, Michael S. Tsirkin <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, open list:X86 Xen CPUs <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/
On 15/10/2022 06:07, Vikram Garhwal wrote:
> xen-mapcache.c contains common functions which can be used for enabling Xen on
> aarch64 with IOREQ handling. Moving it out from hw/i386/xen to hw/xen to make it
> accessible for both aarch64 and x86.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>   hw/i386/meson.build              | 1 +
>   hw/i386/xen/meson.build          | 1 -
>   hw/i386/xen/trace-events         | 5 -----
>   hw/xen/meson.build               | 4 ++++
>   hw/xen/trace-events              | 5 +++++
>   hw/{i386 => }/xen/xen-mapcache.c | 0
>   6 files changed, 10 insertions(+), 6 deletions(-)
>   rename hw/{i386 => }/xen/xen-mapcache.c (100%)
>
> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
> index 213e2e82b3..cfdbfdcbcb 100644
> --- a/hw/i386/meson.build
> +++ b/hw/i386/meson.build
> @@ -33,5 +33,6 @@ subdir('kvm')
>   subdir('xen')
>
>   i386_ss.add_all(xenpv_ss)
> +i386_ss.add_all(xen_ss)
>
>   hw_arch += {'i386': i386_ss}
> diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
> index be84130300..2fcc46e6ca 100644
> --- a/hw/i386/xen/meson.build
> +++ b/hw/i386/xen/meson.build
> @@ -1,6 +1,5 @@
>   i386_ss.add(when: 'CONFIG_XEN', if_true: files(
>     'xen-hvm.c',
> -  'xen-mapcache.c',
>     'xen_apic.c',
>     'xen_platform.c',
>     'xen_pvdevice.c',
> diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
> index 5d6be61090..a0c89d91c4 100644
> --- a/hw/i386/xen/trace-events
> +++ b/hw/i386/xen/trace-events
> @@ -21,8 +21,3 @@ xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
>   cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
>   cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
>
> -# xen-mapcache.c
> -xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
> -xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
> -xen_map_cache_return(void* ptr) "%p"
> -
> diff --git a/hw/xen/meson.build b/hw/xen/meson.build
> index ae0ace3046..19d0637c46 100644
> --- a/hw/xen/meson.build
> +++ b/hw/xen/meson.build
> @@ -22,3 +22,7 @@ else
>   endif
>
>   specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
> +
> +xen_ss = ss.source_set()
> +
> +xen_ss.add(when: 'CONFIG_XEN', if_true: files('xen-mapcache.c'))

Curious as to why you couldn't just add this to the softmmu_ss list above?
Moving it to softmmu_ss breaks the build as it builds without XEN_CONFIG. I think xen option in “softmmu_ss.add(when: ['CONFIG_XEN', xen], if_true: files” is problem here.  I see below error:
/sysemu/xen-mapcache.h:16:8: error: attempt to use poisoned "CONFIG_XEN"
#ifdef CONFIG_XEN
        ^
../hw/xen/xen-mapcache.c:106:6: error: redefinition of 'xen_map_cache_init'
void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)


   Paul

> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index 3da3fd8348..2c8f238f42 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -41,3 +41,8 @@ xs_node_vprintf(char *path, char *value) "%s %s"
>   xs_node_vscanf(char *path, char *value) "%s %s"
>   xs_node_watch(char *path) "%s"
>   xs_node_unwatch(char *path) "%s"
> +
> +# xen-mapcache.c
> +xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
> +xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
> +xen_map_cache_return(void* ptr) "%p"
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> similarity index 100%
> rename from hw/i386/xen/xen-mapcache.c
> rename to hw/xen/xen-mapcache.c

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 21778 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-10-16 17:47   ` Julien Grall
  2022-10-18  1:26     ` Stefano Stabellini
  2022-10-27  8:37     ` Alex Bennée
@ 2022-12-02  3:24     ` Garhwal, Vikram
  2022-12-02  9:53       ` Julien Grall
  2 siblings, 1 reply; 50+ messages in thread
From: Garhwal, Vikram @ 2022-12-02  3:24 UTC (permalink / raw)
  To: Julien Grall, qemu-devel
  Cc: Stabellini, Stefano, Peter Maydell, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:ARM TCG CPUs,
	open list:X86 Xen CPUs

[-- Attachment #1: Type: text/plain, Size: 7489 bytes --]

Hi Julien,

From: Julien Grall <julien@xen.org>
Date: Sunday, October 16, 2022 at 10:48 AM
To: Garhwal, Vikram <vikram.garhwal@amd.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: Stabellini, Stefano <stefano.stabellini@amd.com>, Peter Maydell <peter.maydell@linaro.org>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, open list:ARM TCG CPUs <qemu-arm@nongnu.org>, open list:X86 Xen CPUs <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
Hi,

There seem to be some missing patches on xen-devel (including the cover
letter). Is that expected?
Not sure what went wrong there. I can see all of these on QEMU-devel. Perhaps xen-devel is not in maintainer’s list for all the xen files?


On 15/10/2022 06:07, Vikram Garhwal wrote:
> Add a new machine xenpv which creates a IOREQ server to register/connect with
> Xen Hypervisor.

I don't like the name 'xenpv' because it doesn't convey the fact that
some of the HW may be emulated rather than para-virtualized. In fact one
may only want to use for emulating devices.

Potential name would be 'xen-arm' or re-using 'virt' but with
'accel=xen' to select a Xen layout.

>
> Xen IOREQ connection expect the TARGET_PAGE_SIZE to 4096, and the xenpv machine
> on ARM will have no CPU definitions. We need to define TARGET_PAGE_SIZE
> appropriately ourselves.
>
> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
> TPM emulator and connects to swtpm running on host machine via chardev socket
> and support TPM functionalities for a guest domain.
>
> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
>
> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
> provides access to TPM functionality over socket, chardev and CUSE interface.
> Github repo: https://github.com/stefanberger/swtpm
> Example for starting swtpm on host machine:
>      mkdir /tmp/vtpm2
>      swtpm socket --tpmstate dir=/tmp/vtpm2 \
>      --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &

I see patches for QEMU but not Xen. How can this be tested with existing
Xen? Will libxl ever create QEMU?
Will send the patch for libxl Xen separately.


[...]

> +static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
> +{
> +    xen_dmod = xendevicemodel_open(0, 0);
> +    xen_xc = xc_interface_open(0, 0, 0);
> +
> +    if (xen_xc == NULL) {

You are checking xen_xc but not xen_dmod. Why?

> +        perror("xen: can't open xen interface\n");
> +        return -1;
> +    }
> +
> +    xen_fmem = xenforeignmemory_open(0, 0);
> +    if (xen_fmem == NULL) {
> +        perror("xen: can't open xen fmem interface\n");
> +        xc_interface_close(xen_xc);
> +        return -1;
> +    }
> +
> +    xen_register_ioreq(state, max_cpus, xen_memory_listener);
> +
> +    xenstore_record_dm_state(xenstore, "running");
> +
> +    return 0;
> +}
> +
> +static void xen_enable_tpm(void)
> +{
> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> +#ifdef CONFIG_TPM
> +    Error *errp = NULL;
> +    DeviceState *dev;
> +    SysBusDevice *busdev;
> +
> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
> +    if (be == NULL) {
> +        DPRINTF("Couldn't fine the backend for tpm0\n");
> +        return;
> +    }
> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);

I can't find where GUEST_TPM_BASE is defined. But then the guest memory
layout is not expected to be stable. With your current approach, it
means QEMU would need to be rebuilt for every Xen version. Is it what we
want?
I cannot think of better way to do this. Either we add the the def here or rebuild it if GUEST_TPM_BASE changes for each xen version.



> +
> +    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
> +#endif
> +}
> +
> +static void xen_arm_init(MachineState *machine)
> +{
> +    XenArmState *xam = XEN_ARM(machine);
> +
> +    xam->state =  g_new0(XenIOState, 1);
> +
> +    if (xen_init_ioreq(xam->state, machine->smp.cpus)) {
> +        return;

In another patch, you said the IOREQ would be optional. IHMO, I think
this is a bad idea to register it by default because one may only want
to use PV drivers. Registering IOREQ will add unnecessary overhead in Xen.

Furthermore, it means that someone selecting TPM but Xen is not built
with CONFIG_IOREQ=y (BTW This is still a tech preview but there are
security holes on Arm...) will not get an error. Instead, the OS will
until it crashes when trying to access the TPM.

Overall I think it would be better if IOREQ is only registered when a
device requires (like TPM) it *and* throw an error if there is a problem
during the initialization.


> +    } > +
> +    xen_enable_tpm();
> +
> +    return;
> +}
> +
> +static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
> +{
> +
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    mc->desc = "Xen Para-virtualized PC";
> +    mc->init = xen_arm_init;
> +    mc->max_cpus = 1;
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);

Shouldn't this be protected with #ifdef CONFIG_TPM?

> +}
> +
> +static const TypeInfo xen_arm_machine_type = {
> +    .name = TYPE_XEN_ARM,
> +    .parent = TYPE_MACHINE,
> +    .class_init = xen_arm_machine_class_init,
> +    .instance_size = sizeof(XenArmState),
> +};
> +
> +static void xen_arm_machine_register_types(void)
> +{
> +    type_register_static(&xen_arm_machine_type);
> +}
> +
> +type_init(xen_arm_machine_register_types)
> diff --git a/include/hw/arm/xen_arch_hvm.h b/include/hw/arm/xen_arch_hvm.h
> new file mode 100644
> index 0000000000..f645dfec28
> --- /dev/null
> +++ b/include/hw/arm/xen_arch_hvm.h
> @@ -0,0 +1,12 @@
> +#ifndef HW_XEN_ARCH_ARM_HVM_H
> +#define HW_XEN_ARCH_ARM_HVM_H
> +
> +#include <xen/hvm/ioreq.h>
> +void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
> +void arch_xen_set_memory(XenIOState *state,
> +                         MemoryRegionSection *section,
> +                         bool add);
> +
> +#undef TARGET_PAGE_SIZE

I am a bit puzzled with this #undef. In the commit message you said that
there will be no CPU definition. So the implications is this should not
be defined.

If it is defined, then what guarantees that all the source will use the
correct value?


> +#define TARGET_PAGE_SIZE 4096

It would be better to use XC_PAGE_SIZE (or similar) rather than
hardcoding it.
Corrected this and sent a v2.


> +#endif
> diff --git a/include/hw/xen/arch_hvm.h b/include/hw/xen/arch_hvm.h
> index 26674648d8..c7c515220d 100644
> --- a/include/hw/xen/arch_hvm.h
> +++ b/include/hw/xen/arch_hvm.h
> @@ -1,3 +1,5 @@
>   #if defined(TARGET_I386) || defined(TARGET_X86_64)
>   #include "hw/i386/xen_arch_hvm.h"
> +#elif defined(TARGET_ARM) || defined(TARGET_ARM_64)
> +#include "hw/arm/xen_arch_hvm.h"
>   #endif

Cheers,

--
Julien Grall

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 26085 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
  2022-12-02  3:24     ` Garhwal, Vikram
@ 2022-12-02  9:53       ` Julien Grall
  0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2022-12-02  9:53 UTC (permalink / raw)
  To: Garhwal, Vikram, qemu-devel
  Cc: Stabellini, Stefano, Peter Maydell, Stefano Stabellini,
	Anthony Perard, Paul Durrant, open list:ARM TCG CPUs,
	open list:X86 Xen CPUs



On 02/12/2022 03:24, Garhwal, Vikram wrote:
> Hi Julien,

Hi Vikram,

I am having trouble to differentiate your answers from my remark. For 
instance...


> From: Julien Grall <julien@xen.org>
> Date: Sunday, October 16, 2022 at 10:48 AM
> To: Garhwal, Vikram <vikram.garhwal@amd.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>
> Cc: Stabellini, Stefano <stefano.stabellini@amd.com>, Peter Maydell <peter.maydell@linaro.org>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, open list:ARM TCG CPUs <qemu-arm@nongnu.org>, open list:X86 Xen CPUs <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
> Hi,
> 
> There seem to be some missing patches on xen-devel (including the cover
> letter). Is that expected?
> Not sure what went wrong there. I can see all of these on QEMU-devel. Perhaps xen-devel is not in maintainer’s list for all the xen files?
> 
> 
> On 15/10/2022 06:07, Vikram Garhwal wrote:
>> Add a new machine xenpv which creates a IOREQ server to register/connect with
>> Xen Hypervisor.
> 
> I don't like the name 'xenpv' because it doesn't convey the fact that
> some of the HW may be emulated rather than para-virtualized. In fact one
> may only want to use for emulating devices.
> 
> Potential name would be 'xen-arm' or re-using 'virt' but with
> 'accel=xen' to select a Xen layout.
> 
>>
>> Xen IOREQ connection expect the TARGET_PAGE_SIZE to 4096, and the xenpv machine
>> on ARM will have no CPU definitions. We need to define TARGET_PAGE_SIZE
>> appropriately ourselves.
>>
>> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
>> TPM emulator and connects to swtpm running on host machine via chardev socket
>> and support TPM functionalities for a guest domain.
>>
>> Extra command line for aarch64 xenpv QEMU to connect to swtpm:
>>       -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>>       -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>
>> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
>> provides access to TPM functionality over socket, chardev and CUSE interface.
>> Github repo: https://github.com/stefanberger/swtpm
>> Example for starting swtpm on host machine:
>>       mkdir /tmp/vtpm2
>>       swtpm socket --tpmstate dir=/tmp/vtpm2 \
>>       --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
> 
> I see patches for QEMU but not Xen. How can this be tested with existing
> Xen? Will libxl ever create QEMU?
> Will send the patch for libxl Xen separately.

... the first two lines are my remarks and the 3rd is your answer. Can 
you configure your e-mail client to do proper quoting?

[...]

>> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
>> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
>> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(busdev, &error_fatal);
>> +    sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
> 
> I can't find where GUEST_TPM_BASE is defined. But then the guest memory
> layout is not expected to be stable. With your current approach, it
> means QEMU would need to be rebuilt for every Xen version. Is it what we
> want?
> I cannot think of better way to do this. Either we add the the def here or rebuild it if GUEST_TPM_BASE changes for each xen version.

The alternative would be to specify the address on the QEMU command 
line. The advantage is you could build a system where each guests have 
different layout.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2022-12-02  9:54 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-15  5:07 [PATCH v1 00/12] Introduce xenpv machine for arm architecture Vikram Garhwal
2022-10-15  5:07 ` [PATCH v1 01/12] hw/xen: Correct build config for xen_pt_stub Vikram Garhwal
2022-10-19 14:46   ` Paul Durrant
2022-10-26 19:01   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 02/12] hw/i386/xen/: move xen-mapcache.c to hw/xen/ Vikram Garhwal
2022-10-19 14:53   ` Paul Durrant
2022-12-01 20:25     ` Garhwal, Vikram
2022-10-15  5:07 ` [PATCH v1 03/12] hw/i386/xen: rearrange xen_hvm_init_pc Vikram Garhwal
2022-10-19 14:59   ` Paul Durrant
2022-10-15  5:07 ` [PATCH v1 04/12] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState Vikram Garhwal
2022-10-19 15:56   ` Paul Durrant
2022-10-19 16:00   ` Paul Durrant
2022-10-27  8:59   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 05/12] hw/i386/xen/xen-hvm: create arch_handle_ioreq and arch_xen_set_memory Vikram Garhwal
2022-10-19 16:09   ` Paul Durrant
2022-10-27  9:02   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 06/12] xen-hvm: move common functions to hw/xen/xen-hvm-common.c Vikram Garhwal
2022-10-16 18:07   ` Julien Grall
2022-10-19 16:16   ` Paul Durrant
2022-10-19 23:12     ` Garhwal, Vikram
2022-10-15  5:07 ` [PATCH v1 07/12] include/hw/xen/xen_common: return error from xen_create_ioreq_server Vikram Garhwal
2022-10-16 17:53   ` Julien Grall
2022-10-15  5:07 ` [PATCH v1 08/12] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure Vikram Garhwal
2022-10-27  8:46   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state Vikram Garhwal
2022-10-19 16:20   ` Paul Durrant
2022-10-27  9:14   ` Alex Bennée
2022-10-28 20:26     ` Oleksandr Tyshchenko
2022-10-29  5:36       ` Vikram Garhwal
2022-10-29  5:22     ` Garhwal, Vikram
2022-11-01 11:29       ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 10/12] hw/arm: introduce xenpv machine Vikram Garhwal
2022-10-16 17:47   ` Julien Grall
2022-10-18  1:26     ` Stefano Stabellini
2022-10-18  8:24       ` Julien Grall
2022-10-19  0:15         ` Stefano Stabellini
2022-10-19  9:49           ` Julien Grall
2022-10-27  8:37     ` Alex Bennée
2022-12-02  3:24     ` Garhwal, Vikram
2022-12-02  9:53       ` Julien Grall
2022-10-27  8:02   ` Alex Bennée
2022-10-28 17:57     ` Julien Grall
2022-10-28 19:05       ` Oleksandr Tyshchenko
2022-10-27  8:40   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 11/12] meson.build: enable xenpv machine build for ARM Vikram Garhwal
2022-10-27  9:11   ` Alex Bennée
2022-10-15  5:07 ` [PATCH v1 12/12] meson.build: do not set have_xen_pci_passthrough for aarch64 targets Vikram Garhwal
2022-10-27  9:14   ` Alex Bennée
2022-10-27  8:26 ` [PATCH v1 00/12] Introduce xenpv machine for arm architecture Alex Bennée
2022-10-27  9:24 ` Alex Bennée

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.