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 <address@hidden> 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 <address@hidden>
> ---
>  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
>