Re: [PATCH v2 2/9] hw/hppa/machine: Disable default devices with --nodefaults option

[Helge Deller]

On 1/9/24 17:01, Richard Henderson wrote:

On 1/9/24 22:16, Helge Deller wrote:

On 1/9/24 10:57, Richard Henderson wrote:

On 1/8/24 00:22, del...@kernel.org wrote:

From: Helge Deller <del...@gmx.de>

Add support for the qemu --nodefaults option, which will disable the
following default devices:
- lsi53c895a SCSI controller,
- artist graphics card,
- LASI 82596 NIC,
- tulip PCI NIC,
- second serial PCI card,
- USB OHCI controller.

Adding this option is very useful to allow manual testing and
debugging of the other possible devices on the command line.

Signed-off-by: Helge Deller <del...@gmx.de>
---
  hw/hppa/machine.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index b11907617e..8017002a2a 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -346,11 +346,14 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
      SysBusDevice *s;
      /* SCSI disk setup. */
-    dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-    lsi53c8xx_handle_legacy_cmdline(dev);
+    if (defaults_enabled()) {
+        dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
+        lsi53c8xx_handle_legacy_cmdline(dev);
+    }
      /* Graphics setup. */
-    if (machine->enable_graphics && vga_interface_type != VGA_NONE) {
+    if (defaults_enabled() && machine->enable_graphics &&
+        vga_interface_type != VGA_NONE) {
          vga_interface_created = true;
          dev = qdev_new("artist");
          s = SYS_BUS_DEVICE(dev);
@@ -360,7 +363,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
      }
      /* Network setup. */
-    if (enable_lasi_lan()) {
+    if (defaults_enabled() && enable_lasi_lan()) {
          lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA),
                          qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA));
      }
@@ -385,7 +388,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
      pci_set_word(&pci_dev->config[PCI_SUBSYSTEM_ID], 0x1227); /* Powerbar */
      /* create a second serial PCI card when running Astro */
-    if (!lasi_dev) {
+    if (defaults_enabled() && !lasi_dev) {
          pci_dev = pci_new(-1, "pci-serial-4x");
          qdev_prop_set_chr(DEVICE(pci_dev), "chardev1", serial_hd(1));
          qdev_prop_set_chr(DEVICE(pci_dev), "chardev2", serial_hd(2));
@@ -395,7 +398,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
      }
      /* create USB OHCI controller for USB keyboard & mouse on Astro machines 
*/
-    if (!lasi_dev && machine->enable_graphics) {
+    if (defaults_enabled() && !lasi_dev && machine->enable_graphics) {
          pci_create_simple(pci_bus, -1, "pci-ohci");
          usb_create_simple(usb_bus_find(-1), "usb-kbd");
          usb_create_simple(usb_bus_find(-1), "usb-mouse");

This almost doubles the uses of default_enabled in the entire tree.
I wonder if some of them are redundant or should be using a different
test.

Any proposal?
Maybe introduce a local variable hppa_bare_metal = !defaults_enabled();
and use that instead?

No, not like that.

Ok.

In casual review I am surprised that !defaults_enabled() does not
already imply !enable_graphics, unless the command-line goes on to
explicitly add a graphics device.
Am I missing something?

Will check that tommorow. If it does I'll remove that additional check.

But what other do you suggest in general how I should address your
concerns here?

Helge