Re: [PATCH v2 1/4] meson: Do not build Xen x86_64-softmmu on Aarch64

Re: [PATCH v2 1/4] meson: Do not build Xen x86_64-softmmu on Aarch64

[Philippe Mathieu-Daudé]

On 1/31/21 3:45 PM, andrew.cooper3--- via wrote:
> On 31/01/2021 14:18, Philippe Mathieu-Daudé wrote:
>> The Xen on ARM documentation only mentions the i386-softmmu
>> target. As the x86_64-softmmu doesn't seem used, remove it
>> to avoid wasting cpu cycles building it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> As far as I understand, it only gets used at all on ARM for the
> blkback=>qcow path, and has nothing to do with I440FX or other boards. 
> i.e. it is a paravirt disk and nothing else.

Yeah the PIIX3 part is messy, this is easier to select I440FX which
provides all the required dependencies. TBH I'd rather invest my
time in other tasks, and the Xen folks don't seem interested in getting
this improved. I only did that series to reply to Paolo and pass over
to Alex Bennée.

> xenpv should not be tied to i386-softmmu in the first place, and would
> remove a very-WTF-worthy current state of things.  That said, I have no
> idea how much effort that might be.

Here the problem isn't much Xen but the rest of x86 machines in QEMU.

Regards,

Phil.

Re: [PATCH v2 4/4] hw/xen: Have Xen machines select 9pfs

[Paolo Bonzini]

On 31/01/21 15:18, Philippe Mathieu-Daudé wrote:
> 9pfs is not an accelerator feature but a machine one,
> move the selection on the machine Kconfig (in hw/).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   accel/Kconfig       | 1 -
>   hw/i386/xen/Kconfig | 1 +
>   hw/xen/Kconfig      | 1 +
>   3 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/Kconfig b/accel/Kconfig
> index 461104c7715..b9e9a2d35b0 100644
> --- a/accel/Kconfig
> +++ b/accel/Kconfig
> @@ -15,4 +15,3 @@ config KVM
>   
>   config XEN
>       bool
> -    select FSDEV_9P if VIRTFS
> diff --git a/hw/i386/xen/Kconfig b/hw/i386/xen/Kconfig
> index ad9d774b9ea..4affd842f28 100644
> --- a/hw/i386/xen/Kconfig
> +++ b/hw/i386/xen/Kconfig
> @@ -3,3 +3,4 @@ config XEN_FV
>       default y if XEN
>       depends on XEN
>       select I440FX
> +    select FSDEV_9P if VIRTFS
> diff --git a/hw/xen/Kconfig b/hw/xen/Kconfig
> index 0b8427d6bd1..825277969fa 100644
> --- a/hw/xen/Kconfig
> +++ b/hw/xen/Kconfig
> @@ -5,3 +5,4 @@ config XEN_PV
>       select PCI
>       select USB
>       select IDE_PIIX
> +    select FSDEV_9P if VIRTFS
> 

I think you can compile without FSDEV_9P selected?  If so, this should 
be "imply".

If on the other hand you cannot, and that is because of some other file 
brought in by CONFIG_XEN, this patch shouldn't be there.

I can queue the series once you resolve this doubt.

Paolo

Re: [PATCH v2 4/4] hw/xen: Have Xen machines select 9pfs

[Philippe Mathieu-Daudé]

