Patchwork [RFC,3/6] RAMBlock: Add a name field

login
register
mail settings
Submitter Alex Williamson
Date June 8, 2010, 7:15 p.m.
Message ID <20100608191557.4451.30384.stgit@localhost.localdomain>
Download mbox | patch
Permalink /patch/55020/
State New
Headers show

Comments

Alex Williamson - June 8, 2010, 7:15 p.m.
The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
let the caller specify a name so we can make a positive match.

Note, this only addresses the qemu-kvm callers so far.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 cpu-all.h              |    1 +
 cpu-common.h           |    4 ++--
 exec.c                 |    8 +++++---
 hw/device-assignment.c |    8 ++++++--
 hw/pc.c                |    8 ++++----
 hw/pci.c               |    5 ++++-
 hw/vga.c               |    2 +-
 hw/vmware_vga.c        |    2 +-
 8 files changed, 24 insertions(+), 14 deletions(-)
Anthony Liguori - June 8, 2010, 8:07 p.m.
On 06/08/2010 02:15 PM, Alex Williamson wrote:
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.
>
> Note, this only addresses the qemu-kvm callers so far.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> ---
>
>   cpu-all.h              |    1 +
>   cpu-common.h           |    4 ++--
>   exec.c                 |    8 +++++---
>   hw/device-assignment.c |    8 ++++++--
>   hw/pc.c                |    8 ++++----
>   hw/pci.c               |    5 ++++-
>   hw/vga.c               |    2 +-
>   hw/vmware_vga.c        |    2 +-
>   8 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index 458cb4b..a5b886a 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -865,6 +865,7 @@ typedef struct RAMBlock {
>       uint8_t *host;
>       ram_addr_t offset;
>       ram_addr_t length;
> +    char name[64];
>       QLIST_ENTRY(RAMBlock) next;
>   } RAMBlock;
>
> diff --git a/cpu-common.h b/cpu-common.h
> index 4b0ba60..5b00544 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
>   }
>
>   ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
> -ram_addr_t qemu_ram_alloc(ram_addr_t);
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
>   void qemu_ram_free(ram_addr_t addr);
>   /* This should only be used for ram local to a device.  */
>   void *qemu_get_ram_ptr(ram_addr_t addr);
> diff --git a/exec.c b/exec.c
> index d785de3..dd57515 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
>   }
>   #endif
>
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
>   {
>       RAMBlock *new_block;
>
>       size = TARGET_PAGE_ALIGN(size);
>       new_block = qemu_malloc(sizeof(*new_block));
>
> +    // XXX check duplicates
> +    snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
>    

That strdup() is probably unintentional.

>       new_block->host = host;
>
>       new_block->offset = ram.last_offset;
> @@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
>       return new_block->offset;
>   }
>
> -ram_addr_t qemu_ram_alloc(ram_addr_t size)
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
>   {
>       void *host;
>
> @@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>   #endif
>       }
>
> -    return qemu_ram_map(size, host);
> +    return qemu_ram_map(name, size, host);
>   }
>
>   void qemu_ram_free(ram_addr_t addr)
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 2b963b5..1d631f6 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>
>               if (!slow_map) {
>                   void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
> +                char name[14];
>
> -                pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
> -                                                                virtbase);
> +                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> +                         PCI_SLOT(pci_dev->dev.devfn),
> +                         PCI_FUNC(pci_dev->dev.devfn), i);
> +                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> +                                                  cur_region->size, virtbase);
>               } else
>                   pci_dev->v_addrs[i].memory_index = 0;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index eae0db4..76be151 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
>       linux_boot = (kernel_filename != NULL);
>
>       /* allocate RAM */
> -    ram_addr = qemu_ram_alloc(below_4g_mem_size);
> +    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
>       cpu_register_physical_memory(0, 0xa0000, ram_addr);
>       cpu_register_physical_memory(0x100000,
>                    below_4g_mem_size - 0x100000,
> @@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
>   #if TARGET_PHYS_ADDR_BITS == 32
>           hw_error("To much RAM for 32-bit physical address");
>   #else
> -        ram_addr = qemu_ram_alloc(above_4g_mem_size);
> +        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
>           cpu_register_physical_memory(0x100000000ULL,
>                                        above_4g_mem_size,
>                                        ram_addr);
> @@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
>           (bios_size % 65536) != 0) {
>           goto bios_error;
>       }
> -    bios_offset = qemu_ram_alloc(bios_size);
> +    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
>       ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
>       if (ret != 0) {
>       bios_error:
> @@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
>       }
>       option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
>
> -    option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
> +    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
>       cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
>
>       /* map all the bios at the top of memory */
> diff --git a/hw/pci.c b/hw/pci.c
> index f0b6790..b2632a8 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
>       int size;
>       char *path;
>       void *ptr;
> +    char name[13];
>
>       if (!pdev->romfile)
>           return 0;
> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>           size = 1<<  qemu_fls(size);
>       }
>
> -    pdev->rom_offset = qemu_ram_alloc(size);
> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +    pdev->rom_offset = qemu_ram_alloc(name, size);
>
>       ptr = qemu_get_ram_ptr(pdev->rom_offset);
>       load_image(path, ptr);
> diff --git a/hw/vga.c b/hw/vga.c
> index 0a535ae..47b800f 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
>   #else
>       s->is_vbe_vmstate = 0;
>   #endif
> -    s->vram_offset = qemu_ram_alloc(vga_ram_size);
> +    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
>       s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
>       s->vram_size = vga_ram_size;
>       s->get_bpp = vga_get_bpp;
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index bf2a699..9df8c08 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1164,7 +1164,7 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
>
>
>       s->fifo_size = SVGA_FIFO_SIZE;
> -    s->fifo_offset = qemu_ram_alloc(s->fifo_size);
> +    s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
>       s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
>
>       vga_common_init(&s->vga, vga_ram_size);
>    

