[PATCH 00/10] Kconfig vs. default devices

[PATCH 00/10] Kconfig vs. default devices

[Fabiano Rosas]

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):
  vl.c: Do not add isa-parallel if it's not present
  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 | 6 +++---
 softmmu/vl.c    | 3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.35.3

[PATCH 01/10] vl.c: Do not add isa-parallel if it's not present

[Fabiano Rosas]

Currently the isa-parallel driver is always added by default
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)

Check for the presence of the QOM class and do not include
isa-parallel by default if it's not found.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 softmmu/vl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 9177d95d4e..614e6cf66e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1269,7 +1269,8 @@ static void qemu_disable_default_devices(void)
     if (!has_defaults || machine_class->no_serial) {
         default_serial = 0;
     }
-    if (!has_defaults || machine_class->no_parallel) {
+    if (!has_defaults || machine_class->no_parallel ||
+        !object_class_by_name("isa-parallel")) {
         default_parallel = 0;
     }
     if (!has_defaults || machine_class->no_floppy) {
-- 
2.35.3

[PATCH 02/10] hw/i386: Select E1000E for q35

[Fabiano Rosas]

The e1000e network adapter is the default network card for the q35
machine. Make sure that CONFIG is always selected for that machine.

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..527b95df81 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -93,7 +93,6 @@ config Q35
     bool
     imply VTD
     imply AMD_IOMMU
-    imply E1000E_PCI_EXPRESS
     imply VMPORT
     imply VMMOUSE
     select PC_PCI
@@ -104,6 +103,7 @@ config Q35
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
+    select E1000E_PCI_EXPRESS
 
 config MICROVM
     bool
-- 
2.35.3

[PATCH 03/10] hw/i386: Select VGA_PCI in Kconfig

[Fabiano Rosas]

Machines that have "std" as default VGA display need to always build
vga-pci.c, otherwise we get a crash when CONFIG_PCI_DEVICES=n:

$ ./qemu-system-x86_64 -M q35 -vga std
qemu-system-x86_64: unknown type 'VGA'
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 527b95df81..8e59cb6634 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -30,9 +30,9 @@ config PC
     imply TEST_DEVICES
     imply TPM_CRB
     imply TPM_TIS_ISA
-    imply VGA_PCI
     imply VIRTIO_VGA
     imply NVDIMM
+    select VGA_PCI
     select FDC_ISA
     select I8259
     select I8254
-- 
2.35.3

[PATCH 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

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 8e59cb6634..343783ea3e 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 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

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

[PATCH 06/10] hw/arm: Select VIRTIO_BLK for virt machine

[Fabiano Rosas]

The virt machine has IF_VIRTIO as block_default_type, which causes the
generic code to try to create a virtio-blk-pci device pair at
configure_blockdev()/qemu_create_cli_devices().

Select VIRTIO_BLK 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 -nodefaults -cdrom foo.qcow2
qemu-system-aarch64: -cdrom foo.qcow2: 'virtio-blk' (alias
'virtio-blk-pci') is not a valid device model name

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 8dcc08b7ec..296d4f5176 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -33,6 +33,7 @@ config ARM_VIRT
     select ACPI_HMAT
     select VIRTIO_PCI
     select VIRTIO_NET
+    select VIRTIO_BLK
 
 config CHEETAH
     bool
-- 
2.35.3

[PATCH 07/10] hw/arm: Select XLNX_USB_SUBSYS for xlnx-zcu102 machine

[Fabiano Rosas]

This machine hardcodes initialization of the USB device, so select the
corresponding Kconfig. It is not enough to have it as "default y if
XLNX_VERSAL" at usb/Kconfig because building --without-default-devices
disables the default selection resulting in:

$ ./qemu-system-aarch64 -M xlnx-zcu102
qemu-system-aarch64: missing object type 'usb_dwc3'
Aborted (core dumped)

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 296d4f5176..552e3d04ee 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -405,6 +405,7 @@ config XLNX_VERSAL
     select OR_IRQ
     select XLNX_BBRAM
     select XLNX_EFUSE_VERSAL
+    select XLNX_USB_SUBSYS
 
 config NPCM7XX
     bool
-- 
2.35.3

[PATCH 08/10] hw/arm: Select GICV3_TCG for sbsa-ref machine

[Fabiano Rosas]

This machine hardcodes the creation of the interrupt controller, so
make sure the dependency is explicitly described in the Kconfig.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 552e3d04ee..823f8b11f1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -232,6 +232,7 @@ config SBSA_REF
     select PL061 # GPIO
     select USB_EHCI_SYSBUS
     select WDT_SBSA
+    select ARM_GICV3_TCG
 
 config SABRELITE
     bool
-- 
2.35.3

[PATCH 09/10] hw/arm: Select e1000e for sbsa-ref machine

[Fabiano Rosas]

This machine explicitly selects the e1000e network adapter if no other
option was given in the command line. Make sure e1000e is present in
the build.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 823f8b11f1..5022d519ea 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -233,6 +233,7 @@ config SBSA_REF
     select USB_EHCI_SYSBUS
     select WDT_SBSA
     select ARM_GICV3_TCG
+    select E1000E_PCI_EXPRESS
 
 config SABRELITE
     bool
-- 
2.35.3

[PATCH 10/10] hw/arm: Select VGA_PCI for sbsa-ref machine

[Fabiano Rosas]

The sbsa-ref machine explicitly creates a VGA PCI device, so make sure
vga-pci.c is included in the build.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 5022d519ea..74fceb419d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -234,6 +234,7 @@ config SBSA_REF
     select WDT_SBSA
     select ARM_GICV3_TCG
     select E1000E_PCI_EXPRESS
+    select VGA_PCI
 
 config SABRELITE
     bool
-- 
2.35.3

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

[Peter Maydell]

On Mon, 6 Feb 2023 at 14:14, Fabiano Rosas <farosas@suse.de> 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.
>
> 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();
> }

So we could do one of several things to resolve any particular
one of these:
 (1) make the Kconfig force the device to be compiled in
 (2) make QEMU silently fall back to "don't provide device"
     rather than "provide this default device" if the
     device isn't compiled in
 (3) make QEMU emit an error message saying that the
     command line implies that the machine should have
     (default) device X but X support wasn't compiled in

I don't strongly care which approach we take, but shouldn't
we be consistent about what we do? AFAICT your patch 1
here takes approach (2) but the others take approach (1).

thanks
-- PMM

Re: [PATCH 07/10] hw/arm: Select XLNX_USB_SUBSYS for xlnx-zcu102 machine

[Peter Maydell]

On Mon, 6 Feb 2023 at 14:10, Fabiano Rosas <farosas@suse.de> wrote:
>
> This machine hardcodes initialization of the USB device, so select the
> corresponding Kconfig. It is not enough to have it as "default y if
> XLNX_VERSAL" at usb/Kconfig because building --without-default-devices
> disables the default selection resulting in:
>
> $ ./qemu-system-aarch64 -M xlnx-zcu102
> qemu-system-aarch64: missing object type 'usb_dwc3'
> Aborted (core dumped)
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  hw/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 296d4f5176..552e3d04ee 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -405,6 +405,7 @@ config XLNX_VERSAL
>      select OR_IRQ
>      select XLNX_BBRAM
>      select XLNX_EFUSE_VERSAL
> +    select XLNX_USB_SUBSYS
>
>  config NPCM7XX
>      bool

Shouldn't we also remove the now-useless
"default y if XNLX_VERSAL" line from the XLNX_USB_SUBSYS
stanza ?

thanks
-- PMM

Re: [PATCH 08/10] hw/arm: Select GICV3_TCG for sbsa-ref machine

[Peter Maydell]

On Mon, 6 Feb 2023 at 14:10, Fabiano Rosas <farosas@suse.de> wrote:
>
> This machine hardcodes the creation of the interrupt controller, so
> make sure the dependency is explicitly described in the Kconfig.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  hw/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 552e3d04ee..823f8b11f1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -232,6 +232,7 @@ config SBSA_REF
>      select PL061 # GPIO
>      select USB_EHCI_SYSBUS
>      select WDT_SBSA
> +    select ARM_GICV3_TCG
>
>  config SABRELITE
>      bool
> --
> 2.35.3

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 10/10] hw/arm: Select VGA_PCI for sbsa-ref machine

[Peter Maydell]

On Mon, 6 Feb 2023 at 14:10, Fabiano Rosas <farosas@suse.de> wrote:
>
> The sbsa-ref machine explicitly creates a VGA PCI device, so make sure
> vga-pci.c is included in the build.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  hw/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 5022d519ea..74fceb419d 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -234,6 +234,7 @@ config SBSA_REF
>      select WDT_SBSA
>      select ARM_GICV3_TCG
>      select E1000E_PCI_EXPRESS
> +    select VGA_PCI
>
>  config SABRELITE
>      bool

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 6 Feb 2023 at 14:14, Fabiano Rosas <farosas@suse.de> 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.
>>
>> 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();
>> }
>
> So we could do one of several things to resolve any particular
> one of these:
>  (1) make the Kconfig force the device to be compiled in
>  (2) make QEMU silently fall back to "don't provide device"
>      rather than "provide this default device" if the
>      device isn't compiled in
>  (3) make QEMU emit an error message saying that the
>      command line implies that the machine should have
>      (default) device X but X support wasn't compiled in
>
> I don't strongly care which approach we take, but shouldn't
> we be consistent about what we do? AFAICT your patch 1
> here takes approach (2) but the others take approach (1).

