Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 09.05.23 18:54, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 02:39:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 13:05, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 12:20, David Hildenbrand wrote:
On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
- void *ptr;
char name[32];
const VMStateDescription *vmsd;
+ /*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+ bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
- path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
- if (path == NULL) {
- path = g_strdup(pdev->romfile);
- }
+ if (load_file || pdev->romsize == -1) {
+ path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+ if (path == NULL) {
+ path = g_strdup(pdev->romfile);
+ }
- size = get_image_size(path);
- if (size < 0) {
- error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
- return;
- } else if (size == 0) {
- error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
- return;
- } else if (size > 2 * GiB) {
- error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
- pdev->romfile);
- return;
- }
- if (pdev->romsize != -1) {
- if (size > pdev->romsize) {
- error_setg(errp, "romfile \"%s\" (%u bytes) "
- "is too large for ROM size %u",
- pdev->romfile, (uint32_t)size, pdev->romsize);
+ size = get_image_size(path);
+ if (size < 0) {
+ error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+ return;
+ } else if (size == 0) {
+ error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+ return;
+ } else if (size > 2 * GiB) {
+ error_setg(errp,
+ "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+ pdev->romfile);
return;
}
- } else {
- pdev->romsize =
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 09.05.23 17:54, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 02:39:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 13:05, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 12:20, David Hildenbrand wrote:
On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
- void *ptr;
char name[32];
const VMStateDescription *vmsd;
+ /*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+ bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
- path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
- if (path == NULL) {
- path = g_strdup(pdev->romfile);
- }
+ if (load_file || pdev->romsize == -1) {
+ path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+ if (path == NULL) {
+ path = g_strdup(pdev->romfile);
+ }
- size = get_image_size(path);
- if (size < 0) {
- error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
- return;
- } else if (size == 0) {
- error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
- return;
- } else if (size > 2 * GiB) {
- error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
- pdev->romfile);
- return;
- }
- if (pdev->romsize != -1) {
- if (size > pdev->romsize) {
- error_setg(errp, "romfile \"%s\" (%u bytes) "
- "is too large for ROM size %u",
- pdev->romfile, (uint32_t)size, pdev->romsize);
+ size = get_image_size(path);
+ if (size < 0) {
+ error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+ return;
+ } else if (size == 0) {
+ error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+ return;
+ } else if (size > 2 * GiB) {
+ error_setg(errp,
+ "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+ pdev->romfile);
return;
}
- } else {
- pdev->romsize =
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On Wed, May 03, 2023 at 02:39:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 03.05.23 13:05, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy
> > wrote:
> > > On 03.05.23 12:20, David Hildenbrand wrote:
> > > > On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
> > > > > On incoming migration we have the following sequence to load option
> > > > > ROM:
> > > > >
> > > > > 1. On device realize we do normal load ROM from the file
> > > > >
> > > > > 2. Than, on incoming migration we rewrite ROM from the incoming RAM
> > > > > block. If sizes mismatch we fail.
> > > > >
> > > > > This is not ideal when we migrate to updated distribution: we have to
> > > > > keep old ROM files in new distribution and be careful around romfile
> > > > > property to load correct ROM file. Which is loaded actually just to
> > > > > allocate the ROM with correct length.
> > > > >
> > > > > Note, that romsize property doesn't really help: if we try to specify
> > > > > it when default romfile is larger, it fails with something like:
> > > > >
> > > > > romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size
> > > > > 65536
> > > > >
> > > > > Let's just ignore ROM file when romsize is specified and we are in
> > > > > incoming migration state. In other words, we need only to preallocate
> > > > > ROM of specified size, local ROM file is unrelated.
> > > > >
> > > > > This way:
> > > > >
> > > > > If romsize was specified on source, we just use same commandline as on
> > > > > source, and migration will work independently of local ROM files on
> > > > > target.
> > > > >
> > > > > If romsize was not specified on source (and we have mismatching local
> > > > > ROM file on target host), we have to specify romsize on target to
> > > > > match
> > > > > source romsize. romfile parameter may be kept same as on source or may
> > > > > be dropped, the file is not loaded anyway.
> > > > >
> > > > > As a bonus we avoid extra reading from ROM file on target.
> > > > >
> > > > > Note: when we don't have romsize parameter on source command line and
> > > > > need it for target, it may be calculated as aligned up to power of two
> > > > > size of ROM file on source (if we know, which file is it) or,
> > > > > alternatively it may be retrieved from source QEMU by QMP qom-get
> > > > > command, like
> > > > >
> > > > > { "execute": "qom-get",
> > > > > "arguments": {
> > > > > "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
> > > > > "property": "size" } }
> > > > >
> > > > > Suggested-by: Michael S. Tsirkin
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > > >
> > > > > ---
> > > > > hw/pci/pci.c | 77
> > > > > ++--
> > > > > 1 file changed, 45 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index a442f8fce1..e2cab622e4 100644
> > > > > --- a/hw/pci/pci.c
> > > > > +++ b/hw/pci/pci.c
> > > > > @@ -36,6 +36,7 @@
> > > > > #include "migration/vmstate.h"
> > > > > #include "net/net.h"
> > > > > #include "sysemu/numa.h"
> > > > > +#include "sysemu/runstate.h"
> > > > > #include "sysemu/sysemu.h"
> > > > > #include "hw/loader.h"
> > > > > #include "qemu/error-report.h"
> > > > > @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice
> > > > > *pdev, bool is_default_rom,
> > > > > {
> > > > > int64_t size;
> > > > > g_autofree char *path = NULL;
> > > > > - void *ptr;
> > > > > char name[32];
> > > > > const VMStateDescription *vmsd;
> > > > > + /*
> > > > > + * In case of incoming migration ROM will come with migration
> > > > > stream, no
> > > > > + * reason to load the file. Neither we want to fail if local
> > > > > ROM file
> > > > > + * mismatches with specified romsize.
> > > > > + */
> > > > > + bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> > > > > +
> > > > > if (!pdev->romfile) {
> > > > > return;
> > > > > }
> > > > > @@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice
> > > > > *pdev, bool is_default_rom,
> > > > > return;
> > > > > }
> > > > > - path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> > > > > - if (path == NULL) {
> > > > > - path = g_strdup(pdev->romfile);
> > > > > - }
> > > > > + if (load_file || pdev->romsize == -1) {
> > > > > + path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> > > > > + if (path == NULL) {
> > > > > + path = g_strdup(pdev->romfile);
> > > > > + }
> > > > > - size = get_image_size(path);
> > > > > - if (size < 0) {
> > > > > - error_setg(errp, "failed to find romfile \"%s\"",
> > > > > pdev->romfile);
> > > > > - return;
> > > > > - } else if (size == 0) {
> > > > > - error_setg(errp, "romfile \"%s\"
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
"Michael S. Tsirkin" wrote:
> On Tue, May 02, 2023 at 12:11:38PM +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" wrote:
>>
>> >> > CC pbonzini,dgilbert,quintela,armbru : guys, is poking at
>> >> > runstate_check like
>> >> > this the right way to figure out we are not going to use the
>> >> > device locally before incoming migration will overwrite ROM contents?
>> >>
>> >> RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when
>> >> we parse cmdline option -incoming. VM is not running for sure. And
>> >> starting the VM comes with changing the state. So it's OK.
>> >>
>> >> The possible problem, if we add netcard on target which we didn't
>> >> have on source. I now checked, this works.. But that doesn't seem
>> >> correct to add device that was not present on source - how would it
>> >> work - it's not guaranteed anyway.
>> >
>> > You can add it on source too while migration is in progress, no?
>>
>> DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>> bool from_json, Error **errp)
>> {
>>
>> if (!migration_is_idle()) {
>> error_setg(errp, "device_add not allowed while migrating");
>> return NULL;
>> }
>>
>> It should be similar for unplug.
>>
>> We only support hotplug for some devices during migration, and we
>> shouldn't need any.
>>
>> What I think he means is that you can add a device on the command line
>> on destination that don't exist on the source machine, and that will
>> confuse things.
>>
>> In that case, I would say that the problem is that you are doing
>> something not supported. You are expected that when you run migration
>> you use the same command line that on source, module whatever
>> hot[un]plug operations you have done before migration.
>>
>> Anything else is not supported.
>> And for instance, if you are using libvirt, it will do the right thing.
>>
>> Later, Juan.
>
> OK, so you ack this patch?
Reviewed-by: Juan Quintela
It is ok, or should I do it at toplevel?
Later, Juan.
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 03.05.23 13:05, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 12:20, David Hildenbrand wrote:
On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
- void *ptr;
char name[32];
const VMStateDescription *vmsd;
+ /*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+ bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
- path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
- if (path == NULL) {
- path = g_strdup(pdev->romfile);
- }
+ if (load_file || pdev->romsize == -1) {
+ path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+ if (path == NULL) {
+ path = g_strdup(pdev->romfile);
+ }
- size = get_image_size(path);
- if (size < 0) {
- error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
- return;
- } else if (size == 0) {
- error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
- return;
- } else if (size > 2 * GiB) {
- error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
- pdev->romfile);
- return;
- }
- if (pdev->romsize != -1) {
- if (size > pdev->romsize) {
- error_setg(errp, "romfile \"%s\" (%u bytes) "
- "is too large for ROM size %u",
- pdev->romfile, (uint32_t)size, pdev->romsize);
+ size = get_image_size(path);
+ if (size < 0) {
+ error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+ return;
+ } else if (size == 0) {
+ error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+ return;
+ } else if (size > 2 * GiB) {
+ error_setg(errp,
+ "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+ pdev->romfile);
return;
}
- } else {
- pdev->romsize = pow2ceil(size);
+ if (pdev->romsize != -1) {
+ if (size > pdev->romsize) {
+ error_setg(errp, "romfile \"%s\" (%u bytes) "
+
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 03.05.23 12:20, David Hildenbrand wrote:
> > On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
> > > On incoming migration we have the following sequence to load option
> > > ROM:
> > >
> > > 1. On device realize we do normal load ROM from the file
> > >
> > > 2. Than, on incoming migration we rewrite ROM from the incoming RAM
> > > block. If sizes mismatch we fail.
> > >
> > > This is not ideal when we migrate to updated distribution: we have to
> > > keep old ROM files in new distribution and be careful around romfile
> > > property to load correct ROM file. Which is loaded actually just to
> > > allocate the ROM with correct length.
> > >
> > > Note, that romsize property doesn't really help: if we try to specify
> > > it when default romfile is larger, it fails with something like:
> > >
> > > romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
> > >
> > > Let's just ignore ROM file when romsize is specified and we are in
> > > incoming migration state. In other words, we need only to preallocate
> > > ROM of specified size, local ROM file is unrelated.
> > >
> > > This way:
> > >
> > > If romsize was specified on source, we just use same commandline as on
> > > source, and migration will work independently of local ROM files on
> > > target.
> > >
> > > If romsize was not specified on source (and we have mismatching local
> > > ROM file on target host), we have to specify romsize on target to match
> > > source romsize. romfile parameter may be kept same as on source or may
> > > be dropped, the file is not loaded anyway.
> > >
> > > As a bonus we avoid extra reading from ROM file on target.
> > >
> > > Note: when we don't have romsize parameter on source command line and
> > > need it for target, it may be calculated as aligned up to power of two
> > > size of ROM file on source (if we know, which file is it) or,
> > > alternatively it may be retrieved from source QEMU by QMP qom-get
> > > command, like
> > >
> > > { "execute": "qom-get",
> > > "arguments": {
> > > "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
> > > "property": "size" } }
> > >
> > > Suggested-by: Michael S. Tsirkin
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > ---
> > > hw/pci/pci.c | 77 ++--
> > > 1 file changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index a442f8fce1..e2cab622e4 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -36,6 +36,7 @@
> > > #include "migration/vmstate.h"
> > > #include "net/net.h"
> > > #include "sysemu/numa.h"
> > > +#include "sysemu/runstate.h"
> > > #include "sysemu/sysemu.h"
> > > #include "hw/loader.h"
> > > #include "qemu/error-report.h"
> > > @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev,
> > > bool is_default_rom,
> > > {
> > > int64_t size;
> > > g_autofree char *path = NULL;
> > > - void *ptr;
> > > char name[32];
> > > const VMStateDescription *vmsd;
> > > + /*
> > > + * In case of incoming migration ROM will come with migration
> > > stream, no
> > > + * reason to load the file. Neither we want to fail if local ROM
> > > file
> > > + * mismatches with specified romsize.
> > > + */
> > > + bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> > > +
> > > if (!pdev->romfile) {
> > > return;
> > > }
> > > @@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev,
> > > bool is_default_rom,
> > > return;
> > > }
> > > - path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> > > - if (path == NULL) {
> > > - path = g_strdup(pdev->romfile);
> > > - }
> > > + if (load_file || pdev->romsize == -1) {
> > > + path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> > > + if (path == NULL) {
> > > + path = g_strdup(pdev->romfile);
> > > + }
> > > - size = get_image_size(path);
> > > - if (size < 0) {
> > > - error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
> > > - return;
> > > - } else if (size == 0) {
> > > - error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
> > > - return;
> > > - } else if (size > 2 * GiB) {
> > > - error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2
> > > GiB)",
> > > - pdev->romfile);
> > > - return;
> > > - }
> > > - if (pdev->romsize != -1) {
> > > - if (size > pdev->romsize) {
> > > - error_setg(errp, "romfile \"%s\" (%u bytes) "
> > > - "is too large for ROM size %u",
> > > - pdev->romfile, (uint32_t)size, pdev->romsize);
> > > + size = get_image_size(path);
> > > + if (size < 0) {
> > > +
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 03.05.23 12:20, David Hildenbrand wrote:
On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
- void *ptr;
char name[32];
const VMStateDescription *vmsd;
+ /*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+ bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
- path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
- if (path == NULL) {
- path = g_strdup(pdev->romfile);
- }
+ if (load_file || pdev->romsize == -1) {
+ path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+ if (path == NULL) {
+ path = g_strdup(pdev->romfile);
+ }
- size = get_image_size(path);
- if (size < 0) {
- error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
- return;
- } else if (size == 0) {
- error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
- return;
- } else if (size > 2 * GiB) {
- error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
- pdev->romfile);
- return;
- }
- if (pdev->romsize != -1) {
- if (size > pdev->romsize) {
- error_setg(errp, "romfile \"%s\" (%u bytes) "
- "is too large for ROM size %u",
- pdev->romfile, (uint32_t)size, pdev->romsize);
+ size = get_image_size(path);
+ if (size < 0) {
+ error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+ return;
+ } else if (size == 0) {
+ error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+ return;
+ } else if (size > 2 * GiB) {
+ error_setg(errp,
+ "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+ pdev->romfile);
return;
}
- } else {
- pdev->romsize = pow2ceil(size);
+ if (pdev->romsize != -1) {
+ if (size > pdev->romsize) {
+ error_setg(errp, "romfile \"%s\" (%u bytes) "
+ "is too large for ROM size %u",
+ pdev->romfile, (uint32_t)size, pdev->romsize);
+
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 25.04.23 18:14, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
-void *ptr;
char name[32];
const VMStateDescription *vmsd;
+/*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
-path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
-if (path == NULL) {
-path = g_strdup(pdev->romfile);
-}
+if (load_file || pdev->romsize == -1) {
+path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+if (path == NULL) {
+path = g_strdup(pdev->romfile);
+}
-size = get_image_size(path);
-if (size < 0) {
-error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
-return;
-} else if (size == 0) {
-error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
-return;
-} else if (size > 2 * GiB) {
-error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
- pdev->romfile);
-return;
-}
-if (pdev->romsize != -1) {
-if (size > pdev->romsize) {
-error_setg(errp, "romfile \"%s\" (%u bytes) "
- "is too large for ROM size %u",
- pdev->romfile, (uint32_t)size, pdev->romsize);
+size = get_image_size(path);
+if (size < 0) {
+error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+return;
+} else if (size == 0) {
+error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+return;
+} else if (size > 2 * GiB) {
+error_setg(errp,
+ "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+ pdev->romfile);
return;
}
-} else {
-pdev->romsize = pow2ceil(size);
+if (pdev->romsize != -1) {
+if (size > pdev->romsize) {
+error_setg(errp, "romfile \"%s\" (%u bytes) "
+ "is too large for ROM size %u",
+ pdev->romfile, (uint32_t)size, pdev->romsize);
+return;
+}
+}
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On Tue, May 02, 2023 at 12:11:38PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" wrote:
>
> >> > CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check
> >> > like
> >> > this the right way to figure out we are not going to use the
> >> > device locally before incoming migration will overwrite ROM contents?
> >>
> >> RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when
> >> we parse cmdline option -incoming. VM is not running for sure. And
> >> starting the VM comes with changing the state. So it's OK.
> >>
> >> The possible problem, if we add netcard on target which we didn't
> >> have on source. I now checked, this works.. But that doesn't seem
> >> correct to add device that was not present on source - how would it
> >> work - it's not guaranteed anyway.
> >
> > You can add it on source too while migration is in progress, no?
>
> DeviceState *qdev_device_add_from_qdict(const QDict *opts,
> bool from_json, Error **errp)
> {
>
> if (!migration_is_idle()) {
> error_setg(errp, "device_add not allowed while migrating");
> return NULL;
> }
>
> It should be similar for unplug.
>
> We only support hotplug for some devices during migration, and we
> shouldn't need any.
>
> What I think he means is that you can add a device on the command line
> on destination that don't exist on the source machine, and that will
> confuse things.
>
> In that case, I would say that the problem is that you are doing
> something not supported. You are expected that when you run migration
> you use the same command line that on source, module whatever
> hot[un]plug operations you have done before migration.
>
> Anything else is not supported.
> And for instance, if you are using libvirt, it will do the right thing.
>
> Later, Juan.
OK, so you ack this patch?
--
MST
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 02.05.23 13:11, Juan Quintela wrote:
"Michael S. Tsirkin" wrote:
CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?
RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when
we parse cmdline option -incoming. VM is not running for sure. And
starting the VM comes with changing the state. So it's OK.
The possible problem, if we add netcard on target which we didn't
have on source. I now checked, this works.. But that doesn't seem
correct to add device that was not present on source - how would it
work - it's not guaranteed anyway.
You can add it on source too while migration is in progress, no?
DeviceState *qdev_device_add_from_qdict(const QDict *opts,
bool from_json, Error **errp)
{
if (!migration_is_idle()) {
error_setg(errp, "device_add not allowed while migrating");
return NULL;
}
It should be similar for unplug.
We only support hotplug for some devices during migration, and we
shouldn't need any.
What I think he means is that you can add a device on the command line
on destination that don't exist on the source machine, and that will
confuse things.
Yes, that what I mean
In that case, I would say that the problem is that you are doing
something not supported. You are expected that when you run migration
you use the same command line that on source, module whatever
hot[un]plug operations you have done before migration.
Agree
Anything else is not supported.
And for instance, if you are using libvirt, it will do the right thing.
Later, Juan.
Thanks!
--
Best regards,
Vladimir
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
"Michael S. Tsirkin" wrote:
>> > CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check
>> > like
>> > this the right way to figure out we are not going to use the
>> > device locally before incoming migration will overwrite ROM contents?
>>
>> RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when
>> we parse cmdline option -incoming. VM is not running for sure. And
>> starting the VM comes with changing the state. So it's OK.
>>
>> The possible problem, if we add netcard on target which we didn't
>> have on source. I now checked, this works.. But that doesn't seem
>> correct to add device that was not present on source - how would it
>> work - it's not guaranteed anyway.
>
> You can add it on source too while migration is in progress, no?
DeviceState *qdev_device_add_from_qdict(const QDict *opts,
bool from_json, Error **errp)
{
if (!migration_is_idle()) {
error_setg(errp, "device_add not allowed while migrating");
return NULL;
}
It should be similar for unplug.
We only support hotplug for some devices during migration, and we
shouldn't need any.
What I think he means is that you can add a device on the command line
on destination that don't exist on the source machine, and that will
confuse things.
In that case, I would say that the problem is that you are doing
something not supported. You are expected that when you run migration
you use the same command line that on source, module whatever
hot[un]plug operations you have done before migration.
Anything else is not supported.
And for instance, if you are using libvirt, it will do the right thing.
Later, Juan.
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 02.05.23 12:48, Michael S. Tsirkin wrote:
On Wed, Apr 26, 2023 at 11:00:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 26.04.23 07:43, Michael S. Tsirkin wrote:
On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
let's mention an example error message:
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file.
Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Something I'd like to clarify is that the comment applies to uses where
users/distributions supply their own ROM file. And lots of
users/distributions seem to have already painted themselves into a
corner by supplying a mix of ROM files of unmatching sizes -
basically they don't understand the detail of live migration,
ROM size interaction with it and with memory layout, etc -
as a very small number of people does.
For example, ubuntu doubled ROM file size by padding their ROMs
with 0x at some point, breaking migration for all existing machine
types.
just a web search for
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
will turn up a bunch of confused distros and users.
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
-void *ptr;
char name[32];
const VMStateDescription *vmsd;
+/*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?
RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when we parse
cmdline option -incoming. VM is not running for sure. And starting the VM comes
with changing the state. So it's OK.
The possible problem, if we add netcard on target which we didn't have on
source. I now checked, this works.. But that doesn't seem correct to add device
that was not present on source - how would it work - it's not guaranteed anyway.
You can add it on source too while migration is in progress, no?
No, as INMIGRATE - means incoming migration, it's only on target
--
Best regards,
Vladimir
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On Wed, Apr 26, 2023 at 11:00:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 26.04.23 07:43, Michael S. Tsirkin wrote:
> > On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy
> > wrote:
> > > On incoming migration we have the following sequence to load option
> > > ROM:
> > >
> > > 1. On device realize we do normal load ROM from the file
> > >
> > > 2. Than, on incoming migration we rewrite ROM from the incoming RAM
> > > block. If sizes mismatch we fail.
> >
> > let's mention an example error message:
> > Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8:
> > Invalid argument
> >
> >
> > >
> > > This is not ideal when we migrate to updated distribution: we have to
> > > keep old ROM files in new distribution and be careful around romfile
> > > property to load correct ROM file.
> >
> > > Which is loaded actually just to
> > > allocate the ROM with correct length.
> > > Note, that romsize property doesn't really help: if we try to specify
> > > it when default romfile is larger, it fails with something like:
> > >
> > > romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
> >
> > Something I'd like to clarify is that the comment applies to uses where
> > users/distributions supply their own ROM file. And lots of
> > users/distributions seem to have already painted themselves into a
> > corner by supplying a mix of ROM files of unmatching sizes -
> > basically they don't understand the detail of live migration,
> > ROM size interaction with it and with memory layout, etc -
> > as a very small number of people does.
> > For example, ubuntu doubled ROM file size by padding their ROMs
> > with 0x at some point, breaking migration for all existing machine
> > types.
> >
> > just a web search for
> > Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8:
> > Invalid argument
> >
> > will turn up a bunch of confused distros and users.
> >
> >
> > >
> > > Let's just ignore ROM file when romsize is specified and we are in
> > > incoming migration state. In other words, we need only to preallocate
> > > ROM of specified size, local ROM file is unrelated.
> >
> >
> >
> >
> > >
> > > This way:
> > >
> > > If romsize was specified on source, we just use same commandline as on
> > > source, and migration will work independently of local ROM files on
> > > target.
> > >
> > > If romsize was not specified on source (and we have mismatching local
> > > ROM file on target host), we have to specify romsize on target to match
> > > source romsize. romfile parameter may be kept same as on source or may
> > > be dropped, the file is not loaded anyway.
> > >
> > > As a bonus we avoid extra reading from ROM file on target.
> > >
> > > Note: when we don't have romsize parameter on source command line and
> > > need it for target, it may be calculated as aligned up to power of two
> > > size of ROM file on source (if we know, which file is it) or,
> > > alternatively it may be retrieved from source QEMU by QMP qom-get
> > > command, like
> > >
> > >{ "execute": "qom-get",
> > > "arguments": {
> > >"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
> > >"property": "size" } }
> > >
> > > Suggested-by: Michael S. Tsirkin
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > ---
> > > hw/pci/pci.c | 77 ++--
> > > 1 file changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index a442f8fce1..e2cab622e4 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -36,6 +36,7 @@
> > > #include "migration/vmstate.h"
> > > #include "net/net.h"
> > > #include "sysemu/numa.h"
> > > +#include "sysemu/runstate.h"
> > > #include "sysemu/sysemu.h"
> > > #include "hw/loader.h"
> > > #include "qemu/error-report.h"
> > > @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev,
> > > bool is_default_rom,
> > > {
> > > int64_t size;
> > > g_autofree char *path = NULL;
> > > -void *ptr;
> > > char name[32];
> > > const VMStateDescription *vmsd;
> > > +/*
> > > + * In case of incoming migration ROM will come with migration
> > > stream, no
> > > + * reason to load the file. Neither we want to fail if local ROM
> > > file
> > > + * mismatches with specified romsize.
> > > + */
> > > +bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> > > +
> > > if (!pdev->romfile) {
> > > return;
> > > }
> >
> > CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check
> > like
> > this the right way to figure out we are not going to use the
> > device locally before incoming migration will overwrite ROM contents?
>
> RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when we parse
> cmdline option -incoming. VM is not running for sure. And starting the VM
> comes
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 28.04.23 11:30, Juan Quintela wrote:
"Michael S. Tsirkin" wrote:
On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
let's mention an example error message:
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
This is a mess(TM).
And no easy way to fix it. Everything has its problems.
Ok, I will elaborate.
We have source machine and destination machine.
Easy case, same version of qemu (or at least the same rom files).
The interesting ones is when the sizes are wrong.
Again this splits on two cases:
- target side is bigger
not big deal, during migration we just don't use all the space.
But still doesn't work without my patch, as size mismatch -> migration fail.
Or, if you try to set romsize to match source, it fails on realize, when trying
to load ROM from file and see that specified size is smaller. (again my patch
fixes it).
- target side is smaller
guess what, not easy way to get this working O:-)
We added some changes on the past for this, but I don't remember the
details.
romsize parameter is added. Still, it can't help in all cases.
If I understood his patch correctly, it set seems to try to fix this to
always do the right thing with respect to migration, i.e. using whatever
was on the source. I think this is nice.
Yes. In details:
- If you didn't use romsize before, all you need is to specify correct romsize
on target, it will work, no matter which rom files you have on target
- If you already use romsize - just keep same parameters on target, it will
work, no matter which rom files you have on target.
But we still have left out the big elephant on the ROM, what happens
when we reboot.
Hmm. I now checked, seems nothing happen with these pci ROMs on reboot. They
are not reloaded. pci_qdev_realize() is not called again on reboot.. Or what I
miss?
Right now, when we reboot we still use the rom files for the source.
And I think that in the case of reboot, if the ROM files have changed
(because there was an upgrade or we migrate to a host with a never
version, etc,) we should always do a powerdown + start to let qemu use
the new ROM files.
As far as I know, no management app does that, and especially as we move
to UEFI (i.e. more complex firmware with more posibilities for CVE's) I
think we should considerd this case.
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
-void *ptr;
char name[32];
const VMStateDescription *vmsd;
+/*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?
There is only a way to get into RUN_STATE_INMIGRATE, and that is that we
have started the guest with --incoming . So the check does
what it is intended.
Once told that, I have never been seen it used for this.
/me launches grep on source tree
At least the block layer and usb use it exactly for this. So I will say
it is the correct way of doing it (or at least I can think of a better
way right now).
The grep also shows this:
static void rom_reset(void *unused)
{
Rom *rom;
QTAILQ_FOREACH(rom, , next) {
if (rom->fw_file) {
continue;
}
/*
* We don't need to fill in the RAM with ROM data because we'll fill
* the data in during the next incoming migration in all cases. Note
* that some of those RAMs can actually be modified by the guest.
*/
if (runstate_check(RUN_STATE_INMIGRATE)) {
if (rom->data && rom->isrom) {
/*
* Free it so that a rom_reset after migration doesn't
* overwrite a potentially modified 'rom'.
*/
rom_free_data(rom);
}
continue;
}
It is not exactly the problem at hand, but it is related. I am just
wondering if we can do something common.
Does these roms (of type Rom) from hw/core/loader.c relate to roms in hw/pci/pci.c, which
are "MemoryRegion" ?
--
Best regards,
Vladimir
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
"Michael S. Tsirkin" wrote:
> On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On incoming migration we have the following sequence to load option
>> ROM:
>>
>> 1. On device realize we do normal load ROM from the file
>>
>> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>>block. If sizes mismatch we fail.
>
> let's mention an example error message:
> Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
> argument
This is a mess(TM).
And no easy way to fix it. Everything has its problems.
Ok, I will elaborate.
We have source machine and destination machine.
Easy case, same version of qemu (or at least the same rom files).
The interesting ones is when the sizes are wrong.
Again this splits on two cases:
- target side is bigger
not big deal, during migration we just don't use all the space.
- target side is smaller
guess what, not easy way to get this working O:-)
We added some changes on the past for this, but I don't remember the
details.
If I understood his patch correctly, it set seems to try to fix this to
always do the right thing with respect to migration, i.e. using whatever
was on the source. I think this is nice.
But we still have left out the big elephant on the ROM, what happens
when we reboot.
Right now, when we reboot we still use the rom files for the source.
And I think that in the case of reboot, if the ROM files have changed
(because there was an upgrade or we migrate to a host with a never
version, etc,) we should always do a powerdown + start to let qemu use
the new ROM files.
As far as I know, no management app does that, and especially as we move
to UEFI (i.e. more complex firmware with more posibilities for CVE's) I
think we should considerd this case.
>> @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
>> is_default_rom,
>> {
>> int64_t size;
>> g_autofree char *path = NULL;
>> -void *ptr;
>> char name[32];
>> const VMStateDescription *vmsd;
>>
>> +/*
>> + * In case of incoming migration ROM will come with migration stream, no
>> + * reason to load the file. Neither we want to fail if local ROM file
>> + * mismatches with specified romsize.
>> + */
>> +bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
>> +
>> if (!pdev->romfile) {
>> return;
>> }
>
> CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
> this the right way to figure out we are not going to use the
> device locally before incoming migration will overwrite ROM contents?
There is only a way to get into RUN_STATE_INMIGRATE, and that is that we
have started the guest with --incoming . So the check does
what it is intended.
Once told that, I have never been seen it used for this.
/me launches grep on source tree
At least the block layer and usb use it exactly for this. So I will say
it is the correct way of doing it (or at least I can think of a better
way right now).
The grep also shows this:
static void rom_reset(void *unused)
{
Rom *rom;
QTAILQ_FOREACH(rom, , next) {
if (rom->fw_file) {
continue;
}
/*
* We don't need to fill in the RAM with ROM data because we'll fill
* the data in during the next incoming migration in all cases. Note
* that some of those RAMs can actually be modified by the guest.
*/
if (runstate_check(RUN_STATE_INMIGRATE)) {
if (rom->data && rom->isrom) {
/*
* Free it so that a rom_reset after migration doesn't
* overwrite a potentially modified 'rom'.
*/
rom_free_data(rom);
}
continue;
}
It is not exactly the problem at hand, but it is related. I am just
wondering if we can do something common.
Later, Juan.
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On 26.04.23 07:43, Michael S. Tsirkin wrote:
On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On incoming migration we have the following sequence to load option
ROM:
1. On device realize we do normal load ROM from the file
2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.
let's mention an example error message:
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file.
Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:
romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Something I'd like to clarify is that the comment applies to uses where
users/distributions supply their own ROM file. And lots of
users/distributions seem to have already painted themselves into a
corner by supplying a mix of ROM files of unmatching sizes -
basically they don't understand the detail of live migration,
ROM size interaction with it and with memory layout, etc -
as a very small number of people does.
For example, ubuntu doubled ROM file size by padding their ROMs
with 0x at some point, breaking migration for all existing machine
types.
just a web search for
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
will turn up a bunch of confused distros and users.
Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.
This way:
If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.
If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.
As a bonus we avoid extra reading from ROM file on target.
Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like
{ "execute": "qom-get",
"arguments": {
"path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
"property": "size" } }
Suggested-by: Michael S. Tsirkin
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/pci/pci.c | 77 ++--
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
#include "migration/vmstate.h"
#include "net/net.h"
#include "sysemu/numa.h"
+#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/loader.h"
#include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
{
int64_t size;
g_autofree char *path = NULL;
-void *ptr;
char name[32];
const VMStateDescription *vmsd;
+/*
+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file. Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
if (!pdev->romfile) {
return;
}
CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?
RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when we parse
cmdline option -incoming. VM is not running for sure. And starting the VM comes
with changing the state. So it's OK.
The possible problem, if we add netcard on target which we didn't have on
source. I now checked, this works.. But that doesn't seem correct to add device
that was not present on source - how would it work - it's not guaranteed anyway.
@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
is_default_rom,
return;
}
-path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
-if (path == NULL) {
-path = g_strdup(pdev->romfile);
-}
+if (load_file || pdev->romsize == -1) {
+path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+if (path == NULL) {
+path = g_strdup(pdev->romfile);
+
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On incoming migration we have the following sequence to load option
> ROM:
>
> 1. On device realize we do normal load ROM from the file
>
> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>block. If sizes mismatch we fail.
let's mention an example error message:
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
>
> This is not ideal when we migrate to updated distribution: we have to
> keep old ROM files in new distribution and be careful around romfile
> property to load correct ROM file.
> Which is loaded actually just to
> allocate the ROM with correct length.
> Note, that romsize property doesn't really help: if we try to specify
> it when default romfile is larger, it fails with something like:
>
> romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
Something I'd like to clarify is that the comment applies to uses where
users/distributions supply their own ROM file. And lots of
users/distributions seem to have already painted themselves into a
corner by supplying a mix of ROM files of unmatching sizes -
basically they don't understand the detail of live migration,
ROM size interaction with it and with memory layout, etc -
as a very small number of people does.
For example, ubuntu doubled ROM file size by padding their ROMs
with 0x at some point, breaking migration for all existing machine
types.
just a web search for
Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
argument
will turn up a bunch of confused distros and users.
>
> Let's just ignore ROM file when romsize is specified and we are in
> incoming migration state. In other words, we need only to preallocate
> ROM of specified size, local ROM file is unrelated.
>
> This way:
>
> If romsize was specified on source, we just use same commandline as on
> source, and migration will work independently of local ROM files on
> target.
>
> If romsize was not specified on source (and we have mismatching local
> ROM file on target host), we have to specify romsize on target to match
> source romsize. romfile parameter may be kept same as on source or may
> be dropped, the file is not loaded anyway.
>
> As a bonus we avoid extra reading from ROM file on target.
>
> Note: when we don't have romsize parameter on source command line and
> need it for target, it may be calculated as aligned up to power of two
> size of ROM file on source (if we know, which file is it) or,
> alternatively it may be retrieved from source QEMU by QMP qom-get
> command, like
>
> { "execute": "qom-get",
> "arguments": {
> "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
> "property": "size" } }
>
> Suggested-by: Michael S. Tsirkin
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/pci/pci.c | 77 ++--
> 1 file changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a442f8fce1..e2cab622e4 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -36,6 +36,7 @@
> #include "migration/vmstate.h"
> #include "net/net.h"
> #include "sysemu/numa.h"
> +#include "sysemu/runstate.h"
> #include "sysemu/sysemu.h"
> #include "hw/loader.h"
> #include "qemu/error-report.h"
> @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom,
> {
> int64_t size;
> g_autofree char *path = NULL;
> -void *ptr;
> char name[32];
> const VMStateDescription *vmsd;
>
> +/*
> + * In case of incoming migration ROM will come with migration stream, no
> + * reason to load the file. Neither we want to fail if local ROM file
> + * mismatches with specified romsize.
> + */
> +bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> +
> if (!pdev->romfile) {
> return;
> }
CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?
> @@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom,
> return;
> }
>
> -path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> -if (path == NULL) {
> -path = g_strdup(pdev->romfile);
> -}
> +if (load_file || pdev->romsize == -1) {
> +path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> +if (path == NULL) {
> +path = g_strdup(pdev->romfile);
> +}
>
> -size = get_image_size(path);
> -if (size < 0) {
> -error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
> -return;
> -} else if (size == 0) {
> -error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
> -return;
> -} else if (size > 2 * GiB) {
> -