Regards,

Anthony Liguori
Alex Williamson - June 8, 2010, 8:09 p.m.
On Tue, 2010-06-08 at 15:07 -0500, Anthony Liguori wrote:
> On 06/08/2010 02:15 PM, Alex Williamson wrote:
> > The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> > let the caller specify a name so we can make a positive match.
> >
> > Note, this only addresses the qemu-kvm callers so far.
> >
> > -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> > +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
> >   {
> >       RAMBlock *new_block;
> >
> >       size = TARGET_PAGE_ALIGN(size);
> >       new_block = qemu_malloc(sizeof(*new_block));
> >
> > +    // XXX check duplicates
> > +    snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
> >    
> 
> That strdup() is probably unintentional.

Yep, thanks.

Alex
Chris Wright - June 8, 2010, 9:41 p.m.
* Alex Williamson (alex.williamson@redhat.com) wrote:
> +    // XXX check duplicates

Yes, definitely.  You created a notion of a hierarchical namespace,
can this be formalized any more?  Currently scattered...

> +                char name[14];
> +                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> +                         PCI_SLOT(pci_dev->dev.devfn),
> +                         PCI_FUNC(pci_dev->dev.devfn), i);
> +                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> +                                                  cur_region->size, virtbase);
> +    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> +        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> +    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> +    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> +    char name[13];
(13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})
> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +    pdev->rom_offset = qemu_ram_alloc(name, size);
> +    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> +    s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);

So far we have:
 ram.
     pc.
        lowmem
        highmem
        bios
        rom
     vga.
         vram
     vmsvga.
            fifo
  pci.
      D.F. (B:D.F?)
          bar
          rom
Paul Brook - June 9, 2010, 2:30 a.m.
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.

> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +    pdev->rom_offset = qemu_ram_alloc(name, size);

This looks pretty bogus.  It should be associated with the device, rather than 
incorrectly trying to generate a globally unique name.

Paul
Anthony Liguori - June 9, 2010, 2:40 a.m.
On 06/08/2010 09:30 PM, Paul Brook wrote:
>> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
>> let the caller specify a name so we can make a positive match.
>>      
>    
>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +    pdev->rom_offset = qemu_ram_alloc(name, size);
>>      
> This looks pretty bogus.  It should be associated with the device, rather than
> incorrectly trying to generate a globally unique name.
>    

Not all ram is associated with a device.

For instance, the base ram for a guest.

Regards,

Anthony Liguori