Good point. The one from patch 1 (isa-parallel) is a bit different since
it is used in the -nodefaults logic, but I could probably use the (1)
approach with it as well, let me give it a try.

Re: [PATCH 01/10] vl.c: Do not add isa-parallel if it's not present

[Philippe Mathieu-Daudé]

On 6/2/23 15:08, Fabiano Rosas wrote:
> Currently the isa-parallel driver is always added by default
> 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)
> 
> Check for the presence of the QOM class and do not include
> isa-parallel by default if it's not found.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   softmmu/vl.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 9177d95d4e..614e6cf66e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1269,7 +1269,8 @@ static void qemu_disable_default_devices(void)
>       if (!has_defaults || machine_class->no_serial) {
>           default_serial = 0;
>       }
> -    if (!has_defaults || machine_class->no_parallel) {
> +    if (!has_defaults || machine_class->no_parallel ||
> +        !object_class_by_name("isa-parallel")) {
>           default_parallel = 0;
>       }
>       if (!has_defaults || machine_class->no_floppy) {

How is isa-parallel different, why not the other defaults?

Re: [PATCH 01/10] vl.c: Do not add isa-parallel if it's not present

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 6/2/23 15:08, Fabiano Rosas wrote:
>> Currently the isa-parallel driver is always added by default
>> 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)
>> 
>> Check for the presence of the QOM class and do not include
>> isa-parallel by default if it's not found.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   softmmu/vl.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 9177d95d4e..614e6cf66e 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -1269,7 +1269,8 @@ static void qemu_disable_default_devices(void)
>>       if (!has_defaults || machine_class->no_serial) {
>>           default_serial = 0;
>>       }
>> -    if (!has_defaults || machine_class->no_parallel) {
>> +    if (!has_defaults || machine_class->no_parallel ||
>> +        !object_class_by_name("isa-parallel")) {
>>           default_parallel = 0;
>>       }
>>       if (!has_defaults || machine_class->no_floppy) {
>
> How is isa-parallel different, why not the other defaults?

I doesn't need to be different, I did it like this because I expected to
solve the others in the same way. Peter also flagged the inconsistency,
I'll add a Kconfig dependence like the others.

As to why the other defaults don't have this issue:

serial - for x86, already selected by CONFIG_PC;
         not used on arm;
parallel - for x86, should be selected by CONFIG_PC, I'll fix that on v2;
           not used on arm;
monitor - built in with char.c;
floppy - built in with blockdev.c;
sdcard - built in with blockdev.c;

cdrom - uses mc->block_default_type, so the CONFIG corresponding to that
        device should be selected.
net - verification is only done later during machine init, so the
      machine will need to provide a fallback;
vga - uses mc->default_display, so the CONFIG corresponding to that device
      should be selected.

These last three are addressed in other patches in this series (VGA_PCI, VIRTIO_*).

Re: [PATCH 02/10] hw/i386: Select E1000E for q35

[Thomas Huth]

On 06/02/2023 15.08, Fabiano Rosas wrote:
> The e1000e network adapter is the default network card for the q35
> machine. Make sure that CONFIG is always selected for that machine.
> 
> 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..527b95df81 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -93,7 +93,6 @@ config Q35
>       bool
>       imply VTD
>       imply AMD_IOMMU
> -    imply E1000E_PCI_EXPRESS
>       imply VMPORT
>       imply VMMOUSE
>       select PC_PCI
> @@ -104,6 +103,7 @@ config Q35
>       select DIMM
>       select SMBIOS
>       select FW_CFG_DMA
> +    select E1000E_PCI_EXPRESS

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 03/10] hw/i386: Select VGA_PCI in Kconfig

[Thomas Huth]

On 06/02/2023 15.08, Fabiano Rosas wrote:
> Machines that have "std" as default VGA display need to always build
> vga-pci.c, otherwise we get a crash when CONFIG_PCI_DEVICES=n:
> 
> $ ./qemu-system-x86_64 -M q35 -vga std

I'd remove the "-vga std" in above example to show that it also crashed "by 
default".

> qemu-system-x86_64: unknown type 'VGA'
> 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 527b95df81..8e59cb6634 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -30,9 +30,9 @@ config PC
>       imply TEST_DEVICES
>       imply TPM_CRB
>       imply TPM_TIS_ISA
> -    imply VGA_PCI
>       imply VIRTIO_VGA
>       imply NVDIMM
> +    select VGA_PCI
>       select FDC_ISA
>       select I8259
>       select I8254

Maybe it would be better to add it to the i440fx and the q35 machine only, 
so that you could still compile the isapc machine without it?

  Thomas

Re: [PATCH 03/10] hw/i386: Select VGA_PCI in Kconfig

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 15.08, Fabiano Rosas wrote:
>> Machines that have "std" as default VGA display need to always build
>> vga-pci.c, otherwise we get a crash when CONFIG_PCI_DEVICES=n:
>> 
>> $ ./qemu-system-x86_64 -M q35 -vga std
>
> I'd remove the "-vga std" in above example to show that it also crashed "by 
> default".
>
>> qemu-system-x86_64: unknown type 'VGA'
>> 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 527b95df81..8e59cb6634 100644
>> --- a/hw/i386/Kconfig
>> +++ b/hw/i386/Kconfig
>> @@ -30,9 +30,9 @@ config PC
>>       imply TEST_DEVICES
>>       imply TPM_CRB
>>       imply TPM_TIS_ISA
>> -    imply VGA_PCI
>>       imply VIRTIO_VGA
>>       imply NVDIMM
>> +    select VGA_PCI
>>       select FDC_ISA
>>       select I8259
>>       select I8254
>
> Maybe it would be better to add it to the i440fx and the q35 machine only, 
> so that you could still compile the isapc machine without it?

isapc depends on i440fx, so it's will bring VGA_PCI into the build
anyway.

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

[Thomas Huth]

On 06/02/2023 15.08, Fabiano Rosas wrote:
> 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
> 
> 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 8e59cb6634..343783ea3e 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

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 06/02/2023 15.08, Fabiano Rosas wrote:
> 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
> 
> 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

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 06/10] hw/arm: Select VIRTIO_BLK for virt machine

[Thomas Huth]

On 06/02/2023 15.08, Fabiano Rosas wrote:
> The virt machine has IF_VIRTIO as block_default_type, which causes the
> generic code to try to create a virtio-blk-pci device pair at
> configure_blockdev()/qemu_create_cli_devices().
> 
> Select VIRTIO_BLK 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 -nodefaults -cdrom foo.qcow2
> qemu-system-aarch64: -cdrom foo.qcow2: 'virtio-blk' (alias
> 'virtio-blk-pci') is not a valid device model name
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   hw/arm/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 8dcc08b7ec..296d4f5176 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -33,6 +33,7 @@ config ARM_VIRT
>       select ACPI_HMAT
>       select VIRTIO_PCI
>       select VIRTIO_NET
> +    select VIRTIO_BLK

I'm a little bit torn here ... while it makes sense for me to add VIRTIO_NET 
so that you can run "qemu-system-aarch64 -M virt" without any additional 
arguments, this is now about fixing additional (convenience) CLI options 
that are optional ...

I assume we need those for some qtests? What about checking for virtio-blk 
in those tests instead?

  Thomas

Re: [PATCH 06/10] hw/arm: Select VIRTIO_BLK for virt machine

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 15.08, Fabiano Rosas wrote:
>> The virt machine has IF_VIRTIO as block_default_type, which causes the
>> generic code to try to create a virtio-blk-pci device pair at
>> configure_blockdev()/qemu_create_cli_devices().
>> 
>> Select VIRTIO_BLK 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 -nodefaults -cdrom foo.qcow2
>> qemu-system-aarch64: -cdrom foo.qcow2: 'virtio-blk' (alias
>> 'virtio-blk-pci') is not a valid device model name
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   hw/arm/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index 8dcc08b7ec..296d4f5176 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -33,6 +33,7 @@ config ARM_VIRT
>>       select ACPI_HMAT
>>       select VIRTIO_PCI
>>       select VIRTIO_NET
>> +    select VIRTIO_BLK
>
> I'm a little bit torn here ... while it makes sense for me to add VIRTIO_NET 
> so that you can run "qemu-system-aarch64 -M virt" without any additional 
> arguments, this is now about fixing additional (convenience) CLI options 
> that are optional ...
>
> I assume we need those for some qtests? What about checking for virtio-blk 
> in those tests instead?

Yes, from bios-tables-test.c.

I tried hard to avoid this kind of scenario, but ultimately it's the
-cdrom option that's broken, not the test.

That's just how the code was written. The -cdrom option can use
virtio-blk as a backend because the virt machine has IF_VIRTIO as
block_default_type. So virtio-blk is a dependency as long as -cdrom is
present in the build. Unless we change something else or guard against
it in the code somehow.