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

[Peter Crosthwaite <peter.crosthwaite@petalogix.com>]

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

On Thu, Feb 9, 2012 at 11:22 PM, Andreas Färber <afaerber@suse.de> wrote:

> 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?
>
>
Yeh mistakenly grabbed in the commit. Will remove V2


> >  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().
>
>
This will go away, and be the responsibility of the machine model under
Alex and Pauls suggested usage models. This hunk will be deleted and the
verification of boot parameters (including this CPU one) will become the
responsibility of the machine model.


> > +
> > +    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.
>
>
Yeh sounds accurate, Maybe its a object class of its own? TYPE_BOOTLOADER,
or TYPE_SOFTWARE or something, Maybe theres a few possible code share
niceties with other platforms if you were to define more layers of
heirachy. Could potentially factor out linux specific stuff to an
intermediate alyer and only the arm specific stuff is in the arm_boot
object.


> > +
> > +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().
>
>
Sure. Ill rebase on next revision.


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

[-- Attachment #2: Type: text/html, Size: 10310 bytes --]