[for-2.10] hw/display/sm501: Don't use vmstate_register_ram_global()

Message ID 1500309462-12792-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 4c4414a4388f902b7ae2814f9a64898dd0e426a5
Headers show

Commit Message

Peter Maydell July 17, 2017, 4:37 p.m.
The sm501 device uses vmstate_register_ram_global() to register its
memory region for migration.  This means it gets a name that is
assumed to be global to the whole system, which in turn means that if
you create two of the device we assert because of the duplication:

qemu-system-ppc -device sm501 -device sm501
RAMBlock "sm501.local" already registered, abort!
Aborted (core dumped)

Changing this to just use memory_region_init_ram()'s automatic
registration of the memory region with a device-local name fixes
this.  The downside is that it breaks migration compatibility, but
luckily we only added migration support to this device in the 2.10
release cycle so we haven't released a QEMU version with the broken
implementation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
Recent refactoring that renamed the old "memory_region_init_ram"
to memory_region_init_ram_nomigrate" made this bug a bit easier
to find via grep...

 hw/display/sm501.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.7.4

Comments

Peter Maydell July 24, 2017, 10:28 a.m. | #1
Ping for review, please? Would be nice to get this into rc0.

thanks
-- PMM

On 17 July 2017 at 17:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> The sm501 device uses vmstate_register_ram_global() to register its

> memory region for migration.  This means it gets a name that is

> assumed to be global to the whole system, which in turn means that if

> you create two of the device we assert because of the duplication:

>

> qemu-system-ppc -device sm501 -device sm501

> RAMBlock "sm501.local" already registered, abort!

> Aborted (core dumped)

>

> Changing this to just use memory_region_init_ram()'s automatic

> registration of the memory region with a device-local name fixes

> this.  The downside is that it breaks migration compatibility, but

> luckily we only added migration support to this device in the 2.10

> release cycle so we haven't released a QEMU version with the broken

> implementation.

>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

> Recent refactoring that renamed the old "memory_region_init_ram"

> to memory_region_init_ram_nomigrate" made this bug a bit easier

> to find via grep...

>

>  hw/display/sm501.c | 3 +--

>  1 file changed, 1 insertion(+), 2 deletions(-)

>

> diff --git a/hw/display/sm501.c b/hw/display/sm501.c

> index af792c5..9aa515b 100644

> --- a/hw/display/sm501.c

> +++ b/hw/display/sm501.c

> @@ -1578,9 +1578,8 @@ static void sm501_init(SM501State *s, DeviceState *dev,

>                    s->local_mem_size_index);

>

>      /* local memory */

> -    memory_region_init_ram_nomigrate(&s->local_mem_region, OBJECT(dev), "sm501.local",

> +    memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",

>                             get_local_mem_size(s), &error_fatal);

> -    vmstate_register_ram_global(&s->local_mem_region);

>      memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);

>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);

>

> --P
Dr. David Alan Gilbert July 24, 2017, 6:35 p.m. | #2
* Peter Maydell (peter.maydell@linaro.org) wrote:
> Ping for review, please? Would be nice to get this into rc0.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> 

> thanks

> -- PMM

> 

> On 17 July 2017 at 17:37, Peter Maydell <peter.maydell@linaro.org> wrote:

> > The sm501 device uses vmstate_register_ram_global() to register its

> > memory region for migration.  This means it gets a name that is

> > assumed to be global to the whole system, which in turn means that if

> > you create two of the device we assert because of the duplication:

> >

> > qemu-system-ppc -device sm501 -device sm501

> > RAMBlock "sm501.local" already registered, abort!

> > Aborted (core dumped)

> >

> > Changing this to just use memory_region_init_ram()'s automatic

> > registration of the memory region with a device-local name fixes

> > this.  The downside is that it breaks migration compatibility, but

> > luckily we only added migration support to this device in the 2.10

> > release cycle so we haven't released a QEMU version with the broken

> > implementation.

> >

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> > ---

> > Recent refactoring that renamed the old "memory_region_init_ram"

> > to memory_region_init_ram_nomigrate" made this bug a bit easier

> > to find via grep...

> >

> >  hw/display/sm501.c | 3 +--

> >  1 file changed, 1 insertion(+), 2 deletions(-)

> >

> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c

> > index af792c5..9aa515b 100644

> > --- a/hw/display/sm501.c

> > +++ b/hw/display/sm501.c

> > @@ -1578,9 +1578,8 @@ static void sm501_init(SM501State *s, DeviceState *dev,

> >                    s->local_mem_size_index);

> >

> >      /* local memory */

> > -    memory_region_init_ram_nomigrate(&s->local_mem_region, OBJECT(dev), "sm501.local",

> > +    memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",

> >                             get_local_mem_size(s), &error_fatal);

> > -    vmstate_register_ram_global(&s->local_mem_region);

> >      memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);

> >      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);

> >

> > --P

> 

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Maydell July 25, 2017, 12:49 p.m. | #3
On 24 July 2017 at 19:35, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:

>> Ping for review, please? Would be nice to get this into rc0.

>

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Thanks; applied to master.

-- PMM

Patch

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index af792c5..9aa515b 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1578,9 +1578,8 @@  static void sm501_init(SM501State *s, DeviceState *dev,
                   s->local_mem_size_index);
 
     /* local memory */
-    memory_region_init_ram_nomigrate(&s->local_mem_region, OBJECT(dev), "sm501.local",
+    memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",
                            get_local_mem_size(s), &error_fatal);
-    vmstate_register_ram_global(&s->local_mem_region);
     memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
     s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);