| Message ID | 20170809071718.17924-1-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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...
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
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...
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
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?)
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
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 ?
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
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.
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?
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 :)
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.
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/
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(-)