> Paul
>
>
Paul Brook - June 9, 2010, 2:54 a.m.
> On 06/08/2010 09:30 PM, Paul Brook wrote:
> >> The offset given to a block created via qemu_ram_alloc/map() is
> >> arbitrary, let the caller specify a name so we can make a positive
> >> match.
> >> 
> >> 
> >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> >> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> >> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >> +    pdev->rom_offset = qemu_ram_alloc(name, size);
> > 
> > This looks pretty bogus.  It should be associated with the device, rather
> > than incorrectly trying to generate a globally unique name.
> 
> Not all ram is associated with a device.

Maybe not, but where it is we should be using that information.
Absolute minimum we should be using the existing qdev address rather than 
inventing a new one.  Duplicating this logic inside every device seems like a 
bad idea so I suggest identifying ram blocks by a (name, device) pair. For now 
we can allow a NULL device for system memory.

Paul
Alex Williamson - June 9, 2010, 3:13 a.m.
On Tue, 2010-06-08 at 14:41 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > +    // XXX check duplicates
> 
> Yes, definitely. 

Yep, I was just thinking that without freeing, the uniqueness really
falls apart.  If we hotplug a nic multiple times on the source, we're
going to have multiple pci:d.f for the slot.  If we're lucky, they're
all for the same type of device and therefore the same ROM size and the
ordering will cause them to be merged into one on the target.  That
requires a lot of luck though.  Maybe once we have freeing we just blow
up and call it a device bug if it didn't free and tries to add more ram
with the same name.

> You created a notion of a hierarchical namespace,
> can this be formalized any more?  Currently scattered...

Yeah, it's pretty haphazard, I probably haven't spent enough time
working out what makes sense across all devices.  I started with
"ram.<device/driver>.<context>", where context was whatever I could
guess from the surrounding code.  Then I jumped over to "pci:d.f" (which
should at least be "pci.s:b:d.f") just because I thought the PCI address
space already provided a nicely unique layout.

> > +                char name[14];
> > +                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> > +                         PCI_SLOT(pci_dev->dev.devfn),
> > +                         PCI_FUNC(pci_dev->dev.devfn), i);
> > +                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> > +                                                  cur_region->size, virtbase);
> > +    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> > +        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> > +    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> > +    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> > +    char name[13];
> (13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})

That probably makes sense.

> > +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > +    pdev->rom_offset = qemu_ram_alloc(name, size);
> > +    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> > +    s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
> 
> So far we have:
>  ram.
>      pc.
>         lowmem
>         highmem
>         bios
>         rom
>      vga.
>          vram
>      vmsvga.
>             fifo

So maybe we say "ram." = "platform devices", things that have single
instances or are easy to split up into fixed, unique names.

>   pci.
>       D.F. (B:D.F?)
>           bar
>           rom

This one is pretty obvious.  If this is a reasonable starting point, I
can start hashing through the other archs and take a guess at what makes
sense.  Thanks,

Alex
Alex Williamson - June 9, 2010, 4:19 a.m.
On Wed, 2010-06-09 at 03:54 +0100, Paul Brook wrote:
> > On 06/08/2010 09:30 PM, Paul Brook wrote:
> > >> The offset given to a block created via qemu_ram_alloc/map() is
> > >> arbitrary, let the caller specify a name so we can make a positive
> > >> match.
> > >> 
> > >> 
> > >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> > >> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > >> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > >> +    pdev->rom_offset = qemu_ram_alloc(name, size);
> > > 
> > > This looks pretty bogus.  It should be associated with the device, rather
> > > than incorrectly trying to generate a globally unique name.
> > 
> > Not all ram is associated with a device.
> 
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than 
> inventing a new one.  Duplicating this logic inside every device seems like a 
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now 
> we can allow a NULL device for system memory.

I'm not sure if this is what you're after, but what if we change it to
something like:

+    snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
+             pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
+             PCI_FUNC(pdev->devfn), pdev->qdev.info->name);

This gives us things like "pci.0.03.0.rom-rtl8139".  For pci-assign, we
can expand on the host details, such as:

+                snprintf(name, sizeof(name),
+                         "%s.%02x.%x.bar%d-%s(%04x:%02x:%02x.%d)",
+                         pci_dev->dev.bus->qbus.name,
+                         PCI_SLOT(pci_dev->dev.devfn),
+                         PCI_FUNC(pci_dev->dev.devfn), i,
+                         pci_dev->dev.qdev.info->name,
+                         pci_dev->host.seg, pci_dev->host.bus,
+                         pci_dev->host.dev, pci_dev->host.func);

Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)".  Anywhere closer?
Thanks,

Alex
Gerd Hoffmann - June 9, 2010, 7:28 a.m.
On 06/09/10 04:40, Anthony Liguori wrote:
> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>> The offset given to a block created via qemu_ram_alloc/map() is
>>> arbitrary,
>>> let the caller specify a name so we can make a positive match.
>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>> This looks pretty bogus. It should be associated with the device,
>> rather than
>> incorrectly trying to generate a globally unique name.
>
> Not all ram is associated with a device.

Nevertheless qdev could carry a list of any device specific ram, so 
qdev_free() could automagically cleanup for you on unplug.  You could 
also reuse DeviceState->info->vmsd->name for the ram section naming.

cheers,
   Gerd
Avi Kivity - June 9, 2010, 11:55 a.m.
On 06/09/2010 12:41 AM, Chris Wright wrote:
> pci.
>        D.F. (B:D.F?)
>    

D:B:D.F

>            bar
>            rom
>    

bar.n
Avi Kivity - June 9, 2010, 11:58 a.m.
On 06/09/2010 05:54 AM, Paul Brook wrote:
>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>      
>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>> arbitrary, let the caller specify a name so we can make a positive
>>>> match.
>>>>
>>>>
>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>> +    pdev->rom_offset = qemu_ram_alloc(name, size);
>>>>          
>>> This looks pretty bogus.  It should be associated with the device, rather
>>> than incorrectly trying to generate a globally unique name.
>>>        
>> Not all ram is associated with a device.
>>      
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than
> inventing a new one.  Duplicating this logic inside every device seems like a
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
> we can allow a NULL device for system memory.
>
>    

I agree, this is duplicating the qdev tree in a string.  Devices/BARs 
should have ram qdev fields and so ram can be enumerated completely via 
qdev.

System memory can be part of a system device.
Paul Brook - June 9, 2010, 12:18 p.m.
> > > Not all ram is associated with a device.
> > 
> > Maybe not, but where it is we should be using that information.
> > Absolute minimum we should be using the existing qdev address rather than
> > inventing a new one.  Duplicating this logic inside every device seems
> > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > pair. For now we can allow a NULL device for system memory.
> 
> I'm not sure if this is what you're after, but what if we change it to
> something like:
> 
> +    snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> +             pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> +             PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
> 
> This gives us things like "pci.0.03.0.rom-rtl8139".  For pci-assign, we
> can expand on the host details, such as:
> ..
> Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)".  Anywhere closer?