On 2/1/21 9:34 AM, Paolo Bonzini wrote:
> On 31/01/21 15:18, Philippe Mathieu-Daudé wrote:
>> 9pfs is not an accelerator feature but a machine one,
>> move the selection on the machine Kconfig (in hw/).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   accel/Kconfig       | 1 -
>>   hw/i386/xen/Kconfig | 1 +
>>   hw/xen/Kconfig      | 1 +
>>   3 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/accel/Kconfig b/accel/Kconfig
>> index 461104c7715..b9e9a2d35b0 100644
>> --- a/accel/Kconfig
>> +++ b/accel/Kconfig
>> @@ -15,4 +15,3 @@ config KVM
>>     config XEN
>>       bool
>> -    select FSDEV_9P if VIRTFS
>> diff --git a/hw/i386/xen/Kconfig b/hw/i386/xen/Kconfig
>> index ad9d774b9ea..4affd842f28 100644
>> --- a/hw/i386/xen/Kconfig
>> +++ b/hw/i386/xen/Kconfig
>> @@ -3,3 +3,4 @@ config XEN_FV
>>       default y if XEN
>>       depends on XEN
>>       select I440FX
>> +    select FSDEV_9P if VIRTFS
>> diff --git a/hw/xen/Kconfig b/hw/xen/Kconfig
>> index 0b8427d6bd1..825277969fa 100644
>> --- a/hw/xen/Kconfig
>> +++ b/hw/xen/Kconfig
>> @@ -5,3 +5,4 @@ config XEN_PV
>>       select PCI
>>       select USB
>>       select IDE_PIIX
>> +    select FSDEV_9P if VIRTFS
>>
> 
> I think you can compile without FSDEV_9P selected?  If so, this should
> be "imply".
> 
> If on the other hand you cannot, and that is because of some other file
> brought in by CONFIG_XEN, this patch shouldn't be there.

FYI using 'imply FSDEV_9P' instead I get:

/usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
`xen_be_register_common':
hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'

The function is:

  void xen_be_register_common(void)
  {
      xen_set_dynamic_sysbus();

      xen_be_register("console", &xen_console_ops);
      xen_be_register("vkbd", &xen_kbdmouse_ops);
  #ifdef CONFIG_VIRTFS
      xen_be_register("9pfs", &xen_9pfs_ops);
  #endif
  #ifdef CONFIG_USB_LIBUSB
      xen_be_register("qusb", &xen_usb_ops);
  #endif
  }

The object is compiled using:

-- >8 --
-#ifdef CONFIG_VIRTFS
+#ifdef CONFIG_FSDEV_9P
     xen_be_register("9pfs", &xen_9pfs_ops);
 #endif
---

Respin planned.

Thanks,

Phil.

Re: [PATCH v2 4/4] hw/xen: Have Xen machines select 9pfs

[Paolo Bonzini]

On 01/02/21 10:18, Philippe Mathieu-Daudé wrote:
> FYI using 'imply FSDEV_9P' instead I get:
> 
> /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
> `xen_be_register_common':
> hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'

Ok, so then we have the case of a file (hw/xen/xen-legacy-backend.c) 
brought in by CONFIG_XEN.  In that case this patch is incorrect...

> The function is:
> 
>    void xen_be_register_common(void)
>    {
>        xen_set_dynamic_sysbus();
> 
>        xen_be_register("console", &xen_console_ops);
>        xen_be_register("vkbd", &xen_kbdmouse_ops);
>    #ifdef CONFIG_VIRTFS
>        xen_be_register("9pfs", &xen_9pfs_ops);
>    #endif
>    #ifdef CONFIG_USB_LIBUSB
>        xen_be_register("qusb", &xen_usb_ops);
>    #endif
>    }
> 
> The object is compiled using:
> 
> -- >8 --
> -#ifdef CONFIG_VIRTFS
> +#ifdef CONFIG_FSDEV_9P
>       xen_be_register("9pfs", &xen_9pfs_ops);
>   #endif
> ---

... and this is the best fix, together with:

- a "#include CONFIG_DEVICES" at the top (to get CONFIG_FSDEV_9P)

- moving xen-legacy-backend.c from softmmu_ss to specific_ss (to get 
CONFIG_DEVICES)

- changing "select" to "imply" in accel/Kconfig (otherwise the patch has 
no effect)

But really, doing nothing and just dropping this patch is perfectly fine.

Paolo

Re: [PATCH v2 3/4] hw/xen/Kconfig: Introduce XEN_PV config

