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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/08/2012 01:55 AM, Peter A. G. Crosthwaite wrote:
> 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
>   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;
> +    }
> +
> +    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_ = {

Please don't leave a trailing _ on the end.

> +    .name                  = "arm_linux_loader",
> +    .parent                = TYPE_DEVICE,
> +    .class_init            = arm_boot_class_init,
> +    .instance_size         = sizeof(struct ArmBoot),
> +};
> +
> +static void arm_boot_register(void)
> +{
> +    type_register_static(&arm_boot_info_);
> +}
> +
> +device_init(arm_boot_register)

Why have a separate arm_boot_info instead of folding the fields into ArmBoot?

Regards,

Anthony Liguori

> 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,