Not really.  This identifier is device and bus independent, which is why I 
suggested passing the device to qemu_ram_alloc.  This can then figure out how 
to the identify the device. It should probably do this the same way that we 
identify the saved state for the device.  Currently I think this is an 
arbitrary vmstate name/id, but I expect this to change to a qdev address
(e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Paul
Paul Brook - June 9, 2010, 12:56 p.m.
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > +    // XXX check duplicates
> 
> Yes, definitely.  You created a notion of a hierarchical namespace,
> can this be formalized any more?

We already have one: The qdev tree.

Paul
Anthony Liguori - June 9, 2010, 1:57 p.m.
On 06/09/2010 06:58 AM, Avi Kivity wrote:
> On 06/09/2010 05:54 AM, Paul Brook wrote:
>>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>>> arbitrary, let the caller specify a name so we can make a positive
>>>>> match.
>>>>>
>>>>>
>>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>>> +    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>>> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>>> +    pdev->rom_offset = qemu_ram_alloc(name, size);
>>>> This looks pretty bogus.  It should be associated with the device, 
>>>> rather
>>>> than incorrectly trying to generate a globally unique name.
>>> Not all ram is associated with a device.
>> Maybe not, but where it is we should be using that information.
>> Absolute minimum we should be using the existing qdev address rather 
>> than
>> inventing a new one.  Duplicating this logic inside every device 
>> seems like a
>> bad idea so I suggest identifying ram blocks by a (name, device) 
>> pair. For now
>> we can allow a NULL device for system memory.
>>
>
> I agree, this is duplicating the qdev tree in a string.  Devices/BARs 
> should have ram qdev fields and so ram can be enumerated completely 
> via qdev.

Keep in mind, this has to be a stable string across versions of qemu 
since this is savevm/migration.  Are we absolutely confident that the 
full qdev path isn't going to change?  I'm more confident that a unique 
device name is going to be static across qemu versions.

Regards,

Anthony Liguori

> System memory can be part of a system device.
>
Paul Brook - June 9, 2010, 2:09 p.m.
> Keep in mind, this has to be a stable string across versions of qemu
> since this is savevm/migration.  Are we absolutely confident that the
> full qdev path isn't going to change?  I'm more confident that a unique
> device name is going to be static across qemu versions.

The actual representation of the device address is a secondary issue. The 
important point is that ram blocks should be associated with devices[*], and 
matched in exactly the same way. Devices should not be duplicating this on an 
ad-hoc basis.

Paul

[*] Ignore that we don't currently have a root system device node. A null 
device will suffice for now.
Alex Williamson - June 9, 2010, 4:37 p.m.
On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > > > Not all ram is associated with a device.
> > > 
> > > Maybe not, but where it is we should be using that information.
> > > Absolute minimum we should be using the existing qdev address rather than
> > > inventing a new one.  Duplicating this logic inside every device seems
> > > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > > pair. For now we can allow a NULL device for system memory.
> > 
> > I'm not sure if this is what you're after, but what if we change it to
> > something like:
> > 
> > +    snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> > +             pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> > +             PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
> > 
> > This gives us things like "pci.0.03.0.rom-rtl8139".  For pci-assign, we
> > can expand on the host details, such as:
> > ..
> > Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)".  Anywhere closer?
> 
> Not really.  This identifier is device and bus independent, which is why I 
> suggested passing the device to qemu_ram_alloc.  This can then figure out how 
> to the identify the device. It should probably do this the same way that we 
> identify the saved state for the device.  Currently I think this is an 
> arbitrary vmstate name/id, but I expect this to change to a qdev address
> (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Ok, that seems fairly reasonable, so from a device pointer we can get
something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
something like ":rom" or ":bar.0" to it via an extra string.

qemu_ram_alloc(DeviceState *dev, const char *info, size)

Does a function already exist to print out a qdev address path?  I don't
want to guess at something based on only this example.  Is there a
reserved/unused character we can use to separate the qdev device string
from the supplied name?  Thanks,

Alex
Paul Brook - June 9, 2010, 8:36 p.m.
> > Not really.  This identifier is device and bus independent, which is why
> > I suggested passing the device to qemu_ram_alloc.  This can then figure
> > out how to the identify the device. It should probably do this the same
> > way that we identify the saved state for the device.  Currently I think
> > this is an arbitrary vmstate name/id, but I expect this to change to a
> > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> 
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.
> 
> qemu_ram_alloc(DeviceState *dev, const char *info, size)

Exactly - though personally I wouldn't call the second argument "info".
 
> Does a function already exist to print out a qdev address path?

No.

I may have been a bit misleading here. What we really want to do is use the 
same matching algorithm as is used by the rest of the device state. Currently 
this is a vmstate name and [arbitrary] numeric id. I don't remember whether 
there's a convenient link from a device to its associated vmstate - if there 
isn't there probably should be.

Paul
Gerd Hoffmann - June 10, 2010, 8:23 a.m.
> I may have been a bit misleading here. What we really want to do is use the
> same matching algorithm as is used by the rest of the device state. Currently
> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> there's a convenient link from a device to its associated vmstate - if there
> isn't there probably should be.

DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.

cheers,
   Gerd
Alex Williamson - June 10, 2010, 2:33 p.m.
On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > I may have been a bit misleading here. What we really want to do is use the
> > same matching algorithm as is used by the rest of the device state. Currently
> > this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> > there's a convenient link from a device to its associated vmstate - if there
> > isn't there probably should be.
> 
> DeviceState->info->vmsd->name for the name.
> Dunno about the numeric id, I think savevm.c doesn't export it.

Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
It seems like this highlights a deficiency in the vmstate matching
though.  If on the source we do:

> pci_add addr=4 nic model=e1000
> pci_add addr=3 nic model=e1000

Then we start the target, ordering the nics sequentially, are we going
to store the vmstate into the opposite nics?  AIUI, libvirt does this
correctly today, but I don't like the idea of being required to remember
the history of a vm to migrate it.

Alex
Paul Brook - June 10, 2010, 2:49 p.m.
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > I may have been a bit misleading here. What we really want to do is use
> > > the same matching algorithm as is used by the rest of the device
> > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > don't remember whether there's a convenient link from a device to its
> > > associated vmstate - if there isn't there probably should be.
> > 
> > DeviceState->info->vmsd->name for the name.
> > Dunno about the numeric id, I think savevm.c doesn't export it.
> 
> Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching

Why are you forcing this to be a string?
 
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?

That's a separate problem. As long as you use the same matching as for the 
rest of the device state then it should just work. If it doesn't work then 
migration is already broken so it doen't matter.

Paul
Alex Williamson - June 10, 2010, 3:05 p.m.
On Wed, 2010-06-09 at 21:36 +0100, Paul Brook wrote:
> > > Not really.  This identifier is device and bus independent, which is why
> > > I suggested passing the device to qemu_ram_alloc.  This can then figure
> > > out how to the identify the device. It should probably do this the same
> > > way that we identify the saved state for the device.  Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> > 
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
> > 
> > qemu_ram_alloc(DeviceState *dev, const char *info, size)
> 
> Exactly - though personally I wouldn't call the second argument "info".

Hmm, this gets a little hairy for patch 5/6 where we try to create a
block on the fly to match the migration source.  For now, this is mainly
to catch things like devices that are hot plugged then removed before
migration, but don't currently have a functional qemu_ram_free() to
clean up.  However, if we could get past that and clean up drivers, it
might be nice for the string to provide enough information to
instantiate the missing device on the target.  I suddenly see that
char[64] name becoming insufficient.  Maybe we should follow the vmstate
example and use a variable length string preceded by a length byte (or
two).

Alex
Alex Williamson - June 10, 2010, 3:21 p.m.
On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> > On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > > I may have been a bit misleading here. What we really want to do is use
> > > > the same matching algorithm as is used by the rest of the device
> > > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > > don't remember whether there's a convenient link from a device to its
> > > > associated vmstate - if there isn't there probably should be.
> > > 
> > > DeviceState->info->vmsd->name for the name.
> > > Dunno about the numeric id, I think savevm.c doesn't export it.
> > 
> > Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> > It seems like this highlights a deficiency in the vmstate matching
> 
> Why are you forcing this to be a string?

It seemed like a good way to send an identifier.  What do you suggest?

Alex
Chris Wright - June 10, 2010, 4:40 p.m.
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > to the identify the device. It should probably do this the same way that we 
> > identify the saved state for the device.  Currently I think this is an 
> > arbitrary vmstate name/id, but I expect this to change to a qdev address
> > (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> 
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.

In the fun game of what ifs...

The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
matched on source and target).  The source does hotunplug of 04.0 and
replaces it w/ new device.  I think we need something that is more
uniquely identifying the block.  Not sure that device name is correct or
a generation ID.

thanks,
-chris
Paul Brook - June 10, 2010, 4:45 p.m.
> > > to the identify the device. It should probably do this the same way
> > > that we identify the saved state for the device.  Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> > 
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
> 
> In the fun game of what ifs...
> 
> The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
> matched on source and target).  The source does hotunplug of 04.0 and
> replaces it w/ new device.  I think we need something that is more
> uniquely identifying the block.  Not sure that device name is correct or
> a generation ID.

