Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 28/09/2015 10:11, Markus Armbruster wrote:
>> 
>> For most of the devices my patch marks, we have a pretty good idea on
>> what's wrong with them.  spapr-rng is among the exceptions.  You believe
>> it's actually "the macio object".  Which one?  "macio" is abstract...
>> 
>> You report introspecting "spapr-rng" crashes "while trying to go through
>> the macio object".  I wonder how omitting introspection of macio objects
>> (that's what marking them does to this test) could affect the object
>> we're going through when we crash.
>> 
>>> > Or maybe we could get this also fixed? The problem could be the
>>> > memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>>> > macio_instance_init() ... is this ok here? Or does this rather have to
>>> > go to the realize() function instead?
>> Hmm, does creating and destroying a macio object leave the memory region
>> behind?
>> 
>> Paolo, is calling memory_region_init() in an instance_init() method
>> okay?
>
> Yes, but it has to have a non-NULL owner.
>
> The reason why this particular call has a NULL owner is that the
> (non-qdevified) DBDMA_init object inside it is also passing a NULL
> owner.  DBDMA_init object is also doing a few more non-idempotent things
> such as a malloc, a vmstate_register and a qemu_register_reset.
>
> The full solution would be to qdev-ify DBDMA.  A simpler but also valid
> solution would be to move DBDMA_init to macio_common_realize, in
> addition to setting the owner of s->bar.

All right, we're now sure macio is the broken part that needs to be
marked in this series.

I searched for memory_region_init() calls passing a null owner:

* exec.c memory_map_init()
  hw/i386/pc_piix.c pc_init1()
  hw/i386/pc_q35.c pc_q35_init()
  hw/mips/mips_jazz.c mips_jazz_init()
  hw/mips/mips_r4k.c mips_r4k_init()
  hw/ppc/ppc405_boards.c ref405ep_init()

  Not device model code, doesn't affect my series.

* hw/core/platform-bus.c platform_bus_realize()
  hw/display/vga-pci.c pci_std_vga_realize()

  Realize method, doesn't affect my series.

* hw/misc/omap_gpmc.c omap_gpmc_cs_map()

  Is this sane?  I guess the object_unparent() in omap_gpmc_cs_unmap()
  could make it sane.

* hw/ppc/ppc4xx_devs.c sdram_set_bcr()

  Is this sane?  I guess the object_unparent() there could make it sane.

* hw/misc/macio/macio.c macio_instance_init()

  I understand this isn't currently sane, and I'll mark type "macio"
  (and thus its sub-types) in my v4.

* hw/misc/macio/macio.c macio_escc_legacy_setup() 

  I don't care due to the previous item.