Re: [Qemu-arm] [PATCH] hw/vfio: Add CONFIG switches for calxeda-xgmac and amd-xgbe

[Alex Williamson]

On Tue, 31 Jan 2017 19:51:02 +0100
Thomas Huth <address@hidden> wrote:

> On 31.01.2017 18:30, Alex Williamson wrote:
> > On Tue, 31 Jan 2017 17:36:35 +0100
> > Thomas Huth <address@hidden> wrote:
> >   
> >> Both devices seem to be specific to the ARM platform. It's confusing
> >> for the users if they show up on other target architectures, too
> >> (e.g. when the user runs QEMU with "-device ?" to get a list of
> >> supported devices). Thus let's introduce proper configuration switches
> >> so that the devices are only compiled and included when they are
> >> really required.
> >>
> >> Signed-off-by: Thomas Huth <address@hidden>
> >> ---
> >>  default-configs/arm-softmmu.mak | 2 ++
> >>  hw/vfio/Makefile.objs           | 4 ++--
> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/default-configs/arm-softmmu.mak 
> >> b/default-configs/arm-softmmu.mak
> >> index 6de3e16..a78be51 100644
> >> --- a/default-configs/arm-softmmu.mak
> >> +++ b/default-configs/arm-softmmu.mak
> >> @@ -94,6 +94,8 @@ CONFIG_VERSATILE_PCI=y
> >>  CONFIG_VERSATILE_I2C=y
> >>  
> >>  CONFIG_PCI_GENERIC=y
> >> +CONFIG_VFIO_XGMAC=y
> >> +CONFIG_VFIO_AMD_XGBE=y
> >>  
> >>  CONFIG_SDHCI=y
> >>  CONFIG_INTEGRATOR_DEBUG=y
> >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> >> index c25e32b..05e7fbb 100644
> >> --- a/hw/vfio/Makefile.objs
> >> +++ b/hw/vfio/Makefile.objs
> >> @@ -2,7 +2,7 @@ ifeq ($(CONFIG_LINUX), y)
> >>  obj-$(CONFIG_SOFTMMU) += common.o
> >>  obj-$(CONFIG_PCI) += pci.o pci-quirks.o
> >>  obj-$(CONFIG_SOFTMMU) += platform.o
> >> -obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> >> -obj-$(CONFIG_SOFTMMU) += amd-xgbe.o
> >> +obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
> >> +obj-$(CONFIG_VFIO_AMD_XGBE) += amd-xgbe.o
> >>  obj-$(CONFIG_SOFTMMU) += spapr.o
> >>  endif  
> > 
> > I can't say that I fully agree that this is a good idea.  How many
> > users are actually confused by this, versus the benefit of ensuring
> > that it builds across all architectures?  
> 
> Why do you want this to be build on all architectures? The devices are
> only available on ARM as far as I know, so I can't see any real use for
> this. And it slows down compilation time, too, if we compile it
> everywhere (well, a little bit at least ;-)).
> 
> > Do we want to make platform also specific to ARM  
> 
> I did not notice that one yet, since it does not show up in the "-device
> ?" help text (it's apparently an abstract device), so it also does not
> really bother me.
> But if I remove the device from the build, I get a funny error when I
> try to view the device help text:
> 
> $ qemu-system-tricore -device ?
> **
> ERROR:/home/thuth/devel/qemu/qom/object.c:168:type_get_parent: assertion
> failed: (type->parent_type != NULL)
> Aborted (core dumped)
> 
> So I guess we should rather keep that one for now.
> 
> > and spapr specific to ppc64?  
> 
> I already tried that (with the already existing CONFIG_PSERIES), but the
> code in common.c depends on the code in spapr.c, so it can't be removed
> without reworking the code quite a bit. But spapr.c also does not really
> bug me, since it does not register a type that shows up in the "-device
> ?" help text, so I think it is not worth the effort here.

Ok, I guess my only real objection is that I need to build multiple
targets in order to compile test all of vfio rather than getting that
for free with only building x86_64 for development and only building the
full target list prior to a submitting a pull request.  It's a bit
selfish to impose those extra files on other targets for time and space
reasons, so I guess I don't have any substantive objection to this.
I'll queue it for my next pull request.  Thanks,

Alex