[Qemu-devel] [PATCH 0/2] armv7m: Remove armv7m_init() function

[Qemu-devel] [PATCH 0/2] armv7m: Remove armv7m_init() function

[Peter Maydell]

The armv7m_init() function is a legacy from before we properly QOMified
ARMv7M, and it has some flaws:
    
 * it combines work that needs to be done by an SoC object (creating
   and initializing the TYPE_ARMV7M object) with work that needs to
   be done by the board model (setting the system up to load the ELF
   file specified with -kernel)
 * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
   arrange to propagate the failure outward
 * it uses allocate-and-create via qdev_create() whereas the current
   preferred style for SoC objects is to do creation in-place

This patchset fixes the only current caller (the stellaris board)
to not use it, and then removes the function.
    
New board and SoC models should do the two jobs this function
was doing themselves, in the right places and with whatever their
preferred style/error handling is.

(I've cc'd the people working on the nRF51 SoC model, as a heads-up
that they'll need to update their code so it compiles once this
hits master.)

thanks
-- PMM

Peter Maydell (2):
  stellaris: Stop using armv7m_init()
  hw/arm/armv7m: Remove unused armv7m_init() function

 include/hw/arm/arm.h |  8 ++------
 hw/arm/armv7m.c      | 21 ---------------------
 hw/arm/stellaris.c   | 12 ++++++++++--
 3 files changed, 12 insertions(+), 29 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] stellaris: Stop using armv7m_init()

[Peter Maydell]

The stellaris board is still using the legacy armv7m_init() function,
which predates conversion of the ARMv7M into a proper QOM container
object. Make the board code directly create the ARMv7M object instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/stellaris.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index e886f54976..1b2c383e9f 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -20,6 +20,7 @@
 #include "qemu/log.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "hw/arm/armv7m.h"
 #include "hw/char/pl011.h"
 #include "hw/misc/unimp.h"
 #include "cpu.h"
@@ -1297,8 +1298,13 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
                            &error_fatal);
     memory_region_add_subregion(system_memory, 0x20000000, sram);
 
-    nvic = armv7m_init(system_memory, flash_size, NUM_IRQ_LINES,
-                       ms->kernel_filename, ms->cpu_type);
+    nvic = qdev_create(NULL, TYPE_ARMV7M);
+    qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
+    qdev_prop_set_string(nvic, "cpu-type", ms->cpu_type);
+    object_property_set_link(OBJECT(nvic), OBJECT(get_system_memory()),
+                                     "memory", &error_abort);
+    /* This will exit with an error if the user passed us a bad cpu_type */
+    qdev_init_nofail(nvic);
 
     qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0,
                                 qemu_allocate_irq(&do_sys_reset, NULL, 0));
@@ -1430,6 +1436,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     create_unimplemented_device("analogue-comparator", 0x4003c000, 0x1000);
     create_unimplemented_device("hibernation", 0x400fc000, 0x1000);
     create_unimplemented_device("flash-control", 0x400fd000, 0x1000);
+
+    armv7m_load_kernel(ARM_CPU(first_cpu), ms->kernel_filename, flash_size);
 }
 
 /* FIXME: Figure out how to generate these from stellaris_boards.  */
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] stellaris: Stop using armv7m_init()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Fri, Jun 01, 2018 at 03:43:27PM +0100, Peter Maydell wrote:
> The stellaris board is still using the legacy armv7m_init() function,
> which predates conversion of the ARMv7M into a proper QOM container
> object. Make the board code directly create the ARMv7M object instead.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/stellaris.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/2] hw/arm/armv7m: Remove unused armv7m_init() function

[Peter Maydell]

Remove the now-unused armv7m_init() function. This was a legacy from
before we properly QOMified ARMv7M, and it has some flaws:

 * it combines work that needs to be done by an SoC object (creating
   and initializing the TYPE_ARMV7M object) with work that needs to
   be done by the board model (setting the system up to load the ELF
   file specified with -kernel)
 * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
   arrange to propagate the failure outward
 * it uses allocate-and-create via qdev_create() whereas the current
   preferred style for SoC objects is to do creation in-place

Board and SoC models can instead do the two jobs this function
was doing themselves, in the right places and with whatever their
preferred style/error handling is.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/arm.h |  8 ++------
 hw/arm/armv7m.c      | 21 ---------------------
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 70fa2287e2..ffed39252d 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -23,9 +23,6 @@ typedef enum {
     ARM_ENDIANNESS_BE32,
 } arm_endianness;
 
