* [Qemu-devel] [PATCH 0/3] Arm migration fixes for 3.0 @ 2018-08-03 16:36 Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Peter Maydell @ 2018-08-03 16:36 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: patches, Juan Quintela, Dr . David Alan Gilbert This patchset primarily fixes problems with Arm migration induced by a bug in the core vmstate handling of subsections: currently the migration code incorrectly treats a subsection with no .needed function pointer as if it was the subsection list terminator -- it is ignored and so is everything after it. I did an audit of all uses of subsections in QEMU, and found that we had four which didn't define a .needed function (assuming that this meant "always needed", same as the semantics for not providing a .needed function for a toplevel vmsd). This patchset fixes them all up by providing a dummy needed function. It also fixes an error in vmstate_gicv3_cpu which was accidentally initializing .subsections twice and so ignoring one of the subsections. Strictly speaking only the first patch is a true regression from 2.12. Disclaimer: compile tested only as I have to rush out the door in a moment, but I wanted to get these on-list for review given that rc4 isn't too far away... I'll test them properly on Monday. thanks -- PMM Peter Maydell (3): hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu target/arm: Add dummy needed functions to M profile vmstate subsections hw/intc/arm_gicv3_common.c | 9 ++++++--- target/arm/machine.c | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function 2018-08-03 16:36 [Qemu-devel] [PATCH 0/3] Arm migration fixes for 3.0 Peter Maydell @ 2018-08-03 16:36 ` Peter Maydell 2018-08-03 17:29 ` Dr. David Alan Gilbert 2018-08-06 10:00 ` [Qemu-devel] [Qemu-arm] " Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections Peter Maydell 2 siblings, 2 replies; 8+ messages in thread From: Peter Maydell @ 2018-08-03 16:36 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: patches, Juan Quintela, Dr . David Alan Gilbert Currently the migration code incorrectly treats a subsection with no .needed function pointer as if it was the subsection list terminator -- it is ignored and so is everything after it. Work around this by giving vmstate_gicv3_gicd_no_migration_shift_bug a 'needed' function that always returns true. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This should go into 3.0 to avoid awkward migration compat problems: the no-migration-shift-bug subsection is new in 3.0. --- hw/intc/arm_gicv3_common.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index ff326b374ad..e58bc8b8105 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -203,10 +203,16 @@ static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque, return 0; } +static bool needed_always(void *opaque) +{ + return true; +} + const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = { .name = "arm_gicv3/gicd_no_migration_shift_bug", .version_id = 1, .minimum_version_id = 1, + .needed = needed_always, .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load, .post_load = gicv3_gicd_no_migration_shift_bug_post_load, .fields = (VMStateField[]) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell @ 2018-08-03 17:29 ` Dr. David Alan Gilbert 2018-08-06 10:00 ` [Qemu-devel] [Qemu-arm] " Peter Maydell 1 sibling, 0 replies; 8+ messages in thread From: Dr. David Alan Gilbert @ 2018-08-03 17:29 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches, Juan Quintela * Peter Maydell (peter.maydell@linaro.org) wrote: > Currently the migration code incorrectly treats a subsection with > no .needed function pointer as if it was the subsection list > terminator -- it is ignored and so is everything after it. > Work around this by giving vmstate_gicv3_gicd_no_migration_shift_bug > a 'needed' function that always returns true. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > This should go into 3.0 to avoid awkward migration compat problems: > the no-migration-shift-bug subsection is new in 3.0. > --- > hw/intc/arm_gicv3_common.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index ff326b374ad..e58bc8b8105 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -203,10 +203,16 @@ static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque, > return 0; > } > > +static bool needed_always(void *opaque) > +{ > + return true; > +} > + > const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = { > .name = "arm_gicv3/gicd_no_migration_shift_bug", > .version_id = 1, > .minimum_version_id = 1, > + .needed = needed_always, > .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load, > .post_load = gicv3_gicd_no_migration_shift_bug_post_load, > .fields = (VMStateField[]) { > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell 2018-08-03 17:29 ` Dr. David Alan Gilbert @ 2018-08-06 10:00 ` Peter Maydell 1 sibling, 0 replies; 8+ messages in thread From: Peter Maydell @ 2018-08-06 10:00 UTC (permalink / raw) To: qemu-arm, QEMU Developers Cc: Juan Quintela, Dr . David Alan Gilbert, patches, Shannon Zhao, Shannon Zhao Forgot to cc Shannon on this one. Given that the missing .needed function meant we weren't ever transmitting the subsection with the no-shift-bug flag, this ought to mean that for current master migration of KVM goes wrong (since the destination thinks the source has the bug when it does not). I'm just going to try to set up to test this... thanks -- PMM On 3 August 2018 at 17:36, Peter Maydell <peter.maydell@linaro.org> wrote: > Currently the migration code incorrectly treats a subsection with > no .needed function pointer as if it was the subsection list > terminator -- it is ignored and so is everything after it. > Work around this by giving vmstate_gicv3_gicd_no_migration_shift_bug > a 'needed' function that always returns true. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This should go into 3.0 to avoid awkward migration compat problems: > the no-migration-shift-bug subsection is new in 3.0. > --- > hw/intc/arm_gicv3_common.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index ff326b374ad..e58bc8b8105 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -203,10 +203,16 @@ static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque, > return 0; > } > > +static bool needed_always(void *opaque) > +{ > + return true; > +} > + > const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = { > .name = "arm_gicv3/gicd_no_migration_shift_bug", > .version_id = 1, > .minimum_version_id = 1, > + .needed = needed_always, > .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load, > .post_load = gicv3_gicd_no_migration_shift_bug_post_load, > .fields = (VMStateField[]) { > -- > 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu 2018-08-03 16:36 [Qemu-devel] [PATCH 0/3] Arm migration fixes for 3.0 Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell @ 2018-08-03 16:36 ` Peter Maydell 2018-08-03 17:08 ` Dr. David Alan Gilbert 2018-08-03 16:36 ` [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections Peter Maydell 2 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-08-03 16:36 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: patches, Juan Quintela, Dr . David Alan Gilbert Commit 6692aac411199064 accidentally introduced a second initialization of the .subsections field of vmstate_gicv3_cpu, instead of adding the new subsection to the existing list. The effect of this was probably that migration of GICv3 with virtualization enabled was broken (or alternatively that migration of ICC_SRE_EL1 was broken, depending on which of the two initializers the compiler used). Combine the two into a single list. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Not strictly a 2.12 regression. --- hw/intc/arm_gicv3_common.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index e58bc8b8105..e1a8999cf5b 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -134,9 +134,6 @@ static const VMStateDescription vmstate_gicv3_cpu = { }, .subsections = (const VMStateDescription * []) { &vmstate_gicv3_cpu_virt, - NULL - }, - .subsections = (const VMStateDescription * []) { &vmstate_gicv3_cpu_sre_el1, NULL } -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu 2018-08-03 16:36 ` [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu Peter Maydell @ 2018-08-03 17:08 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 8+ messages in thread From: Dr. David Alan Gilbert @ 2018-08-03 17:08 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches, Juan Quintela * Peter Maydell (peter.maydell@linaro.org) wrote: > Commit 6692aac411199064 accidentally introduced a second initialization > of the .subsections field of vmstate_gicv3_cpu, instead of adding > the new subsection to the existing list. The effect of this was > probably that migration of GICv3 with virtualization enabled was > broken (or alternatively that migration of ICC_SRE_EL1 was broken, > depending on which of the two initializers the compiler used). > Combine the two into a single list. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > Not strictly a 2.12 regression. > --- > hw/intc/arm_gicv3_common.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index e58bc8b8105..e1a8999cf5b 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -134,9 +134,6 @@ static const VMStateDescription vmstate_gicv3_cpu = { > }, > .subsections = (const VMStateDescription * []) { > &vmstate_gicv3_cpu_virt, > - NULL > - }, > - .subsections = (const VMStateDescription * []) { > &vmstate_gicv3_cpu_sre_el1, > NULL > } > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections 2018-08-03 16:36 [Qemu-devel] [PATCH 0/3] Arm migration fixes for 3.0 Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu Peter Maydell @ 2018-08-03 16:36 ` Peter Maydell 2018-08-03 17:31 ` Dr. David Alan Gilbert 2 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-08-03 16:36 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: patches, Juan Quintela, Dr . David Alan Gilbert Currently the migration code incorrectly treats a subsection with no .needed function pointer as if it was the subsection list terminator -- it is ignored and so is everything after it. Work around this by giving various M profile vmstate structs a 'needed' function that always returns true. We reuse m_needed() for this, since it's always true here. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Not strictly a regression as it only affects M profile CPUs with the security extensions, and migration of those was broken anyway in 2.12 due to a different bug. --- target/arm/machine.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/target/arm/machine.c b/target/arm/machine.c index 2e28d086bdf..ff4ec22bf75 100644 --- a/target/arm/machine.c +++ b/target/arm/machine.c @@ -184,6 +184,7 @@ static const VMStateDescription vmstate_m_faultmask_primask = { .name = "cpu/m/faultmask-primask", .version_id = 1, .minimum_version_id = 1, + .needed = m_needed, .fields = (VMStateField[]) { VMSTATE_UINT32(env.v7m.faultmask[M_REG_NS], ARMCPU), VMSTATE_UINT32(env.v7m.primask[M_REG_NS], ARMCPU), @@ -230,6 +231,7 @@ static const VMStateDescription vmstate_m_scr = { .name = "cpu/m/scr", .version_id = 1, .minimum_version_id = 1, + .needed = m_needed, .fields = (VMStateField[]) { VMSTATE_UINT32(env.v7m.scr[M_REG_NS], ARMCPU), VMSTATE_END_OF_LIST() @@ -240,6 +242,7 @@ static const VMStateDescription vmstate_m_other_sp = { .name = "cpu/m/other-sp", .version_id = 1, .minimum_version_id = 1, + .needed = m_needed, .fields = (VMStateField[]) { VMSTATE_UINT32(env.v7m.other_sp, ARMCPU), VMSTATE_END_OF_LIST() -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections 2018-08-03 16:36 ` [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections Peter Maydell @ 2018-08-03 17:31 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 8+ messages in thread From: Dr. David Alan Gilbert @ 2018-08-03 17:31 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches, Juan Quintela * Peter Maydell (peter.maydell@linaro.org) wrote: > Currently the migration code incorrectly treats a subsection with > no .needed function pointer as if it was the subsection list > terminator -- it is ignored and so is everything after it. > Work around this by giving various M profile vmstate structs > a 'needed' function that always returns true. > We reuse m_needed() for this, since it's always true here. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > Not strictly a regression as it only affects M profile CPUs > with the security extensions, and migration of those was > broken anyway in 2.12 due to a different bug. > --- > target/arm/machine.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/target/arm/machine.c b/target/arm/machine.c > index 2e28d086bdf..ff4ec22bf75 100644 > --- a/target/arm/machine.c > +++ b/target/arm/machine.c > @@ -184,6 +184,7 @@ static const VMStateDescription vmstate_m_faultmask_primask = { > .name = "cpu/m/faultmask-primask", > .version_id = 1, > .minimum_version_id = 1, > + .needed = m_needed, > .fields = (VMStateField[]) { > VMSTATE_UINT32(env.v7m.faultmask[M_REG_NS], ARMCPU), > VMSTATE_UINT32(env.v7m.primask[M_REG_NS], ARMCPU), > @@ -230,6 +231,7 @@ static const VMStateDescription vmstate_m_scr = { > .name = "cpu/m/scr", > .version_id = 1, > .minimum_version_id = 1, > + .needed = m_needed, > .fields = (VMStateField[]) { > VMSTATE_UINT32(env.v7m.scr[M_REG_NS], ARMCPU), > VMSTATE_END_OF_LIST() > @@ -240,6 +242,7 @@ static const VMStateDescription vmstate_m_other_sp = { > .name = "cpu/m/other-sp", > .version_id = 1, > .minimum_version_id = 1, > + .needed = m_needed, > .fields = (VMStateField[]) { > VMSTATE_UINT32(env.v7m.other_sp, ARMCPU), > VMSTATE_END_OF_LIST() > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-06 10:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-03 16:36 [Qemu-devel] [PATCH 0/3] Arm migration fixes for 3.0 Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function Peter Maydell 2018-08-03 17:29 ` Dr. David Alan Gilbert 2018-08-06 10:00 ` [Qemu-devel] [Qemu-arm] " Peter Maydell 2018-08-03 16:36 ` [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu Peter Maydell 2018-08-03 17:08 ` Dr. David Alan Gilbert 2018-08-03 16:36 ` [Qemu-devel] [PATCH 3/3] target/arm: Add dummy needed functions to M profile vmstate subsections Peter Maydell 2018-08-03 17:31 ` Dr. David Alan Gilbert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.