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
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
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 name>.instance>\. 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 name>.instance>\. > 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 name>.instance>\. > > 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/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 doname>.instance>\. 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 doname>.instance>\. 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 doname>.instance>\.
> >>> 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 - "name>\"
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
