Re: [PATCH] accel: Move Xen accelerator code under accel/xen/

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 5/7/20 5:58 PM, Philippe Mathieu-Daudé wrote:
> This code is not related to hardware emulation.
> Move it under accel/ with the other hypervisors.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> ---
> We could also move the memory management functions from
> hw/i386/xen/xen-hvm.c but it is not trivial.
>
> Necessary step to remove "exec/ram_addr.h" in the next series.
> ---
>   include/exec/ram_addr.h                    |  2 +-
>   include/hw/xen/xen.h                       | 11 ------
>   include/sysemu/xen.h                       | 40 ++++++++++++++++++++++
>   hw/xen/xen-common.c => accel/xen/xen-all.c |  3 ++
>   hw/acpi/piix4.c                            |  2 +-
>   hw/i386/pc.c                               |  1 +
>   hw/i386/pc_piix.c                          |  1 +
>   hw/i386/pc_q35.c                           |  1 +
>   hw/i386/xen/xen-hvm.c                      |  1 +
>   hw/i386/xen/xen_platform.c                 |  1 +
>   hw/isa/piix3.c                             |  1 +
>   hw/pci/msix.c                              |  1 +
>   migration/savevm.c                         |  2 +-
>   softmmu/vl.c                               |  2 +-
>   stubs/xen-hvm.c                            |  9 -----
>   target/i386/cpu.c                          |  2 +-
>   MAINTAINERS                                |  2 ++
>   accel/Makefile.objs                        |  1 +
>   accel/xen/Makefile.objs                    |  1 +
>   hw/xen/Makefile.objs                       |  2 +-
>   20 files changed, 60 insertions(+), 26 deletions(-)
>   create mode 100644 include/sysemu/xen.h
>   rename hw/xen/xen-common.c => accel/xen/xen-all.c (99%)
>   create mode 100644 accel/xen/Makefile.objs
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5e59a3d8d7..4e05292f91 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -21,7 +21,7 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   #include "cpu.h"
> -#include "hw/xen/xen.h"
> +#include "sysemu/xen.h"
>   #include "sysemu/tcg.h"
>   #include "exec/ramlist.h"
>   #include "exec/ramblock.h"
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 5ac1c6dc55..771dd447f2 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -20,13 +20,6 @@ extern uint32_t xen_domid;
>   extern enum xen_mode xen_mode;
>   extern bool xen_domid_restrict;
>   
> -extern bool xen_allowed;
> -
> -static inline bool xen_enabled(void)
> -{
> -    return xen_allowed;
> -}
> -
>   int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>   void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>   void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int 
> len);
> @@ -39,10 +32,6 @@ void xenstore_store_pv_console_info(int i, struct Chardev 
> *chr);
>   
>   void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory);
>   
> -void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
> -                   struct MemoryRegion *mr, Error **errp);
> -void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
> -
>   void xen_register_framebuffer(struct MemoryRegion *mr);
>   
>   #endif /* QEMU_HW_XEN_H */
> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
> new file mode 100644
> index 0000000000..609d2d4184
> --- /dev/null
> +++ b/include/sysemu/xen.h
> @@ -0,0 +1,40 @@
> +/*
> + * QEMU Xen support
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef SYSEMU_XEN_H
> +#define SYSEMU_XEN_H
> +
> +#ifdef CONFIG_XEN
> +
> +extern bool xen_allowed;
> +
> +#define xen_enabled() (xen_allowed)
> +
> +#ifndef CONFIG_USER_ONLY
> +void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
> +void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
> +                   struct MemoryRegion *mr, Error **errp);
> +#endif
> +
> +#else /* !CONFIG_XEN */
> +
> +#define xen_enabled() 0
> +#ifndef CONFIG_USER_ONLY
> +static inline void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
> +{
> +    abort();
> +}
> +static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
> +                                 MemoryRegion *mr, Error **errp)
> +{
> +    abort();
> +}
> +#endif
Unfortunately this triggers:

#1  0x00007fca778a5895 abort (libc.so.6)
#2  0x000055f51ebd3e12 xen_hvm_modified_memory (qemu-system-aarch64)
#3  0x000055f51ebd44c9 cpu_physical_memory_set_dirty_range 
(qemu-system-aarch64)
#4  0x000055f51ebd9de4 ram_block_add (qemu-system-aarch64)
#5  0x000055f51ebda2e3 qemu_ram_alloc_internal (qemu-system-aarch64)
#6  0x000055f51ebda3be qemu_ram_alloc (qemu-system-aarch64)
#7  0x000055f51ec3db9b memory_region_init_ram_shared_nomigrate 
(qemu-system-aarch64)
#8  0x000055f51ef089e6 ram_backend_memory_alloc (qemu-system-aarch64)
#9  0x000055f51ef081ae host_memory_backend_memory_complete 
(qemu-system-aarch64)
#10 0x000055f51f2b624b user_creatable_complete (qemu-system-aarch64)
#11 0x000055f51ed93382 create_default_memdev (qemu-system-aarch64)
#12 0x000055f51ed96d1a qemu_init (qemu-system-aarch64)
#13 0x000055f51f3b14bb main (qemu-system-aarch64)
#14 0x00007fca778a6f43 __libc_start_main (libc.so.6)
#15 0x000055f51ebd27de _start (qemu-system-aarch64)

I'll resend adding the following patch checking for xen_enabled() before 
calling, as we do with other accelerators:
-- >8 --
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
@@ -330,7 +330,9 @@ static inline void 
cpu_physical_memory_set_dirty_range(ram_addr_t start,
         }
     }

-    xen_hvm_modified_memory(start, length);
+    if (xen_enabled()) {
+        xen_hvm_modified_memory(start, length);
+    }
 }

 #if !defined(_WIN32)
@@ -388,7 +390,9 @@ static inline void 
cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
             }
         }

-        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        if (xen_enabled()) {
+            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        }
     } else {
         uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : 
DIRTY_CLIENTS_NOCODE;
---