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

[Richard Henderson]

On 1/9/24 22:16, Helge Deller wrote:
> On 1/9/24 10:57, Richard Henderson wrote:
> > On 1/8/24 00:22, deller@kernel.org wrote:
> > > From: Helge Deller <deller@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 <deller@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.

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?


r~