diff mbox series

[v2,28/86] arm:raspi: use memdev for RAM

Message ID 1579100861-73692-29-git-send-email-imammedo@redhat.com
State New
Headers show
Series refactor main RAM allocation to use hostmem backend | expand

Commit Message

Igor Mammedov Jan. 15, 2020, 3:06 p.m. UTC 
memory_region_allocate_system_memory() API is going away, so
replace it with memdev allocated MemoryRegion. The later is
initialized by generic code, so board only needs to opt in
to memdev scheme by providing
  MachineClass::default_ram_id
and using MachineState::ram instead of manually initializing
RAM memory region.

PS:
 remove no longer needed RasPiState

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Andrew.Baumann@microsoft.com
CC: philmd@redhat.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/raspi.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 15, 2020, 7:07 p.m. UTC | #1 
On 1/15/20 4:06 PM, Igor Mammedov wrote:
> memory_region_allocate_system_memory() API is going away, so
> replace it with memdev allocated MemoryRegion. The later is
> initialized by generic code, so board only needs to opt in
> to memdev scheme by providing
>    MachineClass::default_ram_id
> and using MachineState::ram instead of manually initializing
> RAM memory region.
> 
> PS:
>   remove no longer needed RasPiState
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Andrew.Baumann@microsoft.com
> CC: philmd@redhat.com
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/raspi.c | 34 +++++++++++++---------------------
>   1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 6a510aa..33ace66 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -32,11 +32,6 @@
>   /* Table of Linux board IDs for different Pi versions */
>   static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
>   
> -typedef struct RasPiState {
> -    BCM283XState soc;
> -    MemoryRegion ram;
> -} RasPiState;
> -
>   static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
>   {
>       static const uint32_t smpboot[] = {
> @@ -166,7 +161,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>   
>   static void raspi_init(MachineState *machine, int version)
>   {
> -    RasPiState *s = g_new0(RasPiState, 1);
> +    Object *soc;
>       uint32_t vcram_size;
>       DriveInfo *di;
>       BlockBackend *blk;
> @@ -179,30 +174,26 @@ static void raspi_init(MachineState *machine, int version)
>           exit(1);
>       }
>   
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
> -                            &error_abort, NULL);
> +    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
>   
> -    /* Allocate and map RAM */
> -    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> -                                         machine->ram_size);
>       /* FIXME: Remove when we have custom CPU address space support */
> -    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
> +    memory_region_add_subregion_overlap(get_system_memory(), 0,
> +                                        machine->ram, 0);
>   
>       /* Setup the SOC */
> -    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
> +    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
>                                      &error_abort);
> -    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
> +    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
>                               &error_abort);
>       int board_rev = version == 3 ? 0xa02082 : 0xa21041;
> -    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
> -                            &error_abort);
> -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
> +    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
> +    object_property_set_bool(soc, true, "realized", &error_abort);
>   
>       /* Create and plug in the SD cards */
>       di = drive_get_next(IF_SD);
>       blk = di ? blk_by_legacy_dinfo(di) : NULL;
> -    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> +    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
>       if (bus == NULL) {
>           error_report("No SD bus found in SOC object");
>           exit(1);
> @@ -211,8 +202,7 @@ static void raspi_init(MachineState *machine, int version)
>       qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>       object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
>   
> -    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> -                                          &error_abort);
> +    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
>       setup_boot(machine, version, machine->ram_size - vcram_size);
>   }
>   
> @@ -233,6 +223,7 @@ static void raspi2_machine_init(MachineClass *mc)
>       mc->min_cpus = BCM283X_NCPUS;
>       mc->default_cpus = BCM283X_NCPUS;
>       mc->default_ram_size = 1 * GiB;
> +    mc->default_ram_id = "ram";
>       mc->ignore_memory_transaction_failures = true;
>   };
>   DEFINE_MACHINE("raspi2", raspi2_machine_init)
> @@ -255,6 +246,7 @@ static void raspi3_machine_init(MachineClass *mc)
>       mc->min_cpus = BCM283X_NCPUS;
>       mc->default_cpus = BCM283X_NCPUS;
>       mc->default_ram_size = 1 * GiB;
> +    mc->default_ram_id = "ram";
>   }
>   DEFINE_MACHINE("raspi3", raspi3_machine_init)
>   #endif
> 

This patch diverges a lot from my current work:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg653818.html
So I'm not very happy about it. Maybe my bad I should ping more 
aggressively my patches. I can respin mine preparing for your series on top.

