Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x: express dependencies with Kconfig

[Thomas Huth <thuth@redhat.com>]

On 2019-01-28 17:08, Cornelia Huck wrote:
> On Mon, 28 Jan 2019 16:29:26 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> Instead of hard-coding all config switches in the config file
>> default-configs/s390x-softmmu.mak, let's use the new Kconfig files
>> to express the necessary dependencies: The S390_CCW_VIRTIO config switch
>> for the "s390-ccw-virtio" machine now selects all non-optional devices.
>>
>> And since we already have the VIRTIO_PCI and VIRTIO_MMIO config switches
>> for the other two virtio transports, this patch also introduces a new
>> config switch VIRTIO_CCW for the third, s390x-specific virtio transport,
>> so that all three virtio transports are now handled in the same way.
> 
> I haven't found time to look at the Kconfig patches, so take the
> following with a generous amount of salt.
> 
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  This goes on top of the curren Kconfig series:
>>
>>  Based-on: 1548410831-19553-1-git-send-email-pbonzini@redhat.com
>>
>>  default-configs/s390x-softmmu.mak | 12 ++++++++----
>>  hw/s390x/Kconfig                  |  4 ++++
>>  hw/s390x/Makefile.objs            |  2 +-
>>  hw/virtio/Kconfig                 |  4 ++++
>>  4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
>> index 2be5059..08b1683 100644
>> --- a/default-configs/s390x-softmmu.mak
>> +++ b/default-configs/s390x-softmmu.mak
>> @@ -1,9 +1,13 @@
>> -CONFIG_PCI=y
>> +# Default configuration for s390x-softmmu
>> +
>> +# Optional devices:
>> +#
>>  CONFIG_VIRTIO_PCI=y
>> -CONFIG_SCLPCONSOLE=y
>>  CONFIG_TERMINAL3270=y
>> -CONFIG_S390_FLIC=y
>>  CONFIG_WDT_DIAG288=y
>> -CONFIG_S390_CCW_VIRTIO=y
>>  CONFIG_VFIO_CCW=y
>>  CONFIG_VFIO_AP=y
> 
> This looks sensible.
> 
>> +
>> +# Boards:
>> +#
>> +CONFIG_S390_CCW_VIRTIO=y
>> diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
>> index 303db7f..9a36c39 100644
>> --- a/hw/s390x/Kconfig
>> +++ b/hw/s390x/Kconfig
>> @@ -1,2 +1,6 @@
>>  config S390_CCW_VIRTIO
>>      bool
>> +    select PCI
>> +    select S390_FLIC
>> +    select SCLPCONSOLE
>> +    select VIRTIO_CCW
> 
> This also makes sense. (I assume CONFIG_PCI, CONFIG_S390_FLIC, and
> CONFIG_SCLPCONSOLE are already defined in the base patch series.)

Yes, they are.

>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index a884aae..0dc798a 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -8,7 +8,7 @@ obj-y += ipl.o
>>  obj-y += css.o
>>  obj-y += s390-virtio-ccw.o
>>  obj-y += 3270-ccw.o
> 
> Should this depend on TERMINAL3270 (at least in the future)?

Yes, this is related to TERMINAL3270 which is already used for
hw/char/terminal3270. So I think we could use $(CONFIG_TERMINAL3270)
here, too.

>> -obj-y += virtio-ccw.o
>> +obj-$(CONFIG_VIRTIO_CCW) += virtio-ccw.o
>>  obj-$(CONFIG_VIRTIO_SERIAL) += virtio-ccw-serial.o
> 
> Hm. Should the individual device types depend both on VIRTIO_<device>
> and on VIRTIO_CCW? Making VIRTIO_<device> depend on VIRTIO_PCI ||
> VIRTIO_CCW || VIRTIO_MMIO is probably not enough, as we could be trying
> to build virtio-ccw devices if only VIRTIO_PCI is set (or virtio-pci
> devices if only VIRTIO_CCW is set, for that matter.)

You're right, the whole section should be fenced via

ifeq ($(CONFIG_VIRTIO_CCW),y)
...
endif

I'll update my patch accordingly and send a v2.

 Thanks,
  Thomas