Re: [PULL 2/6] hw/hppa: Wire up Diva GSP card

[Helge Deller <deller@kernel.org>]

* Peter Maydell <peter.maydell@linaro.org>:
> On Fri, 7 Feb 2025 at 21:08, <deller@kernel.org> wrote:
> >
> > From: Helge Deller <deller@gmx.de>
> >
> > Until now we used a standard serial-pci device to emulate a HP serial
> > console.  This worked nicely with 32-bit Linux and 32-bit HP-UX, but
> > 64-bit HP-UX crashes with it and expects either a Diva GSP card, or a real
> > 64-bit capable PCI graphic card (which we don't have yet).
> > In order to continue with 64-bit HP-UX, switch over to the recently
> > added Diva GSP card emulation.
> >
> > Signed-off-by: Helge Deller <deller@gmx.de>
> 
> Hi; I've been looking at memory leaks reported by the clang
> address-sanitizer, and it flags one up introduced by this change
> from last year:
> 
> 
> > @@ -383,26 +383,17 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
> 
> > +    /* BMC board: HP Diva GSP */
> > +    dev = qdev_new("diva-gsp");
> 
> Here we create a new diva-gsp device...
> 
> > +    if (!object_property_get_bool(OBJECT(dev), "disable", NULL)) {
> 
> ...but then after looking at the "disable" property on it we
> do nothing else with it, and instead...
> 
> > +        pci_dev = pci_new_multifunction(PCI_DEVFN(2, 0), "diva-gsp");
> 
> ...create a second instance of it via pci_new_multifunction().
> So now there are two copies of this device, one half-initialized
> and leaked, and the second one which we actually use.
 
Nice catch!

> What is the intention here? The value of the "disable" property
> on the device is presumably just going to be its default "false"
> value all the time, so can we drop that check entirely and
> create the device via pci_new_multifunction() unconditionally?

Yes.

Background: During development of the various machines it was
easier for me to just disable/enable the Diva card from the command
line via the "disable" property.
Commit 16786eb7bf86 ("hw/hppa: Add BMC on 64-bit machines only") now
simply disables diva on B160L, and for the C3700, although it never
had Diva boards, we will use Diva until we get SuperIO implemented.

The patch below should address this.
Maybe you can review?

Thanks!
Helge


From: Helge Deller <deller@gmx.de>
Subject: [PATCH] hw/hppa: Avoid leaking a diva-gsp device

Create a Diva-gsp unconditionally on all 64-bit PCI machines.
The A400 usually comes with a Diva card. The C3700 has a built-in
SUPERIO chip, which we haven't implemented yet, so running with an
emulated Diva is the best we can do for now.

Signed-off-by: Helge Deller <deller@gmx.de>
Noticed-by: Peter Maydell <peter.maydell@linaro.org>

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index f55e84529f..50ace81528 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -380,18 +380,15 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
 
     if (pci_bus && hppa_is_pa20(&cpu[0]->env)) {
         /* BMC board: HP Diva GSP PCI card */
-        dev = qdev_new("diva-gsp");
-        if (dev && !object_property_get_bool(OBJECT(dev), "disable", NULL)) {
-            pci_dev = pci_new_multifunction(PCI_DEVFN(2, 0), "diva-gsp");
-            if (!lasi_dev) {
-                /* bind default keyboard/serial to Diva card */
-                qdev_prop_set_chr(DEVICE(pci_dev), "chardev1", serial_hd(0));
-                qdev_prop_set_chr(DEVICE(pci_dev), "chardev2", serial_hd(1));
-                qdev_prop_set_chr(DEVICE(pci_dev), "chardev3", serial_hd(2));
-                qdev_prop_set_chr(DEVICE(pci_dev), "chardev4", serial_hd(3));
-            }
-            pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
+        pci_dev = pci_new_multifunction(PCI_DEVFN(2, 0), "diva-gsp");
+        if (!lasi_dev) {
+            /* bind default keyboard/serial to Diva card */
+            qdev_prop_set_chr(DEVICE(pci_dev), "chardev1", serial_hd(0));
+            qdev_prop_set_chr(DEVICE(pci_dev), "chardev2", serial_hd(1));
+            qdev_prop_set_chr(DEVICE(pci_dev), "chardev3", serial_hd(2));
+            qdev_prop_set_chr(DEVICE(pci_dev), "chardev4", serial_hd(3));
         }
+        pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
     }
 
     /* create USB OHCI controller for USB keyboard & mouse on Astro machines */