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

[Helge Deller <deller@gmx.de>]

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, 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.

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