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

[Cornelia Huck <cohuck@redhat.com>]

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.)

> 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)?

> -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.)

Or maybe I simply need to read the patches and find the answer there :)

>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio-ccw-balloon.o
>  obj-$(CONFIG_VIRTIO_SCSI) += virtio-ccw-scsi.o
> diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
> index 74f4573..e0452de 100644
> --- a/hw/virtio/Kconfig
> +++ b/hw/virtio/Kconfig
> @@ -16,6 +16,10 @@ config VIRTIO_MMIO
>      bool
>      select VIRTIO
>  
> +config VIRTIO_CCW
> +    bool
> +    select VIRTIO
> +
>  config VIRTIO_BALLOON
>      bool
>      default y