You shouldn't be solving this problem for RAM blocks. You should be solving it 
for the device state.

Paul
Gerd Hoffmann - June 11, 2010, 8:45 a.m.
On 06/10/10 16:33, Alex Williamson wrote:
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
>>> I may have been a bit misleading here. What we really want to do is use the
>>> same matching algorithm as is used by the rest of the device state. Currently
>>> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
>>> there's a convenient link from a device to its associated vmstate - if there
>>> isn't there probably should be.
>>
>> DeviceState->info->vmsd->name for the name.
>> Dunno about the numeric id, I think savevm.c doesn't export it.
>
> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching
> though.  If on the source we do:
>
>> pci_add addr=4 nic model=e1000
>> pci_add addr=3 nic model=e1000
>
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?

I think so.  We should be able to handle that better.  Nevertheless it 
makes sense to use the same naming scheme for device state and device 
ram.  If we fix this for the device state some day (using qdev most 
likely), we'll go change device ram handling the same way.

cheers,
   Gerd
Gerd Hoffmann - June 11, 2010, 8:48 a.m.
On 06/10/10 17:21, Alex Williamson wrote:
> On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
>>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
>>> It seems like this highlights a deficiency in the vmstate matching
>>
>> Why are you forcing this to be a string?
>
> It seemed like a good way to send an identifier.  What do you suggest?

