Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Cédric Le Goater]

On 3/10/22 19:49, Thomas Huth wrote:
> On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
> > Hi,
> >
> > This version implements Cedric's review suggestions from v4. No
> > drastic design changes were made.
> >
> > Changes from v4:
> > - patches 1,3,5: unchanged
> > - patch 2:
> >    * renamed function to pnv_phb4_xscom_realize()
> >    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
> > - patch 4:
> >    * changed pnv_phb4_get_stack signature to use chip and phb
> >    * added a new helper called pnv_pec_stk_default_phb_realize() to
> > realize the default phb when running with defaults
> > - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
> >
> > Daniel Henrique Barboza (5):
> >    ppc/pnv: set phb4 properties in stk_realize()
> >    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
> >    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
> >    ppc/pnv: Introduce user creatable pnv-phb4 devices
> >    ppc/pnv: turn pnv_phb4_update_regions() into static
>
> It's now possible to crash QEMU with the pnv-phb4 device:
>
> $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
> Unexpected error in object_property_try_add() at 
> ../../devel/qemu/qom/object.c:1229:
> qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 
> 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
> Aborted (core dumped)
>
> Any ideas how to fix this?

This was introduced by :

  commit 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM hierarchy")

It could be fixed with :

@@ -1598,15 +1598,15 @@ static void pnv_phb4_realize(DeviceState
             error_propagate(errp, local_err);
             return;
         }
-    }
 
-    /* Reparent the PHB to the chip to build the device tree */
-    pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
+        /* Reparent the PHB to the chip to build the device tree */
+        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
 
-    s = qdev_get_parent_bus(DEVICE(chip));
-    if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
-        error_propagate(errp, local_err);
-        return;
+        s = qdev_get_parent_bus(DEVICE(chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     /* Set the "big_phb" flag */


but I am not sure we want to keep user-created PHB* devices.

Thanks,

C.