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

[Thomas Huth <thuth@redhat.com>]

On 11/01/2024 23.28, Helge Deller wrote:
> 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?

IIRC enable_graphics is not influenced by --nodefaults, but it should be 
possible to simply check vga_interface_type only - that should get set to 
VGA_NONE when the user started QEMU with --nodefaults.

For networking, other boards normally check nd_table[0]. And for serial, you 
can check whether serial_hd(0) returns a non-NULL value.

For checking whether you have to create SCSI devices by default, you can 
check drive_get_max_bus(IF_SCSI), I think.

  HTH,
   Thomas