[Philippe Mathieu-Daudé]

On 1/31/21 3:18 PM, Philippe Mathieu-Daudé wrote:
> xenpv machine requires USB, IDE_PIIX and PCI:
> 
>   /usr/bin/ld:
>   libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function `xen_be_register_common':
>   hw/xen/xen-legacy-backend.c:757: undefined reference to `xen_usb_ops'
>   libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `unplug_disks':
>   hw/i386/xen/xen_platform.c:153: undefined reference to `pci_piix3_xen_ide_unplug'
>   libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `pci_unplug_nics':
>   hw/i386/xen/xen_platform.c:137: undefined reference to `pci_for_each_device'
>   libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `xen_platform_realize':
>   hw/i386/xen/xen_platform.c:483: undefined reference to `pci_register_bar'
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/Kconfig     | 1 +
>  hw/xen/Kconfig | 7 +++++++
>  2 files changed, 8 insertions(+)
>  create mode 100644 hw/xen/Kconfig
> 
> diff --git a/hw/Kconfig b/hw/Kconfig
> index 5ad3c6b5a4b..f2a95591d94 100644
> --- a/hw/Kconfig
> +++ b/hw/Kconfig
> @@ -39,6 +39,7 @@ source usb/Kconfig
>  source virtio/Kconfig
>  source vfio/Kconfig
>  source watchdog/Kconfig
> +source xen/Kconfig
>  
>  # arch Kconfig
>  source arm/Kconfig
> diff --git a/hw/xen/Kconfig b/hw/xen/Kconfig
> new file mode 100644
> index 00000000000..0b8427d6bd1
> --- /dev/null
> +++ b/hw/xen/Kconfig
> @@ -0,0 +1,7 @@
> +config XEN_PV
> +    bool
> +    default y if XEN
> +    depends on XEN
> +    select PCI
> +    select USB
> +    select IDE_PIIX

Well this is not enough, --without-default-devices fails:

/usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
function `cpu_physical_memory_set_dirty_range':
include/exec/ram_addr.h:333: undefined reference to
`xen_hvm_modified_memory'
/usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
function `ram_block_add':
softmmu/physmem.c:1873: undefined reference to `xen_ram_alloc'
/usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
function `cpu_physical_memory_set_dirty_range':
include/exec/ram_addr.h:333: undefined reference to
`xen_hvm_modified_memory'
/usr/bin/ld: include/exec/ram_addr.h:333: undefined reference to
`xen_hvm_modified_memory'
/usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_memory.c.o: in function
`cpu_physical_memory_set_dirty_range':
include/exec/ram_addr.h:333: undefined reference to
`xen_hvm_modified_memory'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Re: [PATCH v2 4/4] hw/xen: Have Xen machines select 9pfs

[Philippe Mathieu-Daudé]

On 2/1/21 11:23 AM, Paolo Bonzini wrote:
> On 01/02/21 10:18, Philippe Mathieu-Daudé wrote:
>> FYI using 'imply FSDEV_9P' instead I get:
>>
>> /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
>> `xen_be_register_common':
>> hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
> 
> Ok, so then we have the case of a file (hw/xen/xen-legacy-backend.c)
> brought in by CONFIG_XEN.  In that case this patch is incorrect...
> 
>> The function is:
>>
>>    void xen_be_register_common(void)
>>    {
>>        xen_set_dynamic_sysbus();
>>
>>        xen_be_register("console", &xen_console_ops);
>>        xen_be_register("vkbd", &xen_kbdmouse_ops);
>>    #ifdef CONFIG_VIRTFS
>>        xen_be_register("9pfs", &xen_9pfs_ops);
>>    #endif
>>    #ifdef CONFIG_USB_LIBUSB
>>        xen_be_register("qusb", &xen_usb_ops);
>>    #endif
>>    }
>>
>> The object is compiled using:
>>
>> -- >8 --
>> -#ifdef CONFIG_VIRTFS
>> +#ifdef CONFIG_FSDEV_9P
>>       xen_be_register("9pfs", &xen_9pfs_ops);
>>   #endif
>> ---
> 
> ... and this is the best fix, together with:
> 
> - a "#include CONFIG_DEVICES" at the top (to get CONFIG_FSDEV_9P)
> 
> - moving xen-legacy-backend.c from softmmu_ss to specific_ss (to get
> CONFIG_DEVICES)
> 
> - changing "select" to "imply" in accel/Kconfig (otherwise the patch has
> no effect)

OK.

> 
> But really, doing nothing and just dropping this patch is perfectly fine.

Yes, I'll respin what I have so far and continue when I find the
time and motivation another week-end.

Re: [PATCH v2 3/4] hw/xen/Kconfig: Introduce XEN_PV config

[Paolo Bonzini]

On 01/02/21 11:59, Philippe Mathieu-Daudé wrote:
> On 1/31/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>> xenpv machine requires USB, IDE_PIIX and PCI:
>>
>>    /usr/bin/ld:
>>    libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function `xen_be_register_common':
>>    hw/xen/xen-legacy-backend.c:757: undefined reference to `xen_usb_ops'
>>    libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `unplug_disks':
>>    hw/i386/xen/xen_platform.c:153: undefined reference to `pci_piix3_xen_ide_unplug'
>>    libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `pci_unplug_nics':
>>    hw/i386/xen/xen_platform.c:137: undefined reference to `pci_for_each_device'
>>    libqemu-i386-softmmu.fa.p/hw_i386_xen_xen_platform.c.o: in function `xen_platform_realize':
>>    hw/i386/xen/xen_platform.c:483: undefined reference to `pci_register_bar'
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/Kconfig     | 1 +
>>   hw/xen/Kconfig | 7 +++++++
>>   2 files changed, 8 insertions(+)
>>   create mode 100644 hw/xen/Kconfig
>>
>> diff --git a/hw/Kconfig b/hw/Kconfig
>> index 5ad3c6b5a4b..f2a95591d94 100644
>> --- a/hw/Kconfig
>> +++ b/hw/Kconfig
>> @@ -39,6 +39,7 @@ source usb/Kconfig
>>   source virtio/Kconfig
>>   source vfio/Kconfig
>>   source watchdog/Kconfig
>> +source xen/Kconfig
>>   
>>   # arch Kconfig
>>   source arm/Kconfig
>> diff --git a/hw/xen/Kconfig b/hw/xen/Kconfig
>> new file mode 100644
>> index 00000000000..0b8427d6bd1
>> --- /dev/null
>> +++ b/hw/xen/Kconfig
>> @@ -0,0 +1,7 @@
>> +config XEN_PV
>> +    bool
>> +    default y if XEN
>> +    depends on XEN
>> +    select PCI
>> +    select USB
>> +    select IDE_PIIX
> 
> Well this is not enough, --without-default-devices fails:
> 
> /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
> function `cpu_physical_memory_set_dirty_range':
> include/exec/ram_addr.h:333: undefined reference to
> `xen_hvm_modified_memory'
> /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
> function `ram_block_add':
> softmmu/physmem.c:1873: undefined reference to `xen_ram_alloc'
> /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_physmem.c.o: in
> function `cpu_physical_memory_set_dirty_range':
> include/exec/ram_addr.h:333: undefined reference to
> `xen_hvm_modified_memory'
> /usr/bin/ld: include/exec/ram_addr.h:333: undefined reference to
> `xen_hvm_modified_memory'
> /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/softmmu_memory.c.o: in function
> `cpu_physical_memory_set_dirty_range':
> include/exec/ram_addr.h:333: undefined reference to
> `xen_hvm_modified_memory'
> collect2: error: ld returned 1 exit status
> ninja: build stopped: subcommand failed.

I think you can modify xen_hvm_modified_memory to become a virtual 
function in AccelClass.

Paolo