| Message ID | 20100608191557.4451.30384.stgit@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
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
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
* 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
> 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
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 > >
> 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
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
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
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
On 06/09/2010 12:41 AM, Chris Wright wrote: > pci. > D.F. (B:D.F?) > D:B:D.F > bar > rom > bar.n
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.
> > > 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
> * 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
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. >
> 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.
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
> > 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
> 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
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
> 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
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
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
* 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
> > > 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
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
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
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
> 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
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);
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(-)