Meanwhile if you are in a hurry I tested yours, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Igor Mammedov Jan. 16, 2020, 4:55 p.m. UTC | #2 
On Wed, 15 Jan 2020 20:07:34 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/15/20 4:06 PM, Igor Mammedov wrote:
> > memory_region_allocate_system_memory() API is going away, so
> > replace it with memdev allocated MemoryRegion. The later is
> > initialized by generic code, so board only needs to opt in
> > to memdev scheme by providing
> >    MachineClass::default_ram_id
> > and using MachineState::ram instead of manually initializing
> > RAM memory region.
> > 
> > PS:
> >   remove no longer needed RasPiState
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: Andrew.Baumann@microsoft.com
> > CC: philmd@redhat.com
> > CC: peter.maydell@linaro.org
> > CC: qemu-arm@nongnu.org
> > ---
> >   hw/arm/raspi.c | 34 +++++++++++++---------------------
> >   1 file changed, 13 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > index 6a510aa..33ace66 100644
> > --- a/hw/arm/raspi.c
> > +++ b/hw/arm/raspi.c
> > @@ -32,11 +32,6 @@
> >   /* Table of Linux board IDs for different Pi versions */
> >   static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
> >   
> > -typedef struct RasPiState {
> > -    BCM283XState soc;
> > -    MemoryRegion ram;
> > -} RasPiState;
> > -
> >   static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
> >   {
> >       static const uint32_t smpboot[] = {
> > @@ -166,7 +161,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
> >   
> >   static void raspi_init(MachineState *machine, int version)
> >   {
> > -    RasPiState *s = g_new0(RasPiState, 1);
> > +    Object *soc;
> >       uint32_t vcram_size;
> >       DriveInfo *di;
> >       BlockBackend *blk;
> > @@ -179,30 +174,26 @@ static void raspi_init(MachineState *machine, int version)
> >           exit(1);
> >       }
> >   
> > -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > -                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
> > -                            &error_abort, NULL);
> > +    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> > +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
> >   
> > -    /* Allocate and map RAM */
> > -    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> > -                                         machine->ram_size);
> >       /* FIXME: Remove when we have custom CPU address space support */
> > -    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
> > +    memory_region_add_subregion_overlap(get_system_memory(), 0,
> > +                                        machine->ram, 0);
> >   
> >       /* Setup the SOC */
> > -    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
> > +    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
> >                                      &error_abort);
> > -    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
> > +    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
> >                               &error_abort);
> >       int board_rev = version == 3 ? 0xa02082 : 0xa21041;
> > -    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
> > -                            &error_abort);
> > -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
> > +    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
> > +    object_property_set_bool(soc, true, "realized", &error_abort);
> >   
> >       /* Create and plug in the SD cards */
> >       di = drive_get_next(IF_SD);
> >       blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > -    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> > +    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
> >       if (bus == NULL) {
> >           error_report("No SD bus found in SOC object");
> >           exit(1);
> > @@ -211,8 +202,7 @@ static void raspi_init(MachineState *machine, int version)
> >       qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> >       object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> >   
> > -    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> > -                                          &error_abort);
> > +    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
> >       setup_boot(machine, version, machine->ram_size - vcram_size);
> >   }
> >   
> > @@ -233,6 +223,7 @@ static void raspi2_machine_init(MachineClass *mc)
> >       mc->min_cpus = BCM283X_NCPUS;
> >       mc->default_cpus = BCM283X_NCPUS;
> >       mc->default_ram_size = 1 * GiB;
> > +    mc->default_ram_id = "ram";
> >       mc->ignore_memory_transaction_failures = true;
> >   };
> >   DEFINE_MACHINE("raspi2", raspi2_machine_init)
> > @@ -255,6 +246,7 @@ static void raspi3_machine_init(MachineClass *mc)
> >       mc->min_cpus = BCM283X_NCPUS;
> >       mc->default_cpus = BCM283X_NCPUS;
> >       mc->default_ram_size = 1 * GiB;
> > +    mc->default_ram_id = "ram";
> >   }
> >   DEFINE_MACHINE("raspi3", raspi3_machine_init)
> >   #endif
> >   
> 
> This patch diverges a lot from my current work:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg653818.html
perhaps we could generalize size checks on top of that.
there were some ideas in that direction in
  "[PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup"
thread


> So I'm not very happy about it. Maybe my bad I should ping more 
> aggressively my patches. I can respin mine preparing for your series on top.

this patch is trivial,
if your patches merged before this, than I'll just rebase.


> 
> Meanwhile if you are in a hurry I tested yours, so:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>
diff mbox series

Patch

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 6a510aa..33ace66 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -32,11 +32,6 @@ 
 /* Table of Linux board IDs for different Pi versions */
 static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
 
-typedef struct RasPiState {
-    BCM283XState soc;
-    MemoryRegion ram;
-} RasPiState;
-
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
@@ -166,7 +161,7 @@  static void setup_boot(MachineState *machine, int version, size_t ram_size)
 
 static void raspi_init(MachineState *machine, int version)
 {
-    RasPiState *s = g_new0(RasPiState, 1);
+    Object *soc;
     uint32_t vcram_size;
     DriveInfo *di;
     BlockBackend *blk;
@@ -179,30 +174,26 @@  static void raspi_init(MachineState *machine, int version)
         exit(1);
     }
 
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
-                            &error_abort, NULL);
+    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
+    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
 
-    /* Allocate and map RAM */
-    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
-                                         machine->ram_size);
     /* FIXME: Remove when we have custom CPU address space support */
-    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
+    memory_region_add_subregion_overlap(get_system_memory(), 0,
+                                        machine->ram, 0);
 
     /* Setup the SOC */
-    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
+    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
                                    &error_abort);
-    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
+    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
                             &error_abort);
     int board_rev = version == 3 ? 0xa02082 : 0xa21041;
-    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
-                            &error_abort);
-    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
+    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
+    object_property_set_bool(soc, true, "realized", &error_abort);
 
     /* Create and plug in the SD cards */
     di = drive_get_next(IF_SD);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
-    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
+    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
     if (bus == NULL) {
         error_report("No SD bus found in SOC object");
         exit(1);
@@ -211,8 +202,7 @@  static void raspi_init(MachineState *machine, int version)
     qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
     object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
 
-    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
-                                          &error_abort);
+    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
     setup_boot(machine, version, machine->ram_size - vcram_size);
 }
 
@@ -233,6 +223,7 @@  static void raspi2_machine_init(MachineClass *mc)
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = 1 * GiB;
+    mc->default_ram_id = "ram";
     mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
@@ -255,6 +246,7 @@  static void raspi3_machine_init(MachineClass *mc)
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = 1 * GiB;
+    mc->default_ram_id = "ram";
 }
 DEFINE_MACHINE("raspi3", raspi3_machine_init)
 #endif