Re: [Qemu-devel] [RFC PATCH] arm boot: added QOM device definition

["Andreas Färber" <afaerber@suse.de>]

Am 08.02.2012 08:55, schrieb Peter A. G. Crosthwaite:
> From: "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com>
> 
> Create a QOM device for bootstrapping linux on arm. Wraps the existing
> arm_boot code and calls arm_load_kernel() at device init. Allows booting
> of linux without -kernel -initrd -append arguments. The main drawback is
> the boardid now has to be specified as there is no API for querying the
> machine model for that. The code also assumes it is booting on first_cpu.
> Added an automatic detection for the machine ram size so that ram size can
> be detected by the bootloader without needing to get the value from either
> the command line or machine model
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  Makefile.objs    |    1 +
>  hw/arm-misc.h    |   10 ++++----
>  hw/arm_boot.c    |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/versatilepb.c |   14 +++++++-----
>  4 files changed, 73 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index ec35320..c397aa7 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -189,6 +189,7 @@ user-obj-y += $(trace-obj-y)
>  
>  hw-obj-y =
>  hw-obj-y += vl.o loader.o
> +hw-obj-y += image-loader.o

Is this file missing in the patch or why is it being added here?

>  hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
>  hw-obj-y += usb-libhw.o
>  hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 5e5204b..754d8bd 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -25,10 +25,10 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>  
>  /* arm_boot.c */
>  struct arm_boot_info {
> -    int ram_size;
> -    const char *kernel_filename;
> -    const char *kernel_cmdline;
> -    const char *initrd_filename;
> +    uint32_t ram_size;
> +    char *kernel_filename;
> +    char *kernel_cmdline;
> +    char *initrd_filename;
>      target_phys_addr_t loader_start;
>      /* multicore boards that use the default secondary core boot functions
>       * need to put the address of the secondary boot code, the boot reg,
> @@ -39,7 +39,7 @@ struct arm_boot_info {
>      target_phys_addr_t smp_bootreg_addr;
>      target_phys_addr_t smp_priv_base;
>      int nb_cpus;
> -    int board_id;
> +    uint32_t board_id;
>      int (*atag_board)(const struct arm_boot_info *info, void *p);
>      /* multicore boards that use the default secondary core boot functions
>       * can ignore these two function calls. If the default functions won't
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 5f163fd..1f028f4 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -12,6 +12,9 @@
>  #include "sysemu.h"
>  #include "loader.h"
>  #include "elf.h"
> +#include "qdev.h"
> +#include "exec-memory.h"
> +
>  
>  #define KERNEL_ARGS_ADDR 0x100
>  #define KERNEL_LOAD_ADDR 0x00010000
> @@ -245,6 +248,20 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
>      target_phys_addr_t entry;
>      int big_endian;
>  
> +    if (!env) {
> +        env = first_cpu;
> +    }

So, if the env argument is NULL then you use global first_cpu instead -
there's no guarantee that's not NULL either. Considering the way where
I'm heading with CPU QOM'ification this seems like a bad idea. If the
caller passed a bad argument, rather fail, g_assert() or abort().

> +
> +    if (!info->ram_size) {
> +        MemoryRegion *sysmem = get_system_memory();
> +        MemoryRegion *ram = memory_region_find(sysmem, 0, 4).mr;
> +        if (!ram) {
> +            fprintf(stderr, "Ram size not specified and autodetect failed\n");
> +            exit(1);
> +        }
> +        info->ram_size = memory_region_size(ram);
> +    }
> +
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
>          fprintf(stderr, "Kernel image must be specified\n");
> @@ -321,3 +338,45 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
>          qemu_register_reset(do_cpu_reset, env);
>      }
>  }
> +
> +struct ArmBoot {
> +    DeviceState qdev;
> +    struct arm_boot_info info;
> +};
> +
> +static int arm_boot_init(DeviceState *dev)
> +{
> +    struct ArmBoot *s = (struct ArmBoot *)dev;
> +    arm_load_kernel(NULL, &s->info);
> +    return 0;
> +}
> +
> +static Property arm_boot_properties [] = {
> +    DEFINE_PROP_UINT32("boardid", struct ArmBoot, info.board_id, 0),
> +    DEFINE_PROP_STRING("initrd", struct ArmBoot, info.initrd_filename),
> +    DEFINE_PROP_STRING("kernel", struct ArmBoot, info.kernel_filename),
> +    DEFINE_PROP_STRING("cmdline", struct ArmBoot, info.kernel_cmdline),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void arm_boot_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->init = arm_boot_init;
> +    dc->props = arm_boot_properties;
> +}
> +
> +static TypeInfo arm_boot_info_ = {
> +    .name                  = "arm_linux_loader",
> +    .parent                = TYPE_DEVICE,
> +    .class_init            = arm_boot_class_init,
> +    .instance_size         = sizeof(struct ArmBoot),
> +};

Does this really need to be derived from TYPE_DEVICE rather than
TYPE_OBJECT? As you guys correctly argued before, it's not a hardware
device per se.

> +
> +static void arm_boot_register(void)
> +{
> +    type_register_static(&arm_boot_info_);
> +}
> +
> +device_init(arm_boot_register)

NB: A patch on the list introduces a type_init() and changes the
convention from ..._register_devices() to ..._register_types().

Andreas

> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 6e28e78..e42d845 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -313,12 +313,14 @@ static void versatile_init(ram_addr_t ram_size,
>      /*  0x101f3000 UART2.  */
>      /* 0x101f4000 SSPI.  */
>  
> -    versatile_binfo.ram_size = ram_size;
> -    versatile_binfo.kernel_filename = kernel_filename;
> -    versatile_binfo.kernel_cmdline = kernel_cmdline;
> -    versatile_binfo.initrd_filename = initrd_filename;
> -    versatile_binfo.board_id = board_id;
> -    arm_load_kernel(env, &versatile_binfo);
> +	if (kernel_filename) {
> +		versatile_binfo.ram_size = ram_size;
> +		versatile_binfo.kernel_filename = kernel_filename;
> +		versatile_binfo.kernel_cmdline = kernel_cmdline;
> +		versatile_binfo.initrd_filename = initrd_filename;
> +		versatile_binfo.board_id = board_id;
> +		arm_load_kernel(env, &versatile_binfo);
> +	}
>  }
>  
>  static void vpb_init(ram_addr_t ram_size,

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg