[v2] hw/xen: Introduce XEN_FV/XEN_PV Kconfig

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

Posted by Philippe Mathieu-Daudé 4 years, 9 months ago

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
-- 
2.26.2

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

Posted by Paolo Bonzini 4 years, 9 months ago

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

Posted by Philippe Mathieu-Daudé 4 years, 9 months ago

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

Posted by Paolo Bonzini 4 years, 9 months ago

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 4/4] hw/xen: Have Xen machines select 9pfs

Posted by Philippe Mathieu-Daudé 4 years, 9 months ago

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.