[Qemu-devel] unique (or otherwise) RAM block names

[Qemu-devel] unique (or otherwise) RAM block names

[Peter Maydell]

I was going through trying to fix up various devices that currently
fail to register RAM blocks for migration, or register the RAM
block globally rather than locally, and I ran into something
unexpected.

We name RAM blocks in qemu_ram_set_idstr() like this:
  * if the user passed a device pointer, then call qdev_get_dev_path(),
    and if that returns non-NULL, use "path/name"
  * otherwise, use "name"

Unfortunately, it turns out that there's no guarantee that
qdev_get_dev_path() will return anything useful. If the device
isn't on a bus, or the bus's class doesn't implement the get_dev_path
method, then it'll return NULL.

In particular, this means that if you create what you expect to
be a local-to-this-device RAM memory region in a SysBusDevice,
then (because SysBus doesn't implement get_dev_path), there is
no per-device qualification added to the region name, and so the
code silently creates a globally-namespaced RAM region.
Trying to create multiple instances of the device therefore fails.

How can we make this work (preferably without breaking migration
compat for existing devices) ?

(Sysbus isn't the only bus that doesn't implement get_dev_path,
it's just the first one I found.)

thanks
-- PMM

Re: [Qemu-devel] unique (or otherwise) RAM block names

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> I was going through trying to fix up various devices that currently
> fail to register RAM blocks for migration, or register the RAM
> block globally rather than locally, and I ran into something
> unexpected.
> 
> We name RAM blocks in qemu_ram_set_idstr() like this:
>   * if the user passed a device pointer, then call qdev_get_dev_path(),
>     and if that returns non-NULL, use "path/name"
>   * otherwise, use "name"
> 
> Unfortunately, it turns out that there's no guarantee that
> qdev_get_dev_path() will return anything useful. If the device
> isn't on a bus, or the bus's class doesn't implement the get_dev_path
> method, then it'll return NULL.
> 
> In particular, this means that if you create what you expect to
> be a local-to-this-device RAM memory region in a SysBusDevice,
> then (because SysBus doesn't implement get_dev_path), there is
> no per-device qualification added to the region name, and so the
> code silently creates a globally-namespaced RAM region.
> Trying to create multiple instances of the device therefore fails.

For device we also have a nmumerical 'instance_id' parameter that
some odd setups use.

> How can we make this work (preferably without breaking migration
> compat for existing devices) ?
> 
> (Sysbus isn't the only bus that doesn't implement get_dev_path,
> it's just the first one I found.)

My preference would be that for new machine types we should
define get_dev_path on the busses that are missing it.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] unique (or otherwise) RAM block names

[Peter Maydell]

On 1 June 2018 at 18:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> In particular, this means that if you create what you expect to
> be a local-to-this-device RAM memory region in a SysBusDevice,
> then (because SysBus doesn't implement get_dev_path), there is
> no per-device qualification added to the region name, and so the
> code silently creates a globally-namespaced RAM region.
> Trying to create multiple instances of the device therefore fails.

I did an audit of everywhere that creates a RAM region with an
owner (either by calling vmstate_register_ram() with a non-NULL
owner, or by calling memory_region_init_{ram,rom,rom_device} with
a non-NULL owner:

hw/display/g364fb.c:    vmstate_register_ram(&s->mem_vram, dev);
  TYPE_G364 (sysbus)
hw/display/vga.c:    vmstate_register_ram(&s->vram, global_vmstate ?
NULL : DEVICE(obj));
  NULL dev, or passed by caller of vga_common_init()
     TYPE_ISA_CIRRUS_VGA (ISA) (but OK as it passes global_vmstate = true)
     TYPE_PCI_CIRRUS_VGA (PCI)
     TYPE_PCI_QXL (PCI)
     TYPE_PCI_VGA (PCI)
     TYPE_VIRTIO_VGA (PCI)
     TYPE_VMWARE_SVGA (PCI)
hw/mem/pc-dimm.c:    vmstate_register_ram(vmstate_mr, dev);
  TYPE_PC_DIMM (?)
hw/misc/ivshmem.c:    vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
  TYPE_IVSHMEM_COMMON (PCI)

hw/display/qxl.c:    memory_region_init_ram(&qxl->rom_bar,
OBJECT(qxl), "qxl.vrom",
hw/display/qxl.c:    memory_region_init_ram(&qxl->vram_bar,
OBJECT(qxl), "qxl.vram",
hw/display/qxl.c:    memory_region_init_ram(&qxl->vga.vram,
OBJECT(dev), "qxl.vgavram",
  TYPE_PCI_QXL (PCI)
hw/pci/pci.c:    memory_region_init_rom(&pdev->rom, OBJECT(pdev),
name, size, &error_fatal);
  -- PCIDevice
hw/arm/aspeed_soc.c:    memory_region_init_ram(&s->sram, OBJECT(dev),
"aspeed.sram",
  TYPE_ASPEED_SOC (sysbus)
hw/arm/integratorcp.c:    memory_region_init_ram(&s->flash, OBJECT(d),
"integrator.flash", 0x100000,
  TYPE_INTEGRATOR_CM (sysbus)
hw/display/bochs-display.c:    memory_region_init_ram(&s->vram, obj,
"bochs-display-vram", s->vgamem,
  TYPE_BOCHS_DISPLAY (PCI)
hw/display/sm501.c:    memory_region_init_ram(&s->local_mem_region,
OBJECT(dev), "sm501.local",
  TYPE_SYSBUS_SM501 (sysbus)
  TYPE_PCI_SM501 (PCI)
hw/tpm/tpm_crb.c:    memory_region_init_ram(&s->cmdmem, OBJECT(s),
  TYPE_TPM_CRB (TYPE_DEVICE)
hw/xen/xen_pt_load_rom.c:    memory_region_init_ram(&dev->rom, owner,
name, st.st_size, &error_abort);
  PCI device
hw/arm/aspeed.c:        memory_region_init_rom(boot_rom, OBJECT(bmc),
"aspeed.boot_rom",
   -- this is bogus because BMC isn't really an object; it happens
that its first member is an AspeedSoCState; TYPE_ASPEED_SOC is a
TYPE_DEVICE

memory_region_init_rom_device()
hw/block/pflash_cfi01.c:    memory_region_init_rom_device(
 TYPE_CFI_PFLASH01 (sysbus)
hw/block/pflash_cfi02.c:
memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
 TYPE_CFI_PFLASH02 (sysbus)

PCI implements get_dev_path. The other uses of RAM regions with
owners but unexpectedly non-local names thus are:
  TYPE_G364 (sysbus)
  TYPE_PC_DIMM (?)
  TYPE_ASPEED_SOC (sysbus)
  TYPE_INTEGRATOR_CM (sysbus)
  TYPE_SYSBUS_SM501 (sysbus)
  TYPE_TPM_CRB (TYPE_DEVICE)
  TYPE_CFI_PFLASH01 (sysbus)
  TYPE_CFI_PFLASH02 (sysbus)

thanks
-- PMM

Re: [Qemu-devel] unique (or otherwise) RAM block names

[Laszlo Ersek]

On 06/01/18 19:34, Peter Maydell wrote:
> I was going through trying to fix up various devices that currently
> fail to register RAM blocks for migration, or register the RAM
> block globally rather than locally, and I ran into something
> unexpected.
> 
> We name RAM blocks in qemu_ram_set_idstr() like this:
>   * if the user passed a device pointer, then call qdev_get_dev_path(),
>     and if that returns non-NULL, use "path/name"
>   * otherwise, use "name"
> 
> Unfortunately, it turns out that there's no guarantee that
> qdev_get_dev_path() will return anything useful. If the device
> isn't on a bus, or the bus's class doesn't implement the get_dev_path
> method, then it'll return NULL.
> 
> In particular, this means that if you create what you expect to
> be a local-to-this-device RAM memory region in a SysBusDevice,
> then (because SysBus doesn't implement get_dev_path), there is
> no per-device qualification added to the region name, and so the
> code silently creates a globally-namespaced RAM region.
> Trying to create multiple instances of the device therefore fails.
> 
> How can we make this work (preferably without breaking migration
> compat for existing devices) ?
> 
> (Sysbus isn't the only bus that doesn't implement get_dev_path,
> it's just the first one I found.)

Related: commit f58b39d2d5b6 ("virtio-mmio: format transport base
address in BusClass.get_dev_path", 2016-07-14).

Thanks
Laszlo