-/* armv7m.c */
-DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
-                         const char *kernel_filename, const char *cpu_type);
 /**
  * armv7m_load_kernel:
  * @cpu: CPU
@@ -33,9 +30,8 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
  * @mem_size: mem_size: maximum image size to load
  *
  * Load the guest image for an ARMv7M system. This must be called by
- * any ARMv7M board, either directly or via armv7m_init(). (This is
- * necessary to ensure that the CPU resets correctly on system reset,
- * as well as for kernel loading.)
+ * any ARMv7M board. (This is necessary to ensure that the CPU resets
+ * correctly on system reset, as well as for kernel loading.)
  */
 void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size);
 
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index f123cc7d3d..a4ab7d2069 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -261,27 +261,6 @@ static void armv7m_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
-/* Init CPU and memory for a v7-M based board.
-   mem_size is in bytes.
-   Returns the ARMv7M device.  */
-
-DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
-                         const char *kernel_filename, const char *cpu_type)
-{
-    DeviceState *armv7m;
-
-    armv7m = qdev_create(NULL, TYPE_ARMV7M);
-    qdev_prop_set_uint32(armv7m, "num-irq", num_irq);
-    qdev_prop_set_string(armv7m, "cpu-type", cpu_type);
-    object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()),
-                                     "memory", &error_abort);
-    /* This will exit with an error if the user passed us a bad cpu_type */
-    qdev_init_nofail(armv7m);
-
-    armv7m_load_kernel(ARM_CPU(first_cpu), kernel_filename, mem_size);
-    return armv7m;
-}
-
 void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
 {
     int image_size;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/2] hw/arm/armv7m: Remove unused armv7m_init() function

[Joel Stanley]

Hi Peter,

On 2 June 2018 at 00:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> Remove the now-unused armv7m_init() function. This was a legacy from
> before we properly QOMified ARMv7M, and it has some flaws:
>
>  * it combines work that needs to be done by an SoC object (creating
>    and initializing the TYPE_ARMV7M object) with work that needs to
>    be done by the board model (setting the system up to load the ELF
>    file specified with -kernel)
>  * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
>    arrange to propagate the failure outward
>  * it uses allocate-and-create via qdev_create() whereas the current
>    preferred style for SoC objects is to do creation in-place
>
> Board and SoC models can instead do the two jobs this function
> was doing themselves, in the right places and with whatever their
> preferred style/error handling is.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/arm/arm.h |  8 ++------
>  hw/arm/armv7m.c      | 21 ---------------------
>  2 files changed, 2 insertions(+), 27 deletions(-)
>
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 70fa2287e2..ffed39252d 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -23,9 +23,6 @@ typedef enum {
>      ARM_ENDIANNESS_BE32,
>  } arm_endianness;
>
> -/* armv7m.c */
> -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> -                         const char *kernel_filename, const char *cpu_type);
>  /**
>   * armv7m_load_kernel:
>   * @cpu: CPU
> @@ -33,9 +30,8 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>   * @mem_size: mem_size: maximum image size to load
>   *
>   * Load the guest image for an ARMv7M system. This must be called by
> - * any ARMv7M board, either directly or via armv7m_init(). (This is
> - * necessary to ensure that the CPU resets correctly on system reset,
> - * as well as for kernel loading.)
> + * any ARMv7M board. (This is necessary to ensure that the CPU resets
> + * correctly on system reset, as well as for kernel loading.)
>   */
>  void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size);
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index f123cc7d3d..a4ab7d2069 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -261,27 +261,6 @@ static void armv7m_reset(void *opaque)
>      cpu_reset(CPU(cpu));
>  }
>
> -/* Init CPU and memory for a v7-M based board.
> -   mem_size is in bytes.
> -   Returns the ARMv7M device.  */
> -
> -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> -                         const char *kernel_filename, const char *cpu_type)
> -{
> -    DeviceState *armv7m;
> -
> -    armv7m = qdev_create(NULL, TYPE_ARMV7M);
> -    qdev_prop_set_uint32(armv7m, "num-irq", num_irq);
> -    qdev_prop_set_string(armv7m, "cpu-type", cpu_type);
> -    object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()),
> -                                     "memory", &error_abort);

It looks like the snippet above is going to be a cut/paste for all v7m machines.

Would it achieve your goal by instead removing the armv7m_load_kernel
call from this function?

Cheers,

Joel

> -    /* This will exit with an error if the user passed us a bad cpu_type */
> -    qdev_init_nofail(armv7m);
> -
> -    armv7m_load_kernel(ARM_CPU(first_cpu), kernel_filename, mem_size);
> -    return armv7m;
> -}
> -
>  void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
>  {
>      int image_size;
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH 2/2] hw/arm/armv7m: Remove unused armv7m_init() function

[Peter Maydell]

On 3 June 2018 at 01:48, Joel Stanley <joel@jms.id.au> wrote:
> Hi Peter,
>
> On 2 June 2018 at 00:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Remove the now-unused armv7m_init() function. This was a legacy from
>> before we properly QOMified ARMv7M, and it has some flaws:
>>
>>  * it combines work that needs to be done by an SoC object (creating
>>    and initializing the TYPE_ARMV7M object) with work that needs to
>>    be done by the board model (setting the system up to load the ELF
>>    file specified with -kernel)
>>  * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
>>    arrange to propagate the failure outward
>>  * it uses allocate-and-create via qdev_create() whereas the current
>>    preferred style for SoC objects is to do creation in-place
>>
>> Board and SoC models can instead do the two jobs this function
>> was doing themselves, in the right places and with whatever their
>> preferred style/error handling is.

