diff mbox

[v2] 9pfs: fix dependencies

Message ID 20170809071718.17924-1-cohuck@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck Aug. 9, 2017, 7:17 a.m. UTC
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(-)

Comments

Thomas Huth Aug. 9, 2017, 8:23 a.m. UTC | #1
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
Cornelia Huck Aug. 9, 2017, 8:27 a.m. UTC | #2
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...
Thomas Huth Aug. 9, 2017, 9:07 a.m. UTC | #3
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
Cornelia Huck Aug. 9, 2017, 9:13 a.m. UTC | #4
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...
Daniel P. Berrangé Aug. 9, 2017, 9:24 a.m. UTC | #5
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
Cornelia Huck Aug. 9, 2017, 9:43 a.m. UTC | #6
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?)
Peter Maydell Aug. 9, 2017, 9:44 a.m. UTC | #7
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
Greg Kurz Aug. 9, 2017, 9:47 a.m. UTC | #8
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 ?
Peter Maydell Aug. 9, 2017, 9:47 a.m. UTC | #9
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
Cornelia Huck Aug. 9, 2017, 9:52 a.m. UTC | #10
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.
Cornelia Huck Aug. 9, 2017, 11:06 a.m. UTC | #11
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?
Greg Kurz Aug. 9, 2017, 12:10 p.m. UTC | #12
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 :)
Cornelia Huck Aug. 9, 2017, 4:02 p.m. UTC | #13
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 mbox

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/