[Qemu-devel] [PATCH v2] 9pfs: fix dependencies

Cornelia Huck posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170809071718.17924-1-cohuck@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
default-configs/s390x-softmmu.mak | 1 +
fsdev/Makefile.objs               | 9 +++------
hw/Makefile.objs                  | 2 +-
3 files changed, 5 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
device.

Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

Changes v1->v2: drop extraneous spaces, fix build on cris

---
 default-configs/s390x-softmmu.mak | 1 +
 fsdev/Makefile.objs               | 9 +++------
 hw/Makefile.objs                  | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index 51191b77df..e4c5236ceb 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -8,3 +8,4 @@ CONFIG_S390_FLIC=y
 CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
 CONFIG_VFIO_CCW=$(CONFIG_LINUX)
 CONFIG_WDT_DIAG288=y
+CONFIG_VIRTIO_CCW=y
diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
index 659df6e187..3d157add31 100644
--- a/fsdev/Makefile.objs
+++ b/fsdev/Makefile.objs
@@ -1,10 +1,7 @@
-ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
-# only pull in the actual virtio-9p device if we also enabled virtio.
-common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
-else
-common-obj-y = qemu-fsdev-dummy.o
-endif
+# only pull in the actual virtio-9p device if we also enabled a virtio backend.
+common-obj-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))= qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
+common-obj-$(call lnot,$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))) = qemu-fsdev-dummy.o
 common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
 
 # Toplevel always builds this; targets without virtio will put it in
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index a2c61f6b09..335f26b65e 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -1,4 +1,4 @@
-devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
+devices-dirs-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW))) += 9pfs/
 devices-dirs-$(CONFIG_SOFTMMU) += acpi/
 devices-dirs-$(CONFIG_SOFTMMU) += adc/
 devices-dirs-$(CONFIG_SOFTMMU) += audio/
-- 
2.13.4


Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Thomas Huth 6 years, 7 months ago
On 09.08.2017 09:17, Cornelia Huck wrote:
> Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> device.
> 
> Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> Changes v1->v2: drop extraneous spaces, fix build on cris
> 
> ---
>  default-configs/s390x-softmmu.mak | 1 +
>  fsdev/Makefile.objs               | 9 +++------
>  hw/Makefile.objs                  | 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index 51191b77df..e4c5236ceb 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -8,3 +8,4 @@ CONFIG_S390_FLIC=y
>  CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
>  CONFIG_VFIO_CCW=$(CONFIG_LINUX)
>  CONFIG_WDT_DIAG288=y
> +CONFIG_VIRTIO_CCW=y
> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
> index 659df6e187..3d157add31 100644
> --- a/fsdev/Makefile.objs
> +++ b/fsdev/Makefile.objs
> @@ -1,10 +1,7 @@
> -ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
>  # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
> -# only pull in the actual virtio-9p device if we also enabled virtio.
> -common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> -else
> -common-obj-y = qemu-fsdev-dummy.o
> -endif
> +# only pull in the actual virtio-9p device if we also enabled a virtio backend.
> +common-obj-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))= qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> +common-obj-$(call lnot,$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))) = qemu-fsdev-dummy.o
>  common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
>  
>  # Toplevel always builds this; targets without virtio will put it in
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index a2c61f6b09..335f26b65e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,4 +1,4 @@
> -devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
> +devices-dirs-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW))) += 9pfs/
>  devices-dirs-$(CONFIG_SOFTMMU) += acpi/
>  devices-dirs-$(CONFIG_SOFTMMU) += adc/
>  devices-dirs-$(CONFIG_SOFTMMU) += audio/

Patch should be fine now, I think...

