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

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

[Thomas Huth]

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 <thuth@redhat.com>
---
 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
-- 
1.8.3.1

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

[Alex Williamson]

On Tue, 31 Jan 2017 17:36:35 +0100
Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
> ---
>  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?  Do we want to make platform
also specific to ARM and spapr specific to ppc64?  Thanks,

Alex

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

[Thomas Huth]

On 31.01.2017 18:30, Alex Williamson wrote:
> On Tue, 31 Jan 2017 17:36:35 +0100
> Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
>> ---
>>  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.

 Thomas

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

[Auger Eric]

Hi Thomas,

On 31/01/2017 19:51, Thomas Huth wrote:
> On 31.01.2017 18:30, Alex Williamson wrote:
>> On Tue, 31 Jan 2017 17:36:35 +0100
>> Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
>>> ---
>>>  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.

FYI I will send patches to make VFIO platform device non abstract.

Thanks

Eric
> 
>> 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.
> 
>  Thomas
> 

Re: [Qemu-devel] [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 <thuth@redhat.com> wrote:

> On 31.01.2017 18:30, Alex Williamson wrote:
> > On Tue, 31 Jan 2017 17:36:35 +0100
> > Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
> >> ---
> >>  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