Do it the same way savevm handles device state?  I think it simply puts 
a u32 for the instance id.

cheers,
   Gerd
Alex Williamson - June 11, 2010, 3:50 p.m.
On Fri, 2010-06-11 at 10:48 +0200, Gerd Hoffmann wrote:
> On 06/10/10 17:21, Alex Williamson wrote:
> > On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> >>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> >>> It seems like this highlights a deficiency in the vmstate matching
> >>
> >> Why are you forcing this to be a string?
> >
> > It seemed like a good way to send an identifier.  What do you suggest?
> 
> Do it the same way savevm handles device state?  I think it simply puts 
> a u32 for the instance id.

I see, so then we'd have:

uint8 - string length (should we decide to go with a variable length)
buffer - "<vmsd->name>\<driver string>"
uint32 - instance_id

I'm not sure I see the benefit to that since then we'd have to do both a
strcmp and an instance_id match.  It's unlikely we'll have instance_ids
large enough that even make it a space savings in the protocol stream
versus "name.id" ("e1000.0").

The trouble I'm running into is that the SaveStateEntry.instance_id is
effectively private, and there's no easy way to associate a
SaveStateEntry to a device since it passes an opaque pointer, which
could be whatever the driver decides it wants.  I'm wondering if we
should pass the DeviceState pointer in when registering the vmstate so
that we can stuff the instance_id into the DeviceInfo.  Or maybe
DeviceInfo should include a pointer to the SaveStateEntry.  It all seems
pretty messy.  Any thoughts?

Alex
Paul Brook - June 11, 2010, 4:12 p.m.
> The trouble I'm running into is that the SaveStateEntry.instance_id is
> effectively private, and there's no easy way to associate a
> SaveStateEntry to a device since it passes an opaque pointer, which
> could be whatever the driver decides it wants.  I'm wondering if we
> should pass the DeviceState pointer in when registering the vmstate so
> that we can stuff the instance_id into the DeviceInfo.  Or maybe
> DeviceInfo should include a pointer to the SaveStateEntry.  It all seems
> pretty messy.  Any thoughts?

DeviceInfo is effectively a const structure (it may be modified at startup, 
but there's only one of it shared between multiple devices). I suspect you 
mean DeviceState.

Most likely a lot of the messyness is because we still have devices that have 
not been qdev-ified, so the VMState code can't assume it has a device. We 
should fix this.

Paul

Patch

diff --git a/cpu-all.h b/cpu-all.h
index 458cb4b..a5b886a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -865,6 +865,7 @@  typedef struct RAMBlock {
     uint8_t *host;
     ram_addr_t offset;
     ram_addr_t length;
+    char name[64];
     QLIST_ENTRY(RAMBlock) next;
 } RAMBlock;
 
diff --git a/cpu-common.h b/cpu-common.h
index 4b0ba60..5b00544 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -40,8 +40,8 @@  static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
 }
 
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
-ram_addr_t qemu_ram_alloc(ram_addr_t);
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
 void *qemu_get_ram_ptr(ram_addr_t addr);
diff --git a/exec.c b/exec.c
index d785de3..dd57515 100644
--- a/exec.c
+++ b/exec.c
@@ -2774,13 +2774,15 @@  static void *file_ram_alloc(ram_addr_t memory, const char *path)
 }
 #endif
 
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
 {
     RAMBlock *new_block;
 
     size = TARGET_PAGE_ALIGN(size);
     new_block = qemu_malloc(sizeof(*new_block));
 
+    // XXX check duplicates
+    snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
     new_block->host = host;
 
     new_block->offset = ram.last_offset;
@@ -2801,7 +2803,7 @@  ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
     return new_block->offset;
 }
 
-ram_addr_t qemu_ram_alloc(ram_addr_t size)
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
 {
     void *host;
 
@@ -2833,7 +2835,7 @@  ram_addr_t qemu_ram_alloc(ram_addr_t size)
 #endif
     }
 
-    return qemu_ram_map(size, host);
+    return qemu_ram_map(name, size, host);
 }
 
 void qemu_ram_free(ram_addr_t addr)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 2b963b5..1d631f6 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -569,9 +569,13 @@  static int assigned_dev_register_regions(PCIRegion *io_regions,
 
             if (!slow_map) {
                 void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
+                char name[14];
 
-                pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
-                                                                virtbase);
+                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
+                         PCI_SLOT(pci_dev->dev.devfn),
+                         PCI_FUNC(pci_dev->dev.devfn), i);
+                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
+                                                  cur_region->size, virtbase);
             } else
                 pci_dev->v_addrs[i].memory_index = 0;
 
diff --git a/hw/pc.c b/hw/pc.c
index eae0db4..76be151 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -834,7 +834,7 @@  void pc_memory_init(ram_addr_t ram_size,
     linux_boot = (kernel_filename != NULL);
 
     /* allocate RAM */
-    ram_addr = qemu_ram_alloc(below_4g_mem_size);
+    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
     cpu_register_physical_memory(0, 0xa0000, ram_addr);
     cpu_register_physical_memory(0x100000,
                  below_4g_mem_size - 0x100000,
@@ -845,7 +845,7 @@  void pc_memory_init(ram_addr_t ram_size,
 #if TARGET_PHYS_ADDR_BITS == 32
         hw_error("To much RAM for 32-bit physical address");
 #else
-        ram_addr = qemu_ram_alloc(above_4g_mem_size);
+        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
         cpu_register_physical_memory(0x100000000ULL,
                                      above_4g_mem_size,
                                      ram_addr);
@@ -866,7 +866,7 @@  void pc_memory_init(ram_addr_t ram_size,
         (bios_size % 65536) != 0) {
         goto bios_error;
     }
-    bios_offset = qemu_ram_alloc(bios_size);
+    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
     if (ret != 0) {
     bios_error:
@@ -892,7 +892,7 @@  void pc_memory_init(ram_addr_t ram_size,
     }
     option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
 
-    option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
+    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
     cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
 
     /* map all the bios at the top of memory */
diff --git a/hw/pci.c b/hw/pci.c
index f0b6790..b2632a8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1889,6 +1889,7 @@  static int pci_add_option_rom(PCIDevice *pdev)
     int size;
     char *path;
     void *ptr;
+    char name[13];
 
     if (!pdev->romfile)
         return 0;
@@ -1924,7 +1925,9 @@  static int pci_add_option_rom(PCIDevice *pdev)
         size = 1 << qemu_fls(size);
     }
 
-    pdev->rom_offset = qemu_ram_alloc(size);
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);
 
     ptr = qemu_get_ram_ptr(pdev->rom_offset);
     load_image(path, ptr);
diff --git a/hw/vga.c b/hw/vga.c
index 0a535ae..47b800f 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2292,7 +2292,7 @@  void vga_common_init(VGACommonState *s, int vga_ram_size)
 #else
     s->is_vbe_vmstate = 0;
 #endif
-    s->vram_offset = qemu_ram_alloc(vga_ram_size);
+    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
     s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
     s->vram_size = vga_ram_size;
     s->get_bpp = vga_get_bpp;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index bf2a699..9df8c08 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1164,7 +1164,7 @@  static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
 
 
     s->fifo_size = SVGA_FIFO_SIZE;
-    s->fifo_offset = qemu_ram_alloc(s->fifo_size);
+    s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
     s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
 
     vga_common_init(&s->vga, vga_ram_size);