From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvTxL-0002pC-1S for qemu-devel@nongnu.org; Thu, 09 Feb 2012 08:22:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RvTxE-00044T-Kx for qemu-devel@nongnu.org; Thu, 09 Feb 2012 08:22:34 -0500 Received: from cantor2.suse.de ([195.135.220.15]:52224 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RvTxE-00043q-BE for qemu-devel@nongnu.org; Thu, 09 Feb 2012 08:22:28 -0500 Message-ID: <4F33C892.2010700@suse.de> Date: Thu, 09 Feb 2012 14:22:26 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1328687721-16030-1-git-send-email-peter.crosthwaite@petalogix.com> In-Reply-To: <1328687721-16030-1-git-send-email-peter.crosthwaite@petalogix.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH] arm boot: added QOM device definition List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Peter A. G. Crosthwaite" Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, paul@codesourcery.com, agraf@suse.de, qemu-devel@nongnu.org Am 08.02.2012 08:55, schrieb Peter A. G. Crosthwaite: > From: "Peter A. G. Crosthwaite" >=20 > Create a QOM device for bootstrapping linux on arm. Wraps the existing > arm_boot code and calls arm_load_kernel() at device init. Allows bootin= g > of linux without -kernel -initrd -append arguments. The main drawback i= s > 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_cp= u. > 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 eit= her > the command line or machine model >=20 > Signed-off-by: Peter A. G. Crosthwaite > --- > Makefile.objs | 1 + > hw/arm-misc.h | 10 ++++---- > hw/arm_boot.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > hw/versatilepb.c | 14 +++++++----- > 4 files changed, 73 insertions(+), 11 deletions(-) >=20 > 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 +=3D $(trace-obj-y) > =20 > hw-obj-y =3D > hw-obj-y +=3D vl.o loader.o > +hw-obj-y +=3D image-loader.o Is this file missing in the patch or why is it being added here? > hw-obj-$(CONFIG_VIRTIO) +=3D virtio-console.o > hw-obj-y +=3D usb-libhw.o > hw-obj-$(CONFIG_VIRTIO_PCI) +=3D 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_m= em, > =20 > /* 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 funct= ions > * need to put the address of the secondary boot code, the boot re= g, > @@ -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 funct= ions > * can ignore these two function calls. If the default functions w= on'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" > + > =20 > #define KERNEL_ARGS_ADDR 0x100 > #define KERNEL_LOAD_ADDR 0x00010000 > @@ -245,6 +248,20 @@ void arm_load_kernel(CPUState *env, struct arm_boo= t_info *info) > target_phys_addr_t entry; > int big_endian; > =20 > + if (!env) { > + env =3D 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 =3D get_system_memory(); > + MemoryRegion *ram =3D memory_region_find(sysmem, 0, 4).mr; > + if (!ram) { > + fprintf(stderr, "Ram size not specified and autodetect fai= led\n"); > + exit(1); > + } > + info->ram_size =3D 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_boo= t_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 =3D (struct ArmBoot *)dev; > + arm_load_kernel(NULL, &s->info); > + return 0; > +} > + > +static Property arm_boot_properties [] =3D { > + 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 =3D DEVICE_CLASS(klass); > + > + dc->init =3D arm_boot_init; > + dc->props =3D arm_boot_properties; > +} > + > +static TypeInfo arm_boot_info_ =3D { > + .name =3D "arm_linux_loader", > + .parent =3D TYPE_DEVICE, > + .class_init =3D arm_boot_class_init, > + .instance_size =3D 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. */ > =20 > - versatile_binfo.ram_size =3D ram_size; > - versatile_binfo.kernel_filename =3D kernel_filename; > - versatile_binfo.kernel_cmdline =3D kernel_cmdline; > - versatile_binfo.initrd_filename =3D initrd_filename; > - versatile_binfo.board_id =3D board_id; > - arm_load_kernel(env, &versatile_binfo); > + if (kernel_filename) { > + versatile_binfo.ram_size =3D ram_size; > + versatile_binfo.kernel_filename =3D kernel_filename; > + versatile_binfo.kernel_cmdline =3D kernel_cmdline; > + versatile_binfo.initrd_filename =3D initrd_filename; > + versatile_binfo.board_id =3D board_id; > + arm_load_kernel(env, &versatile_binfo); > + } > } > =20 > static void vpb_init(ram_addr_t ram_size, --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg