[PATCH v2 00/10] Kconfig vs. default devices

[PATCH v2 00/10] Kconfig vs. default devices

[Fabiano Rosas]

v2:
Applying the feedback received, all small tweaks.

Patch 6 still needs consensus on whether to apply the fix to Kconfig
or elsewhere. Link to the previous version:
https://lore.kernel.org/r/461ba038-31bf-49c4-758b-94ece36f136f@redhat.com

changelog:

- patch 1: moved isa-parallel to a build time check like the other
           patches;
- patch 3: tweaked commit message;
- patch 7: removed the default from XLNX_USB_SUBSYS.

v1:
https://lore.kernel.org/r/20230206140809.26028-1-farosas@suse.de

We currently have a situation where disabling a Kconfig might result
in a runtime error when QEMU selects the corresponding device as a
default value for an option. But first a disambiguation:

Kconfig default::
  a device "Foo" for which there's "config FOO default y" or "config X
  imply FOO" in Kconfig.

QEMU hardcoded default::
  a fallback; a device "Foo" that is chosen in case no corresponding
  option is given in the command line.

The issue I'm trying to solve is that there is no link between the two
"defaults" above, which means that when the user at build time
de-selects a Kconfig default, either via configs/devices/*/*.mak or
--without-default-devices, the subsequent invocation at runtime might
continue to try to create the missing device due to QEMU defaults.

Even a experienced user that tweaks the build via .mak files is not
required to know about what QEMU developers chose to use as fallbacks
in the code. Moreover, the person/entity that builds the code might
not be the same that runs it, which makes it even more confusing.

We do have -nodefaults in the command line, but that doesn't include
all types of fallbacks that might be set in the code. It also does not
cover individual CONFIGs and their respective use as a fallback in the
code.

So my proposal here is actually simple: Let's make sure every fallback
device creation *without* a validation check gets a hard dependency in
Kconfig. A validation check being something like:

if (has_defaults && object_get_class("foo") {
   create_foo();
}

Fabiano Rosas (10):
  hw/i386: Select CONFIG_PARALLEL for PC machines
  hw/i386: Select E1000E for q35
  hw/i386: Select VGA_PCI in Kconfig
  hw/i386: Select E1000_PCI for i440fx
  hw/arm: Select VIRTIO_NET for virt machine
  hw/arm: Select VIRTIO_BLK for virt machine
  hw/arm: Select XLNX_USB_SUBSYS for xlnx-zcu102 machine
  hw/arm: Select GICV3_TCG for sbsa-ref machine
  hw/arm: Select e1000e for sbsa-ref machine
  hw/arm: Select VGA_PCI for sbsa-ref machine

 hw/arm/Kconfig  | 7 +++++++
 hw/i386/Kconfig | 8 ++++----
 hw/usb/Kconfig  | 1 -
 3 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.35.3

[PATCH v2 04/10] hw/i386: Select E1000_PCI for i440fx

[Fabiano Rosas]

The i440fx machines uses the e1000 adapter as the default when no
other network card is configured.

Move the E1000_PCI entry in Kconfig from 'imply' to 'select' to avoid
the following situation:

./qemu-system-i386 -machine pc-i440fx-8.0
qemu-system-i386: Unsupported NIC model: e1000

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/i386/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 36590e2691..e98af283b4 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -66,7 +66,6 @@ config PC_ACPI
 
 config I440FX
     bool
-    imply E1000_PCI
     imply VMPORT
     imply VMMOUSE
     select ACPI_PIIX4
@@ -78,6 +77,7 @@ config I440FX
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
+    select E1000_PCI
 
 config ISAPC
     bool
-- 
2.35.3

[PATCH v2 05/10] hw/arm: Select VIRTIO_NET for virt machine

[Fabiano Rosas]

The 'virt' machine uses virtio-net-pci as a fallback when no other
network driver has been selected via command line. Select VIRTIO_NET
and VIRTIO_PCI from CONFIG_ARM_VIRT to avoid errors when PCI_DEVICES=n
(due to e.g. --without-default-devices):

$ ./qemu-system-aarch64 -M virt -accel tcg -cpu max
qemu-system-aarch64: Unsupported NIC model: virtio-net-pci

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 2d157de9b8..8dcc08b7ec 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -31,6 +31,8 @@ config ARM_VIRT
     select VIRTIO_MEM_SUPPORTED
     select ACPI_CXL
     select ACPI_HMAT
+    select VIRTIO_PCI
+    select VIRTIO_NET
 
 config CHEETAH
     bool
-- 
2.35.3

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Philippe Mathieu-Daudé]

On 8/2/23 20:26, Fabiano Rosas wrote:

> We currently have a situation where disabling a Kconfig might result
> in a runtime error when QEMU selects the corresponding device as a
> default value for an option. But first a disambiguation:
> 
> Kconfig default::
>    a device "Foo" for which there's "config FOO default y" or "config X
>    imply FOO" in Kconfig.
> 
> QEMU hardcoded default::
>    a fallback; a device "Foo" that is chosen in case no corresponding
>    option is given in the command line.
> 
> The issue I'm trying to solve is that there is no link between the two
> "defaults" above, which means that when the user at build time
> de-selects a Kconfig default, either via configs/devices/*/*.mak or
> --without-default-devices, the subsequent invocation at runtime might
> continue to try to create the missing device due to QEMU defaults.

