| Message ID | 20230208192654.8854-1-farosas@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Kconfig vs. default devices | expand |
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(-) >
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
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
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
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.
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(-)