But thinking about this again, I wonder whether it would be enough to
simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
sufficient to assert that there is also at least one kind of virtio
transport available, right?
Otherwise this will look really horrible as soon as somebody also tries
to add support for virtio-mmio here later ;-)

 Thomas

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 10:23:04 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 09.08.2017 09:17, Cornelia Huck wrote:
> > Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> > on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> > device.
> > 
> > Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> > CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > Changes v1->v2: drop extraneous spaces, fix build on cris
> > 
> > ---
> >  default-configs/s390x-softmmu.mak | 1 +
> >  fsdev/Makefile.objs               | 9 +++------
> >  hw/Makefile.objs                  | 2 +-
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> > index 51191b77df..e4c5236ceb 100644
> > --- a/default-configs/s390x-softmmu.mak
> > +++ b/default-configs/s390x-softmmu.mak
> > @@ -8,3 +8,4 @@ CONFIG_S390_FLIC=y
> >  CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
> >  CONFIG_VFIO_CCW=$(CONFIG_LINUX)
> >  CONFIG_WDT_DIAG288=y
> > +CONFIG_VIRTIO_CCW=y
> > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
> > index 659df6e187..3d157add31 100644
> > --- a/fsdev/Makefile.objs
> > +++ b/fsdev/Makefile.objs
> > @@ -1,10 +1,7 @@
> > -ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
> >  # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
> > -# only pull in the actual virtio-9p device if we also enabled virtio.
> > -common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> > -else
> > -common-obj-y = qemu-fsdev-dummy.o
> > -endif
> > +# only pull in the actual virtio-9p device if we also enabled a virtio backend.
> > +common-obj-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))= qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> > +common-obj-$(call lnot,$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))) = qemu-fsdev-dummy.o
> >  common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
> >  
> >  # Toplevel always builds this; targets without virtio will put it in
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index a2c61f6b09..335f26b65e 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -1,4 +1,4 @@
> > -devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
> > +devices-dirs-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW))) += 9pfs/
> >  devices-dirs-$(CONFIG_SOFTMMU) += acpi/
> >  devices-dirs-$(CONFIG_SOFTMMU) += adc/
> >  devices-dirs-$(CONFIG_SOFTMMU) += audio/  
> 
> Patch should be fine now, I think...
> 
> But thinking about this again, I wonder whether it would be enough to
> simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> sufficient to assert that there is also at least one kind of virtio
> transport available, right?
> Otherwise this will look really horrible as soon as somebody also tries
> to add support for virtio-mmio here later ;-)

Do all virtio transports have support for 9p, though? I thought it was
only virtio-pci and virtio-ccw...

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Thomas Huth 6 years, 7 months ago
On 09.08.2017 10:27, Cornelia Huck wrote:
> On Wed, 9 Aug 2017 10:23:04 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 09.08.2017 09:17, Cornelia Huck wrote:
>>> Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
>>> on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
>>> device.
>>>
>>> Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
>>> CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> Changes v1->v2: drop extraneous spaces, fix build on cris
>>>
>>> ---
>>>  default-configs/s390x-softmmu.mak | 1 +
>>>  fsdev/Makefile.objs               | 9 +++------
>>>  hw/Makefile.objs                  | 2 +-
>>>  3 files changed, 5 insertions(+), 7 deletions(-)
[...]
>>
>> Patch should be fine now, I think...
>>
>> But thinking about this again, I wonder whether it would be enough to
>> simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
>> sufficient to assert that there is also at least one kind of virtio
>> transport available, right?
>> Otherwise this will look really horrible as soon as somebody also tries
>> to add support for virtio-mmio here later ;-)
> 
> Do all virtio transports have support for 9p, though? I thought it was
> only virtio-pci and virtio-ccw...

While virtio-pci and virtio-ccw seem to require separate dedicated
devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
virtio-mmio seems to work different. As far as I can see, there are no
dedicated virtio-xxx-mmio devices in the code at all. According to
https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
you simply have to use virtio-xxx-device here instead. And a
virtio-9p-device is available. So theoretically, the 9p code should work
with virtio-mmio, too, or is there a problem that I did not see yet?

Anyway, we likely should not blindly enable this, so unless somebody has
a setup to test it, we should go with your current patch instead, I think.

 Thomas

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 11:07:38 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 09.08.2017 10:27, Cornelia Huck wrote:
> > On Wed, 9 Aug 2017 10:23:04 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 09.08.2017 09:17, Cornelia Huck wrote:  
> >>> Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> >>> on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> >>> device.
> >>>
> >>> Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> >>> CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>
> >>> Changes v1->v2: drop extraneous spaces, fix build on cris
> >>>
> >>> ---
> >>>  default-configs/s390x-softmmu.mak | 1 +
> >>>  fsdev/Makefile.objs               | 9 +++------
> >>>  hw/Makefile.objs                  | 2 +-
> >>>  3 files changed, 5 insertions(+), 7 deletions(-)  
> [...]
> >>
> >> Patch should be fine now, I think...
> >>
> >> But thinking about this again, I wonder whether it would be enough to
> >> simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> >> sufficient to assert that there is also at least one kind of virtio
> >> transport available, right?
> >> Otherwise this will look really horrible as soon as somebody also tries
> >> to add support for virtio-mmio here later ;-)  
> > 
> > Do all virtio transports have support for 9p, though? I thought it was
> > only virtio-pci and virtio-ccw...  
> 
> While virtio-pci and virtio-ccw seem to require separate dedicated
> devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
> virtio-mmio seems to work different. As far as I can see, there are no
> dedicated virtio-xxx-mmio devices in the code at all. According to
> https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
> you simply have to use virtio-xxx-device here instead. And a
> virtio-9p-device is available. So theoretically, the 9p code should work
> with virtio-mmio, too, or is there a problem that I did not see yet?
> 
> Anyway, we likely should not blindly enable this, so unless somebody has
> a setup to test it, we should go with your current patch instead, I think.