This will keep bitrotting if we don't cover such configs in our CI.

Why do you want to get this fixed BTW? I'm not sure there is a big
interest (as in "almost no users").

I tried to do that few years ago [*] and Thomas said:

"in our CI, we should test what users really need,
  and not each and every distantly possible combination."

[*] 
https://lore.kernel.org/qemu-devel/81aca179-4320-f00b-d9dc-7eca449ebce7@redhat.com/

> Fabiano Rosas (10):
>    hw/i386: Select CONFIG_PARALLEL for PC machines
>    hw/i386: Select E1000E for q35
>    hw/i386: Select VGA_PCI in Kconfig
>    hw/i386: Select E1000_PCI for i440fx
>    hw/arm: Select VIRTIO_NET for virt machine
>    hw/arm: Select VIRTIO_BLK for virt machine
>    hw/arm: Select XLNX_USB_SUBSYS for xlnx-zcu102 machine
>    hw/arm: Select GICV3_TCG for sbsa-ref machine
>    hw/arm: Select e1000e for sbsa-ref machine
>    hw/arm: Select VGA_PCI for sbsa-ref machine
> 
>   hw/arm/Kconfig  | 7 +++++++
>   hw/i386/Kconfig | 8 ++++----
>   hw/usb/Kconfig  | 1 -
>   3 files changed, 11 insertions(+), 5 deletions(-)
> 

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Thomas Huth]

On 08/02/2023 20.43, Philippe Mathieu-Daudé wrote:
> On 8/2/23 20:26, Fabiano Rosas wrote:
> 
>> We currently have a situation where disabling a Kconfig might result
>> in a runtime error when QEMU selects the corresponding device as a
>> default value for an option. But first a disambiguation:
>>
>> Kconfig default::
>>    a device "Foo" for which there's "config FOO default y" or "config X
>>    imply FOO" in Kconfig.
>>
>> QEMU hardcoded default::
>>    a fallback; a device "Foo" that is chosen in case no corresponding
>>    option is given in the command line.
>>
>> The issue I'm trying to solve is that there is no link between the two
>> "defaults" above, which means that when the user at build time
>> de-selects a Kconfig default, either via configs/devices/*/*.mak or
>> --without-default-devices, the subsequent invocation at runtime might
>> continue to try to create the missing device due to QEMU defaults.
> 
> This will keep bitrotting if we don't cover such configs in our CI.
> 
> Why do you want to get this fixed BTW? I'm not sure there is a big
> interest (as in "almost no users").
> 
> I tried to do that few years ago [*] and Thomas said:
> 
> "in our CI, we should test what users really need,
>   and not each and every distantly possible combination."

You're mis-quoting me here. That comment was made when we were talking about 
very arbitrary configs that likely nobody is going to use.
Fabiano's series here is about the --without-default-devices configure 
option which everybody could add to their set of "configure" options easily.

  Thomas

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 08/02/2023 20.43, Philippe Mathieu-Daudé wrote:
>> On 8/2/23 20:26, Fabiano Rosas wrote:
>> 
>>> We currently have a situation where disabling a Kconfig might result
>>> in a runtime error when QEMU selects the corresponding device as a
>>> default value for an option. But first a disambiguation:
>>>
>>> Kconfig default::
>>>    a device "Foo" for which there's "config FOO default y" or "config X
>>>    imply FOO" in Kconfig.
>>>
>>> QEMU hardcoded default::
>>>    a fallback; a device "Foo" that is chosen in case no corresponding
>>>    option is given in the command line.
>>>
>>> The issue I'm trying to solve is that there is no link between the two
>>> "defaults" above, which means that when the user at build time
>>> de-selects a Kconfig default, either via configs/devices/*/*.mak or
>>> --without-default-devices, the subsequent invocation at runtime might
>>> continue to try to create the missing device due to QEMU defaults.
>> This will keep bitrotting if we don't cover such configs in our CI.
>> Why do you want to get this fixed BTW? I'm not sure there is a big
>> interest (as in "almost no users").
>> I tried to do that few years ago [*] and Thomas said:
>> "in our CI, we should test what users really need,
>>   and not each and every distantly possible combination."
>
> You're mis-quoting me here. That comment was made when we were talking
> about very arbitrary configs that likely nobody is going to use.
> Fabiano's series here is about the --without-default-devices configure
> option which everybody could add to their set of "configure" options
> easily.