>> -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>> -                         const char *kernel_filename, const char *cpu_type)
>> -{
>> -    DeviceState *armv7m;
>> -
>> -    armv7m = qdev_create(NULL, TYPE_ARMV7M);
>> -    qdev_prop_set_uint32(armv7m, "num-irq", num_irq);
>> -    qdev_prop_set_string(armv7m, "cpu-type", cpu_type);
>> -    object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()),
>> -                                     "memory", &error_abort);
>
> It looks like the snippet above is going to be a cut/paste for all v7m machines.

If you look at pretty much all the v7m machines except stellaris,
they don't use this code fragment. (Instead you have an
object_initialize()/qdev_set_parent_bus() in the SoC container's
init function, calls to set properties and realize in the container's
realize, and a call to armv7m_load_kernel() in the board code.)

I agree that what you might call the "modern style" of writing
SoC containers like that has a lot of boilerplate, but that's
not specific to the armv7m object, and if we want to reduce
boilerplate we should look at how we can do it consistently
across devices.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/armv7m: Remove unused armv7m_init() function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Fri, Jun 01, 2018 at 03:43:28PM +0100, Peter Maydell wrote:
> Remove the now-unused armv7m_init() function. This was a legacy from
> before we properly QOMified ARMv7M, and it has some flaws:
> 
>  * it combines work that needs to be done by an SoC object (creating
>    and initializing the TYPE_ARMV7M object) with work that needs to
>    be done by the board model (setting the system up to load the ELF
>    file specified with -kernel)
>  * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
>    arrange to propagate the failure outward
>  * it uses allocate-and-create via qdev_create() whereas the current
>    preferred style for SoC objects is to do creation in-place
> 
> Board and SoC models can instead do the two jobs this function
> was doing themselves, in the right places and with whatever their
> preferred style/error handling is.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/arm/arm.h |  8 ++------
>  hw/arm/armv7m.c      | 21 ---------------------
>  2 files changed, 2 insertions(+), 27 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/2] armv7m: Remove armv7m_init() function

[Peter Maydell]

Ping for code review?

thanks
-- PMM

On 1 June 2018 at 15:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> The armv7m_init() function is a legacy from before we properly QOMified
> ARMv7M, and it has some flaws:
>
>  * it combines work that needs to be done by an SoC object (creating
>    and initializing the TYPE_ARMV7M object) with work that needs to
>    be done by the board model (setting the system up to load the ELF
>    file specified with -kernel)
>  * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
>    arrange to propagate the failure outward
>  * it uses allocate-and-create via qdev_create() whereas the current
>    preferred style for SoC objects is to do creation in-place
>
> This patchset fixes the only current caller (the stellaris board)
> to not use it, and then removes the function.
>
> New board and SoC models should do the two jobs this function
> was doing themselves, in the right places and with whatever their
> preferred style/error handling is.
>
> (I've cc'd the people working on the nRF51 SoC model, as a heads-up
> that they'll need to update their code so it compiles once this
> hits master.)
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   stellaris: Stop using armv7m_init()
>   hw/arm/armv7m: Remove unused armv7m_init() function
>
>  include/hw/arm/arm.h |  8 ++------
>  hw/arm/armv7m.c      | 21 ---------------------
>  hw/arm/stellaris.c   | 12 ++++++++++--
>  3 files changed, 12 insertions(+), 29 deletions(-)

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/2] armv7m: Remove armv7m_init() function

[Philippe Mathieu-Daudé]

On 06/11/2018 11:04 AM, Peter Maydell wrote:
> Ping for code review?
> 
> thanks
> -- PMM
> 
> On 1 June 2018 at 15:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The armv7m_init() function is a legacy from before we properly QOMified
>> ARMv7M, and it has some flaws:
>>
>>  * it combines work that needs to be done by an SoC object (creating
>>    and initializing the TYPE_ARMV7M object) with work that needs to
>>    be done by the board model (setting the system up to load the ELF
>>    file specified with -kernel)
>>  * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to
>>    arrange to propagate the failure outward
>>  * it uses allocate-and-create via qdev_create() whereas the current
>>    preferred style for SoC objects is to do creation in-place
>>
>> This patchset fixes the only current caller (the stellaris board)
>> to not use it, and then removes the function.

Good cleanup.

>>
>> New board and SoC models should do the two jobs this function
>> was doing themselves, in the right places and with whatever their
>> preferred style/error handling is.
>>
>> (I've cc'd the people working on the nRF51 SoC model, as a heads-up
>> that they'll need to update their code so it compiles once this
>> hits master.)
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (2):
>>   stellaris: Stop using armv7m_init()
>>   hw/arm/armv7m: Remove unused armv7m_init() function
>>
>>  include/hw/arm/arm.h |  8 ++------
>>  hw/arm/armv7m.c      | 21 ---------------------
>>  hw/arm/stellaris.c   | 12 ++++++++++--
>>  3 files changed, 12 insertions(+), 29 deletions(-)
> 

Series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>