Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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 dovmsd-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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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 dovmsd-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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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 dovmsd-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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
* Alex Williamson (alex.william...@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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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(: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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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.
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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.
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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(: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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
[Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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.william...@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));
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, 0xa, ram_addr);
cpu_register_physical_memory(0x10,
below_4g_mem_size - 0x10,
@@ -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(0x1ULL,
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 =
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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
Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
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(:01:10.0). Anywhere closer? Thanks, Alex