Yes, I'd prefer if somebody with a virtio-mmio setup could chime in.

Given the current Makefiles, this cannot have worked for !pci anyway...

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Daniel P. Berrange 6 years, 7 months ago
On Wed, Aug 09, 2017 at 11:07:38AM +0200, Thomas Huth wrote:
> On 09.08.2017 10:27, Cornelia Huck wrote:
> > On Wed, 9 Aug 2017 10:23:04 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> > 
> >> On 09.08.2017 09:17, Cornelia Huck wrote:
> >>> Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> >>> on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> >>> device.
> >>>
> >>> Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> >>> CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>
> >>> Changes v1->v2: drop extraneous spaces, fix build on cris
> >>>
> >>> ---
> >>>  default-configs/s390x-softmmu.mak | 1 +
> >>>  fsdev/Makefile.objs               | 9 +++------
> >>>  hw/Makefile.objs                  | 2 +-
> >>>  3 files changed, 5 insertions(+), 7 deletions(-)
> [...]
> >>
> >> Patch should be fine now, I think...
> >>
> >> But thinking about this again, I wonder whether it would be enough to
> >> simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> >> sufficient to assert that there is also at least one kind of virtio
> >> transport available, right?
> >> Otherwise this will look really horrible as soon as somebody also tries
> >> to add support for virtio-mmio here later ;-)
> > 
> > Do all virtio transports have support for 9p, though? I thought it was
> > only virtio-pci and virtio-ccw...
> 
> While virtio-pci and virtio-ccw seem to require separate dedicated
> devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
> virtio-mmio seems to work different. As far as I can see, there are no
> dedicated virtio-xxx-mmio devices in the code at all. According to
> https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
> you simply have to use virtio-xxx-device here instead. And a
> virtio-9p-device is available. So theoretically, the 9p code should work
> with virtio-mmio, too, or is there a problem that I did not see yet?
> 
> Anyway, we likely should not blindly enable this, so unless somebody has
> a setup to test it, we should go with your current patch instead, I think.

qemu-system-arm supports virtio-mmio so you can use that to test it


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 10:24:13 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Aug 09, 2017 at 11:07:38AM +0200, Thomas Huth wrote:
> > On 09.08.2017 10:27, Cornelia Huck wrote:  
> > > On Wed, 9 Aug 2017 10:23:04 +0200
> > > Thomas Huth <thuth@redhat.com> wrote:
> > >   
> > >> On 09.08.2017 09:17, Cornelia Huck wrote:  
> > >>> Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> > >>> on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> > >>> device.
> > >>>
> > >>> Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> > >>> CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> > >>>
> > >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > >>> ---
> > >>>
> > >>> Changes v1->v2: drop extraneous spaces, fix build on cris
> > >>>
> > >>> ---
> > >>>  default-configs/s390x-softmmu.mak | 1 +
> > >>>  fsdev/Makefile.objs               | 9 +++------
> > >>>  hw/Makefile.objs                  | 2 +-
> > >>>  3 files changed, 5 insertions(+), 7 deletions(-)  
> > [...]  
> > >>
> > >> Patch should be fine now, I think...
> > >>
> > >> But thinking about this again, I wonder whether it would be enough to
> > >> simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> > >> sufficient to assert that there is also at least one kind of virtio
> > >> transport available, right?
> > >> Otherwise this will look really horrible as soon as somebody also tries
> > >> to add support for virtio-mmio here later ;-)  
> > > 
> > > Do all virtio transports have support for 9p, though? I thought it was
> > > only virtio-pci and virtio-ccw...  
> > 
> > While virtio-pci and virtio-ccw seem to require separate dedicated
> > devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
> > virtio-mmio seems to work different. As far as I can see, there are no
> > dedicated virtio-xxx-mmio devices in the code at all. According to
> > https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
> > you simply have to use virtio-xxx-device here instead. And a
> > virtio-9p-device is available. So theoretically, the 9p code should work
> > with virtio-mmio, too, or is there a problem that I did not see yet?
> > 
> > Anyway, we likely should not blindly enable this, so unless somebody has
> > a setup to test it, we should go with your current patch instead, I think.  
> 
> qemu-system-arm supports virtio-mmio so you can use that to test it

Hm, the default config for arm enables CONFIG_PCI, so machines using
virtio-mmio and 9p would be broken with that patch... should we rather
depend on PCI || VIRTIO_CCW?

(Any arches not enabling PCI that use virtio-mmio? Or is arm the only
user of virtio-mmio?)

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Peter Maydell 6 years, 7 months ago
On 9 August 2017 at 10:07, Thomas Huth <thuth@redhat.com> wrote:
> While virtio-pci and virtio-ccw seem to require separate dedicated
> devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,

You don't *have* to use the dedicated virtio-foo-pci device;
if you like you can manually plug together the virtio-pci
transport and the virtio-foo-device backend yourself. The
'fused' device is just for convenience and compatibility
with existing command lines. There is no fused device for
virtio-mmio because the board itself creates the transport
devices so the user only needs to create the backends
(which then auto-plug into the virtio bus).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Peter Maydell 6 years, 7 months ago
On 9 August 2017 at 10:07, Thomas Huth <thuth@redhat.com> wrote:
> While virtio-pci and virtio-ccw seem to require separate dedicated
> devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
> virtio-mmio seems to work different. As far as I can see, there are no
> dedicated virtio-xxx-mmio devices in the code at all. According to
> https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
> you simply have to use virtio-xxx-device here instead. And a
> virtio-9p-device is available. So theoretically, the 9p code should work
> with virtio-mmio, too, or is there a problem that I did not see yet?
>
> Anyway, we likely should not blindly enable this, so unless somebody has
> a setup to test it, we should go with your current patch instead, I think.

As you say, we already compile the virtio-9p-device that can
plug into any virtio transport. So why not just build it
whenever virtio of any form is enabled? Having it only
build if PCI is also enabled seems very odd: the backend
should not care at all about what transport it is using.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 10:47:18 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 9 August 2017 at 10:07, Thomas Huth <thuth@redhat.com> wrote:
> > While virtio-pci and virtio-ccw seem to require separate dedicated
> > devices (e.g. virtio-9p-pci and virtio-9p-ccw) for everything,
> > virtio-mmio seems to work different. As far as I can see, there are no
> > dedicated virtio-xxx-mmio devices in the code at all. According to
> > https://www.redhat.com/archives/libvir-list/2013-August/msg00009.html
> > you simply have to use virtio-xxx-device here instead. And a
> > virtio-9p-device is available. So theoretically, the 9p code should work
> > with virtio-mmio, too, or is there a problem that I did not see yet?
> >
> > Anyway, we likely should not blindly enable this, so unless somebody has
> > a setup to test it, we should go with your current patch instead, I think.  
> 
> As you say, we already compile the virtio-9p-device that can
> plug into any virtio transport. So why not just build it
> whenever virtio of any form is enabled? Having it only
> build if PCI is also enabled seems very odd: the backend
> should not care at all about what transport it is using.

Given the previous discussions, I think just dropping the PCI
dependency is indeed the way to go. I'll send a v3.

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Greg Kurz 6 years, 7 months ago
On Wed, 9 Aug 2017 10:27:37 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 9 Aug 2017 10:23:04 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 09.08.2017 09:17, Cornelia Huck wrote:  
> > > Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
> > > on CONFIG_VIRTFS and on the presence of an appropriate virtio transport
> > > device.
> > > 
> > > Let's introduce CONFIG_VIRTIO_CCW to cover s390x and check for
> > > CONFIG_VIRTFS && (CONFIG_VIRTIO_PCI || CONFIG_VIRTIO_CCW).
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > > 
> > > Changes v1->v2: drop extraneous spaces, fix build on cris
> > > 
> > > ---
> > >  default-configs/s390x-softmmu.mak | 1 +
> > >  fsdev/Makefile.objs               | 9 +++------
> > >  hw/Makefile.objs                  | 2 +-
> > >  3 files changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> > > index 51191b77df..e4c5236ceb 100644
> > > --- a/default-configs/s390x-softmmu.mak
> > > +++ b/default-configs/s390x-softmmu.mak
> > > @@ -8,3 +8,4 @@ CONFIG_S390_FLIC=y
> > >  CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
> > >  CONFIG_VFIO_CCW=$(CONFIG_LINUX)
> > >  CONFIG_WDT_DIAG288=y
> > > +CONFIG_VIRTIO_CCW=y
> > > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
> > > index 659df6e187..3d157add31 100644
> > > --- a/fsdev/Makefile.objs
> > > +++ b/fsdev/Makefile.objs
> > > @@ -1,10 +1,7 @@
> > > -ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
> > >  # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
> > > -# only pull in the actual virtio-9p device if we also enabled virtio.
> > > -common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> > > -else
> > > -common-obj-y = qemu-fsdev-dummy.o
> > > -endif
> > > +# only pull in the actual virtio-9p device if we also enabled a virtio backend.
> > > +common-obj-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))= qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> > > +common-obj-$(call lnot,$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW)))) = qemu-fsdev-dummy.o
> > >  common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
> > >  
> > >  # Toplevel always builds this; targets without virtio will put it in
> > > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > > index a2c61f6b09..335f26b65e 100644
> > > --- a/hw/Makefile.objs
> > > +++ b/hw/Makefile.objs
> > > @@ -1,4 +1,4 @@
> > > -devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
> > > +devices-dirs-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO_PCI),$(CONFIG_VIRTIO_CCW))) += 9pfs/
> > >  devices-dirs-$(CONFIG_SOFTMMU) += acpi/
> > >  devices-dirs-$(CONFIG_SOFTMMU) += adc/
> > >  devices-dirs-$(CONFIG_SOFTMMU) += audio/    
> > 
> > Patch should be fine now, I think...
> > 
> > But thinking about this again, I wonder whether it would be enough to
> > simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> > sufficient to assert that there is also at least one kind of virtio
> > transport available, right?
> > Otherwise this will look really horrible as soon as somebody also tries
> > to add support for virtio-mmio here later ;-)  
> 