Indeed - while trying to reduce the compile time I ran into this with a
plain --without-default-devices check. We also have in the meantime
introduced --with-devices-FOO so we can do minimal builds.

>
>  Thomas


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 01/10] hw/i386: Select CONFIG_PARALLEL for PC machines

[Thomas Huth]

On 08/02/2023 20.26, Fabiano Rosas wrote:
> Currently the isa-parallel driver is always added by the PC machines
> regardless of the presence of the actual code in the build, which can
> lead to a crash:
> 
> qemu-system-i386: unknown type 'isa-parallel'
> Aborted (core dumped)
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   hw/i386/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index 1bf47b0b0b..d3c340e053 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -20,7 +20,6 @@ config PC
>       imply PCI_IPMI_BT
>       imply IPMI_SSIF
>       imply ISA_DEBUG
> -    imply PARALLEL
>       imply PCI_DEVICES
>       imply PVPANIC_ISA
>       imply QXL
> @@ -46,6 +45,7 @@ config PC
>       select ACPI_VMGENID
>       select VIRTIO_PMEM_SUPPORTED
>       select VIRTIO_MEM_SUPPORTED
> +    select PARALLEL
>   
>   config PC_PCI
>       bool

Phew ... Ack from a plain upstream point of view. From a Red Hat downstream 
point of view, this will cause another downstream-only patch for us, since 
the binaries (and the machine types) that we have in RHEL have the 
"isa-parallel" device not compiled in on purpose.

So I started wondering now whether we could tackle all this a little bit 
different, in a more flexible way ... something similar like you did in your 
parallel port patch in v1 / something similar to what Peter suggested in his 
option (2) here:

https://lore.kernel.org/qemu-devel/CAFEAcA9VkFU_bh=aBAOoXCUCeSm1xuR+H+uerd468=vVuDrJEg@mail.gmail.com/ 


For display devices, we already have default_display in MachineClass, so 
instead of always selecting the VGA device in your "Select VGA_PCI in 
Kconfig" patch, we could check that in vl.c and set default_vga = 0 if it is 
not available.

For network devices, there is already default_nic_model in PCMachineClass 
... we could move that to the generic MachineClass and use it in vl.c according.

Then no_parallel, no_serial, no_floppy, no_cdrom, no_sdcard in the 
MachineClass could be replaced by a "char *default_XYZ", too, so that we 
rather have a device name here to indicate the availability of a default 
device than a boolean flag. If the pointer is not set ==> no default device. 
If the pointer is set and the device is available ==> use the default 
device. If the pointer is set and the device is not compiled in ==> emit a 
warning, but continue as if the pointer was not set.

What do you think?

  Thomas

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Alex Bennée]

Fabiano Rosas <farosas@suse.de> writes:

> v2:
> Applying the feedback received, all small tweaks.
>
> Patch 6 still needs consensus on whether to apply the fix to Kconfig
> or elsewhere. Link to the previous version:
> https://lore.kernel.org/r/461ba038-31bf-49c4-758b-94ece36f136f@redhat.com
>
> changelog:
>
> - patch 1: moved isa-parallel to a build time check like the other
>            patches;
> - patch 3: tweaked commit message;
> - patch 7: removed the default from XLNX_USB_SUBSYS.

I've queued the ARM tweaks to testing/next where I can add a test for it
in the end. I'll leave the x86 stuff for discussion of the more complete
solution that avoids hacky downstream patching.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro