From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Commit 355477f8c73e9 skips rom reset when we're an incoming migration
so as not to overwrite shared ram in the ignore-shared migration
optimisation.
However, it's got an unexpected side effect that because it skips
freeing the ROM data, when rom_reset gets called later on, after
migration (e.g. during a reboot), the ROM does get reset to the original
file contents. Because of seabios/x86's weird reboot process
this confuses a reboot into hanging after a migration.
Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration")
https://bugzilla.redhat.com/show_bug.cgi?id=1809380
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/core/loader.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index d1b78f60cd..4e583eb3bd 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1119,19 +1119,24 @@ static void rom_reset(void *unused)
{
Rom *rom;
- /*
- * 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 on ARM
- * so this is probably the only right thing to do here.
- */
- if (runstate_check(RUN_STATE_INMIGRATE))
- return;
-
QTAILQ_FOREACH(rom, &roms, 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 on ARM
+ * so this is probably the only right thing to do here.
+ */
+ if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) {
+ /*
+ * Free it so that a rom_reset after migration doesn't overwrite a
+ * potentially modified 'rom'.
+ */
+ rom_free_data(rom);
+ }
+
if (rom->data == NULL) {
continue;
}
--
2.24.1
On Fri, 13 Mar 2020 at 12:31, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Commit 355477f8c73e9 skips rom reset when we're an incoming migration > so as not to overwrite shared ram in the ignore-shared migration > optimisation. > However, it's got an unexpected side effect that because it skips > freeing the ROM data, when rom_reset gets called later on, after > migration (e.g. during a reboot), the ROM does get reset to the original > file contents. Because of seabios/x86's weird reboot process > this confuses a reboot into hanging after a migration. > > Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration") > https://bugzilla.redhat.com/show_bug.cgi?id=1809380 > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/core/loader.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > QTAILQ_FOREACH(rom, &roms, 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 on ARM > + * so this is probably the only right thing to do here. > + */ > + if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) { > + /* > + * Free it so that a rom_reset after migration doesn't overwrite a > + * potentially modified 'rom'. > + */ > + rom_free_data(rom); Shouldn't this condition match the condition in rom_reset() for when we call rom_free_data()? You want the behaviour on a subsequent reset to match the behaviour you'd get if you did a reset on the source end without the migration. > + } > + > if (rom->data == NULL) { > continue; > } If you put this check above your new one you wouldn't need to check rom->data in it. Also it would make the loop structure better match rom_reset(), which checks rom->fw_file first, then rom->data, then has the condition for "do we need to call rom_free_data()". thanks -- PMM
On Fri, 13 Mar 2020 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 13 Mar 2020 at 12:31, Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Commit 355477f8c73e9 skips rom reset when we're an incoming migration > > so as not to overwrite shared ram in the ignore-shared migration > > optimisation. > > However, it's got an unexpected side effect that because it skips > > freeing the ROM data, when rom_reset gets called later on, after > > migration (e.g. during a reboot), the ROM does get reset to the original > > file contents. Because of seabios/x86's weird reboot process > > this confuses a reboot into hanging after a migration. > > > > Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration") > > https://bugzilla.redhat.com/show_bug.cgi?id=1809380 > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > hw/core/loader.c | 23 ++++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > QTAILQ_FOREACH(rom, &roms, 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 on ARM > > + * so this is probably the only right thing to do here. > > + */ > > + if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) { > > + /* > > + * Free it so that a rom_reset after migration doesn't overwrite a > > + * potentially modified 'rom'. > > + */ > > + rom_free_data(rom); > > Shouldn't this condition match the condition in rom_reset() > for when we call rom_free_data()? You want the behaviour > on a subsequent reset to match the behaviour you'd get > if you did a reset on the source end without the migration. Wait, this *is* rom_reset(). Now I'm really confused. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Fri, 13 Mar 2020 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Fri, 13 Mar 2020 at 12:31, Dr. David Alan Gilbert (git) > > <dgilbert@redhat.com> wrote: > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Commit 355477f8c73e9 skips rom reset when we're an incoming migration > > > so as not to overwrite shared ram in the ignore-shared migration > > > optimisation. > > > However, it's got an unexpected side effect that because it skips > > > freeing the ROM data, when rom_reset gets called later on, after > > > migration (e.g. during a reboot), the ROM does get reset to the original > > > file contents. Because of seabios/x86's weird reboot process > > > this confuses a reboot into hanging after a migration. > > > > > > Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration") > > > https://bugzilla.redhat.com/show_bug.cgi?id=1809380 > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > hw/core/loader.c | 23 ++++++++++++++--------- > > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > > > > QTAILQ_FOREACH(rom, &roms, 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 on ARM > > > + * so this is probably the only right thing to do here. > > > + */ > > > + if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) { > > > + /* > > > + * Free it so that a rom_reset after migration doesn't overwrite a > > > + * potentially modified 'rom'. > > > + */ > > > + rom_free_data(rom); > > > > Shouldn't this condition match the condition in rom_reset() > > for when we call rom_free_data()? You want the behaviour > > on a subsequent reset to match the behaviour you'd get > > if you did a reset on the source end without the migration. > > Wait, this *is* rom_reset(). Now I'm really confused. The exsiting rom_reset gets called multiple times: a) During init This actually copies the ROMs and then calls rom_free_data b) During a subsequent reboot This is mostly skipped because rom->data is now free because of the prior call to rom_free_data during (a) During an inbound migrate, (a) happens before the migration, and (b) happens during a reboot after the migration. The problem is that 355477f8c73e9 caused (a) to be skipped then when (b) happens it actually overwrites the ROM because the rom_free_data had been skipped. What I'm doing here is doing the rom_free_data(..) which causes it to then skip this iteration during (a) AND causes it to skip it during (b). Dave > thanks > -- PMM > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, 13 Mar 2020 at 13:34, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > On Fri, 13 Mar 2020 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Fri, 13 Mar 2020 at 12:31, Dr. David Alan Gilbert (git) > > > <dgilbert@redhat.com> wrote: > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > Commit 355477f8c73e9 skips rom reset when we're an incoming migration > > > > so as not to overwrite shared ram in the ignore-shared migration > > > > optimisation. > > > > However, it's got an unexpected side effect that because it skips > > > > freeing the ROM data, when rom_reset gets called later on, after > > > > migration (e.g. during a reboot), the ROM does get reset to the original > > > > file contents. Because of seabios/x86's weird reboot process > > > > this confuses a reboot into hanging after a migration. > > > > > > > > Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration") > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1809380 > > > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > --- > > > > hw/core/loader.c | 23 ++++++++++++++--------- > > > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > > > > > > > QTAILQ_FOREACH(rom, &roms, 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 on ARM > > > > + * so this is probably the only right thing to do here. > > > > + */ > > > > + if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) { > > > > + /* > > > > + * Free it so that a rom_reset after migration doesn't overwrite a > > > > + * potentially modified 'rom'. > > > > + */ > > > > + rom_free_data(rom); > > > > > > Shouldn't this condition match the condition in rom_reset() > > > for when we call rom_free_data()? You want the behaviour > > > on a subsequent reset to match the behaviour you'd get > > > if you did a reset on the source end without the migration. > > > > Wait, this *is* rom_reset(). Now I'm really confused. > > The exsiting rom_reset gets called multiple times: > a) During init > This actually copies the ROMs and then calls rom_free_data > > b) During a subsequent reboot > This is mostly skipped because rom->data is now free because > of the prior call to rom_free_data during (a) > > During an inbound migrate, (a) happens before the migration, and > (b) happens during a reboot after the migration. > > The problem is that 355477f8c73e9 caused (a) to be skipped > then when (b) happens it actually overwrites the ROM because > the rom_free_data had been skipped. What I'm doing here is > doing the rom_free_data(..) which causes it to then skip this > iteration during (a) AND causes it to skip it during (b). OK, but why is your condition for when to call rom_free_data() in this special case not the same as the condition that we use in the normal no-migration-involved case? I would expect those to match up. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Fri, 13 Mar 2020 at 13:34, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > > On Fri, 13 Mar 2020 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > On Fri, 13 Mar 2020 at 12:31, Dr. David Alan Gilbert (git) > > > > <dgilbert@redhat.com> wrote: > > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > Commit 355477f8c73e9 skips rom reset when we're an incoming migration > > > > > so as not to overwrite shared ram in the ignore-shared migration > > > > > optimisation. > > > > > However, it's got an unexpected side effect that because it skips > > > > > freeing the ROM data, when rom_reset gets called later on, after > > > > > migration (e.g. during a reboot), the ROM does get reset to the original > > > > > file contents. Because of seabios/x86's weird reboot process > > > > > this confuses a reboot into hanging after a migration. > > > > > > > > > > Fixes: 355477f8c73e9 ("migration: do not rom_reset() during incoming migration") > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1809380 > > > > > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > --- > > > > > hw/core/loader.c | 23 ++++++++++++++--------- > > > > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > QTAILQ_FOREACH(rom, &roms, 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 on ARM > > > > > + * so this is probably the only right thing to do here. > > > > > + */ > > > > > + if (runstate_check(RUN_STATE_INMIGRATE) && rom->data) { > > > > > + /* > > > > > + * Free it so that a rom_reset after migration doesn't overwrite a > > > > > + * potentially modified 'rom'. > > > > > + */ > > > > > + rom_free_data(rom); > > > > > > > > Shouldn't this condition match the condition in rom_reset() > > > > for when we call rom_free_data()? You want the behaviour > > > > on a subsequent reset to match the behaviour you'd get > > > > if you did a reset on the source end without the migration. > > > > > > Wait, this *is* rom_reset(). Now I'm really confused. > > > > The exsiting rom_reset gets called multiple times: > > a) During init > > This actually copies the ROMs and then calls rom_free_data > > > > b) During a subsequent reboot > > This is mostly skipped because rom->data is now free because > > of the prior call to rom_free_data during (a) > > > > During an inbound migrate, (a) happens before the migration, and > > (b) happens during a reboot after the migration. > > > > The problem is that 355477f8c73e9 caused (a) to be skipped > > then when (b) happens it actually overwrites the ROM because > > the rom_free_data had been skipped. What I'm doing here is > > doing the rom_free_data(..) which causes it to then skip this > > iteration during (a) AND causes it to skip it during (b). > > OK, but why is your condition for when to call rom_free_data() > in this special case not the same as the condition that we > use in the normal no-migration-involved case? I would expect > those to match up. Ah yes, I think you're right, so something like: if (runstate_check(RUN_STATE_INMIGRATE) && rom->data && rom->isrom) { I'll try that after lunch. Dave > thanks > -- PMM > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, 13 Mar 2020 at 13:57, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > Ah yes, I think you're right, so something like: > > if (runstate_check(RUN_STATE_INMIGRATE) && rom->data && rom->isrom) { I think you would see the difference here for images loaded into RAM, rather than ROM -- they need to be reinstated on reset, because the guest can scribble on them. So we retain the data and don't free it. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Fri, 13 Mar 2020 at 13:57, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > Ah yes, I think you're right, so something like: > > > > if (runstate_check(RUN_STATE_INMIGRATE) && rom->data && rom->isrom) { > > I think you would see the difference here for images > loaded into RAM, rather than ROM -- they need to be > reinstated on reset, because the guest can scribble > on them. So we retain the data and don't free it. Hmm, that's true; so I'm failing to skip a copy in the !isrom case, whch the original patch needed. So what I think we'll need is: if (runstate_check(RUN_STATE_INMIGRATE)) { if (rom->data && rom->isrom) { rom_free_data(rom); } continue; } > thanks > -- PMM > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2024 Red Hat, Inc.