And virtio isn't the only transport for 9p: we also have a Xen backend,
which happen to be built because targets that support Xen also have
CONFIG_PCI I guess.

Cc'ing Stefano and Paolo who had a discussion during the review of
9p Xen backend patches:

https://patchwork.kernel.org/patch/9622325/

> Do all virtio transports have support for 9p, though? I thought it was
> only virtio-pci and virtio-ccw...

Hmm... I don't see any device-specific code in virtio-mmio.. why would it
be different for 9p than for block or net ?
Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 11:47:05 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 9 Aug 2017 10:27:37 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 9 Aug 2017 10:23:04 +0200
> > Thomas Huth <thuth@redhat.com> wrote:

> > > But thinking about this again, I wonder whether it would be enough to
> > > simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> > > sufficient to assert that there is also at least one kind of virtio
> > > transport available, right?
> > > Otherwise this will look really horrible as soon as somebody also tries
> > > to add support for virtio-mmio here later ;-)    
> >   
> 
> And virtio isn't the only transport for 9p: we also have a Xen backend,
> which happen to be built because targets that support Xen also have
> CONFIG_PCI I guess.

Only if they also have virtio enabled, no?

Should the condition be VIRTFS && (VIRTIO || XEN), then?

Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Greg Kurz 6 years, 7 months ago
On Wed, 9 Aug 2017 13:06:14 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 9 Aug 2017 11:47:05 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Wed, 9 Aug 2017 10:27:37 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Wed, 9 Aug 2017 10:23:04 +0200
> > > Thomas Huth <thuth@redhat.com> wrote:  
> 
> > > > But thinking about this again, I wonder whether it would be enough to
> > > > simply check for CONFIG_VIRTIO=y here instead. CONFIG_VIRTIO=y should be
> > > > sufficient to assert that there is also at least one kind of virtio
> > > > transport available, right?
> > > > Otherwise this will look really horrible as soon as somebody also tries
> > > > to add support for virtio-mmio here later ;-)      
> > >     
> > 
> > And virtio isn't the only transport for 9p: we also have a Xen backend,
> > which happen to be built because targets that support Xen also have
> > CONFIG_PCI I guess.  
> 
> Only if they also have virtio enabled, no?
> 

Yes, you're right. This is actually the case for i386 and x86_64 targets,
which seem to be the only that support Xen.

> Should the condition be VIRTFS && (VIRTIO || XEN), then?

That's what I was beginning to think as well :)
Re: [Qemu-devel] [PATCH v2] 9pfs: fix dependencies
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 9 Aug 2017 14:10:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 9 Aug 2017 13:06:14 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:

> > Should the condition be VIRTFS && (VIRTIO || XEN), then?  
> 
> That's what I was beginning to think as well :)

OK, here's what should work:

- fsdev/Makefile.objs needs to check for VIRTFS && (VIRTIO || XEN)
- hw/Makefile.objs needs to check for VIRTFS && (VIRTIO || XEN) before
  building hw/9pfs/
- hw/9pfs/Makefile.objs needs a new VIRTIO check for the virtio device

I'm preparing a patch.