| Message ID | 1328687721-16030-1-git-send-email-peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
> 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); > + } > } This should be using the new object you just added. You also need to fix all the other uses of arm_load_kernel. Paul
2012/2/8 Paul Brook <paul@codesourcery.com> > > 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); > > + } > > } > > This should be using the new object you just added. > Yes I agree. There is another question tho that if this approach is to be considered, should this call to arm_load_kernel be removed from the machine model altogether? Only reason to keep it would be backwards compatibility for the original command line format. You also need to fix all the other uses of arm_load_kernel. > > Ok. I wanted to put this idea forward for a single machine model (versatilepb in this case) as a proof of concept, before applying a significant change pattern to all the arm machines. If this approach is to be considered then I can recreate this series with the change pattern applied to all arm platforms. Paul > Peter
> > > + arm_load_kernel(env, &versatile_binfo); > > > + } > > > > > > } > > > > This should be using the new object you just added. > > Yes I agree. There is another question tho that if this approach is to be > considered, should this call to arm_load_kernel be removed from the machine > model altogether? Only reason to keep it would be backwards compatibility > for the original command line format. I think we want the -kernel commandline option, butI'mnot attached to a particular implementation. Once we have an arm_linux_loader device then arm_load_kernel should go away, or at least be a private implementation detail of arm_linux_loader. Who creates the arm_linux_loader object (common code or board init function), and how we arrange for it to have the right properties (filename from -kernel, board ID from specific machine) is something I haven't entirely figured out. I suspect we want to replace the arm_load_kernel call with an arm_linux_loader device with appropriate properties. We should have some mechanism for the user to override/augment those properties (e.g. overriding the FDT file). I don't know if that functionality actually exists, or if what we have is particularly well thought out. Ideally the -kernel commandline would just be shorthand for setting/overriding the filename property on that device. The machine->init arguments are removed. That's probably going to need wider coordination with other arches. Paul
2012/2/8 Paul Brook <paul@codesourcery.com> > > > > + arm_load_kernel(env, &versatile_binfo); > > > > + } > > > > > > > > } > > > > > > This should be using the new object you just added. > > > > Yes I agree. There is another question tho that if this approach is to be > > considered, should this call to arm_load_kernel be removed from the > machine > > model altogether? Only reason to keep it would be backwards compatibility > > for the original command line format. > > I think we want the -kernel commandline option, butI'mnot attached to a > particular implementation. > Once we have an arm_linux_loader device then arm_load_kernel should go > away, > or at least be a private implementation detail of arm_linux_loader. > > Who creates the arm_linux_loader object (common code or board init > function), > and how we arrange for it to have the right properties (filename from > -kernel, > board ID from specific machine) is something I haven't entirely figured > out. > > Peter Maydell just emailed a series today that exported kernel_filename & friends as machine opts. Maybe if fundamental machine properties (such as num cpus, ram size and board ID) come from machine opts and the bootload specifics such as filepaths and command lines come from the bootloader device props? The existing -kernel arguments could come via the machine opts path and form defaults for the bootloader device if no -kernel arg is specified to the bootloader device? I suspect we want to replace the arm_load_kernel call with an > arm_linux_loader > device with appropriate properties. > > Ok, so does this mean the machine model would still explicitly instantiate the bootloader device? Will there be a way of removing the bootloader from the machine model so if I need to create my own custom bootloader flow i can? We are doing some custom non-linux boots in our application where arm-boot.c is not working for us so being able to swap out arm_boot.c for another implemenation is one of our goals. The bootloader-as-a-device flow is ideal for this but only works if a user can choose their bootloader on the command line. > We should have some mechanism for the user to override/augment those > properties (e.g. overriding the FDT file). I don't know if that > functionality > actually exists, or if what we have is particularly well thought out. > Ideally > the -kernel commandline would just be shorthand for setting/overriding the > filename property on that device. The machine->init arguments are > removed. That's probably going to need wider coordination with other arches. > > Paul >
> > I suspect we want to replace the arm_load_kernel call with an > > arm_linux_loader device with appropriate properties. > > Ok, so does this mean the machine model would still explicitly instantiate > the bootloader device? Yes. Bootloaders inherently have machine specific knowledge. They need to know ram location, board ID, secondary CPU boot protocols, etc. Requiring the user specify all these things separately from the rest of the machine description is IMO not acceptable. > Will there be a way of removing the bootloader from > the machine model so if I need to create my own custom bootloader flow i > can? We are doing some custom non-linux boots in our application where > arm-boot.c is not working for us so being able to swap out arm_boot.c for > another implemenation is one of our goals. The bootloader-as-a-device flow > is ideal for this but only works if a user can choose their bootloader on > the command line. I think that's solved the same lines as any other custom machine variant. It's also part of the reason I think it's important to cleanly separate devices that provide functionality from those that are just convenience objects for creating a particular assembly of devices. Having generic ELF and maybe binary blob image loaders is certainly useful. They can all coexist happily within the same machine. I'm inclined to say that anything else isn't worth the effort. Users with other special needs can use third party tools[1] to embed/wrap their magic in a regular image. Paul [1] In many cases cases you just need to splat the supplied bootloader+kernel images into ram, not postprocessing is necessary. For most of the rest objcopy is sufficient.
2012/2/8 Paul Brook <paul@codesourcery.com> > > > I suspect we want to replace the arm_load_kernel call with an > > > arm_linux_loader device with appropriate properties. > > > > Ok, so does this mean the machine model would still explicitly > instantiate > > the bootloader device? > > Yes. Bootloaders inherently have machine specific knowledge. They need to > know ram location, board ID, secondary CPU boot protocols, etc. Requiring > the > user specify all these things separately from the rest of the machine > description is IMO not acceptable. > > So what im suggesting here is that machines export these properties to a globally accessible location. Perhaps via the machine opts mechanism? Then we are in a best of both worls situation where machine models do not need bootloader awareness yet bootloaders can still query qemu for ram_size, smp#, board_id and friends. > > Will there be a way of removing the bootloader from > > the machine model so if I need to create my own custom bootloader flow i > > can? We are doing some custom non-linux boots in our application where > > arm-boot.c is not working for us so being able to swap out arm_boot.c for > > another implemenation is one of our goals. The bootloader-as-a-device > flow > > is ideal for this but only works if a user can choose their bootloader on > > the command line. > > I think that's solved the same lines as any other custom machine variant. > It's also part of the reason I think it's important to cleanly separate > devices that provide functionality from those that are just convenience > objects for creating a particular assembly of devices. > > Having generic ELF and maybe binary blob image loaders is certainly useful. > They can all coexist happily within the same machine. > > I'm inclined to say that anything else isn't worth the effort. Users with > other special needs can use third party tools[1] to embed/wrap their magic > in > a regular image. > > Paul > > [1] In many cases cases you just need to splat the supplied > bootloader+kernel > images into ram, not postprocessing is necessary. For most of the rest > objcopy is sufficient. >
> 2012/2/8 Paul Brook <paul@codesourcery.com> > > > > > I suspect we want to replace the arm_load_kernel call with an > > > > arm_linux_loader device with appropriate properties. > > > > > > Ok, so does this mean the machine model would still explicitly > > > instantiate the bootloader device? > > > > Yes. Bootloaders inherently have machine specific knowledge. They need > > to know ram location, board ID, secondary CPU boot protocols, etc. > > Requiring the user specify all these things separately from the rest of > > the machine description is IMO not acceptable. > > So what im suggesting here is that machines export these properties to a > globally accessible location. Perhaps via the machine opts mechanism? Then > we are in a best of both worls situation where machine models do not need > bootloader awareness yet bootloaders can still query qemu for ram_size, > smp#, board_id and friends. Hmm, I suppose this might work. I'm not sure what you think the benefit of this is though. Fact is the machine needs to have bootloader awareness, whether it be instantating an object or setting magic variables. Having devices rummage around in global state feels messy. I'd much rather use actual properties on the device. IMO changing the bootloader is similar complexity to (say) changing a UART. i.e. it's a board-level change not an end-user level change. Board-level changes are something that will happen after QOM conversion, i.e. when we replace machine->init with a board config file. Paul
On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 13:27, Paul Brook wrote: > > >> 2012/2/8 Paul Brook <paul@codesourcery.com> > >> > >>>>> I suspect we want to replace the arm_load_kernel call with an > >>>>> arm_linux_loader device with appropriate properties. > >>>> > >>>> Ok, so does this mean the machine model would still explicitly > >>>> instantiate the bootloader device? > >>> > >>> Yes. Bootloaders inherently have machine specific knowledge. They > need > >>> to know ram location, board ID, secondary CPU boot protocols, etc. > >>> Requiring the user specify all these things separately from the rest of > >>> the machine description is IMO not acceptable. > >> > >> So what im suggesting here is that machines export these properties to a > >> globally accessible location. Perhaps via the machine opts mechanism? > Then > >> we are in a best of both worls situation where machine models do not > need > >> bootloader awareness yet bootloaders can still query qemu for ram_size, > >> smp#, board_id and friends. > > > > Hmm, I suppose this might work. I'm not sure what you think the benefit > of > > this is though. Fact is the machine needs to have bootloader awareness, > > whether it be instantating an object or setting magic variables. > > Having devices rummage around in global state feels messy. I'd much > rather > > use actual properties on the device. IMO changing the bootloader is > similar > > complexity to (say) changing a UART. i.e. it's a board-level change not > an > > end-user level change. Board-level changes are something that will > happen > > after QOM conversion, i.e. when we replace machine->init with a board > config > > file. > > > Yeah, basically the variable flow goes: > > vl.c -> machine_opts -> machine_init() -> device properties -> > device_init() > So that the machine init function that creates the bootloader device > enumerates the machine_opts (just like is done in Peter's patches) and then > passes those on to the bootloader device as device properties. > > So in patch 4/4 in Peters series where he adds a new bootloader feature (the -dtb switch) its done slightly differently, the machine model does not handle the machine_opts at all, i.e. The machine model has no awareness of this dtb argument. Instead the arm boot loader directly queries the machine_opts API itself: @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) exit(1); } + info->dtb_filename = qemu_opt_get(qemu_opts_find( qemu_find_opts("machine"), + 0), "dtb"); + There is no path through the machine_init for this particular property. What I am suggesting is that a similar approach is take for machine model set properties (such as ram_size), but instead of the command line setting the props its done by the machine model. The machine model qemu_opt_set() the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did this for the key properties related to boot then you would remove the need for machines to have awareness of their boot process. > The rationale behind machine opts is that they're basically a dynamic > number of properties for the not-yet-existing machine object. > > > Alex > >
On 08.02.2012, at 14:04, Peter Crosthwaite wrote: > > > On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 13:27, Paul Brook wrote: > > >> 2012/2/8 Paul Brook <paul@codesourcery.com> > >> > >>>>> I suspect we want to replace the arm_load_kernel call with an > >>>>> arm_linux_loader device with appropriate properties. > >>>> > >>>> Ok, so does this mean the machine model would still explicitly > >>>> instantiate the bootloader device? > >>> > >>> Yes. Bootloaders inherently have machine specific knowledge. They need > >>> to know ram location, board ID, secondary CPU boot protocols, etc. > >>> Requiring the user specify all these things separately from the rest of > >>> the machine description is IMO not acceptable. > >> > >> So what im suggesting here is that machines export these properties to a > >> globally accessible location. Perhaps via the machine opts mechanism? Then > >> we are in a best of both worls situation where machine models do not need > >> bootloader awareness yet bootloaders can still query qemu for ram_size, > >> smp#, board_id and friends. > > > > Hmm, I suppose this might work. I'm not sure what you think the benefit of > > this is though. Fact is the machine needs to have bootloader awareness, > > whether it be instantating an object or setting magic variables. > > Having devices rummage around in global state feels messy. I'd much rather > > use actual properties on the device. IMO changing the bootloader is similar > > complexity to (say) changing a UART. i.e. it's a board-level change not an > > end-user level change. Board-level changes are something that will happen > > after QOM conversion, i.e. when we replace machine->init with a board config > > file. > > > Yeah, basically the variable flow goes: > > vl.c -> machine_opts -> machine_init() -> device properties -> device_init() > > So that the machine init function that creates the bootloader device enumerates the machine_opts (just like is done in Peter's patches) and then passes those on to the bootloader device as device properties. > > > So in patch 4/4 in Peters series where he adds a new bootloader feature (the -dtb switch) its done slightly differently, the machine model does not handle the machine_opts at all, i.e. The machine model has no awareness of this dtb argument. Instead the arm boot loader directly queries the machine_opts API itself: > > @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) > exit(1); > } > > + info->dtb_filename = qemu_opt_get(qemu_opts_find(qemu_find_opts("machine"), > + 0), "dtb"); > + > > There is no path through the machine_init for this particular property. Ah, I see. So he derived from the original proposal, oh well :). > What I am suggesting is that a similar approach is take for machine model set properties (such as ram_size), but instead of the command line setting the props its done by the machine model. The machine model qemu_opt_set() the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did this for the key properties related to boot then you would remove the need for machines to have awareness of their boot process. But that's exactly what we want. We want the machine to be aware of its boot process, because that's the one place that needs to instantiate the boot device, no? Alex
On Wed, Feb 8, 2012 at 11:10 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 14:04, Peter Crosthwaite wrote: > > > > On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: > >> >> On 08.02.2012, at 13:27, Paul Brook wrote: >> >> >> 2012/2/8 Paul Brook <paul@codesourcery.com> >> >> >> >>>>> I suspect we want to replace the arm_load_kernel call with an >> >>>>> arm_linux_loader device with appropriate properties. >> >>>> >> >>>> Ok, so does this mean the machine model would still explicitly >> >>>> instantiate the bootloader device? >> >>> >> >>> Yes. Bootloaders inherently have machine specific knowledge. They >> need >> >>> to know ram location, board ID, secondary CPU boot protocols, etc. >> >>> Requiring the user specify all these things separately from the rest >> of >> >>> the machine description is IMO not acceptable. >> >> >> >> So what im suggesting here is that machines export these properties to >> a >> >> globally accessible location. Perhaps via the machine opts mechanism? >> Then >> >> we are in a best of both worls situation where machine models do not >> need >> >> bootloader awareness yet bootloaders can still query qemu for ram_size, >> >> smp#, board_id and friends. >> > >> > Hmm, I suppose this might work. I'm not sure what you think the >> benefit of >> > this is though. Fact is the machine needs to have bootloader awareness, >> > whether it be instantating an object or setting magic variables. >> > Having devices rummage around in global state feels messy. I'd much >> rather >> > use actual properties on the device. IMO changing the bootloader is >> similar >> > complexity to (say) changing a UART. i.e. it's a board-level change not >> an >> > end-user level change. Board-level changes are something that will >> happen >> > after QOM conversion, i.e. when we replace machine->init with a board >> config >> > file. >> >> >> Yeah, basically the variable flow goes: >> >> vl.c -> machine_opts -> machine_init() -> device properties -> >> device_init() >> > > So that the machine init function that creates the bootloader device >> enumerates the machine_opts (just like is done in Peter's patches) and then >> passes those on to the bootloader device as device properties. >> >> > So in patch 4/4 in Peters series where he adds a new bootloader feature > (the -dtb switch) its done slightly differently, the machine model does not > handle the machine_opts at all, i.e. The machine model has no awareness of > this dtb argument. Instead the arm boot loader directly queries the > machine_opts API itself: > > @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct > arm_boot_info *info) > exit(1); > } > > + info->dtb_filename = qemu_opt_get(qemu_opts_find( > qemu_find_opts("machine"), > + 0), "dtb"); > + > > There is no path through the machine_init for this particular property. > > > Ah, I see. So he derived from the original proposal, oh well :). > > Isn't this the best approach tho? If you changed it over to the other flow, then you would end up with a change pattern where every machine model would need to get and pass the new argument. This way the diff is limited to the command line infrastructure and the bootloader. > What I am suggesting is that a similar approach is take for machine model > set properties (such as ram_size), but instead of the command line setting > the props its done by the machine model. The machine model qemu_opt_set() > the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did > this for the key properties related to boot then you would remove the need > for machines to have awareness of their boot process. > > > But that's exactly what we want. We want the machine to be aware of its > boot process, because that's the one place that needs to instantiate the > boot device, no? > > More a case of the reverse isnt it? The bootloader needs to know about the machine its generating code for, but the machine generation process doesnt need to know about the software its going to run. The machine being aware of the bootloader implementation and instantiating it you are putting in place a policy where you forcing a particular bootflow on every user of that machine. The hardware is placing policy on what software its running. In the case of arm boot you end up with a situation where you are trying to write a bootloader that can handle every possible scenario, what I am suggesting is arm_boot.c as it is becomes a linux specific bootloader and other bootflows such as booting from elfs or raw images (or other unforseen bootflows) are different bootloader devices. The choice of which bootloader you use is driven by which -device you put down on command line. > > Alex > > Peter
On 08.02.2012, at 14:30, Peter Crosthwaite wrote: > > > On Wed, Feb 8, 2012 at 11:10 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 14:04, Peter Crosthwaite wrote: > >> >> >> On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: >> >> On 08.02.2012, at 13:27, Paul Brook wrote: >> >> >> 2012/2/8 Paul Brook <paul@codesourcery.com> >> >> >> >>>>> I suspect we want to replace the arm_load_kernel call with an >> >>>>> arm_linux_loader device with appropriate properties. >> >>>> >> >>>> Ok, so does this mean the machine model would still explicitly >> >>>> instantiate the bootloader device? >> >>> >> >>> Yes. Bootloaders inherently have machine specific knowledge. They need >> >>> to know ram location, board ID, secondary CPU boot protocols, etc. >> >>> Requiring the user specify all these things separately from the rest of >> >>> the machine description is IMO not acceptable. >> >> >> >> So what im suggesting here is that machines export these properties to a >> >> globally accessible location. Perhaps via the machine opts mechanism? Then >> >> we are in a best of both worls situation where machine models do not need >> >> bootloader awareness yet bootloaders can still query qemu for ram_size, >> >> smp#, board_id and friends. >> > >> > Hmm, I suppose this might work. I'm not sure what you think the benefit of >> > this is though. Fact is the machine needs to have bootloader awareness, >> > whether it be instantating an object or setting magic variables. >> > Having devices rummage around in global state feels messy. I'd much rather >> > use actual properties on the device. IMO changing the bootloader is similar >> > complexity to (say) changing a UART. i.e. it's a board-level change not an >> > end-user level change. Board-level changes are something that will happen >> > after QOM conversion, i.e. when we replace machine->init with a board config >> > file. >> >> >> Yeah, basically the variable flow goes: >> >> vl.c -> machine_opts -> machine_init() -> device properties -> device_init() >> >> So that the machine init function that creates the bootloader device enumerates the machine_opts (just like is done in Peter's patches) and then passes those on to the bootloader device as device properties. >> >> >> So in patch 4/4 in Peters series where he adds a new bootloader feature (the -dtb switch) its done slightly differently, the machine model does not handle the machine_opts at all, i.e. The machine model has no awareness of this dtb argument. Instead the arm boot loader directly queries the machine_opts API itself: >> >> @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) >> exit(1); >> } >> >> + info->dtb_filename = qemu_opt_get(qemu_opts_find(qemu_find_opts("machine"), >> + 0), "dtb"); >> + >> >> There is no path through the machine_init for this particular property. > > Ah, I see. So he derived from the original proposal, oh well :). > > > Isn't this the best approach tho? If you changed it over to the other flow, then you would end up with a change pattern where every machine model would need to get and pass the new argument. This way the diff is limited to the command line infrastructure and the bootloader. If you want the smallest diff, just make things globals and call it a day. This whole thing is a discussion around device architecture, right? > >> What I am suggesting is that a similar approach is take for machine model set properties (such as ram_size), but instead of the command line setting the props its done by the machine model. The machine model qemu_opt_set() the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did this for the key properties related to boot then you would remove the need for machines to have awareness of their boot process. > > But that's exactly what we want. We want the machine to be aware of its boot process, because that's the one place that needs to instantiate the boot device, no? > > > More a case of the reverse isnt it? The bootloader needs to know about the machine its generating code for, but the machine generation process doesnt need to know about the software its going to run. The machine being aware of the bootloader implementation and instantiating it you are putting in place a policy where you forcing a particular bootflow on every user of that machine. The hardware is placing policy on what software its running. > > In the case of arm boot you end up with a situation where you are trying to write a bootloader that can handle every possible scenario, what I am suggesting is arm_boot.c as it is becomes a linux specific bootloader and other bootflows such as booting from elfs or raw images (or other unforseen bootflows) are different bootloader devices. The choice of which bootloader you use is driven by which -device you put down on command line. Hrm. I wouldn't want to have to select different bootloader types using -device. Today, we have autodetect code in almost all targets that checks the binary and figures out what to load from it. So on x86 you just do -kernel foo and if foo is a Linux kernel, it will run it using that loader, while if it's a multiboot image, it will load it through that. Any reason you can't just upstream your specific bootloader code? We can then worry about replacing the loader device at a different point in time and model things easily now. Alex
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,
On Wed, Feb 8, 2012 at 11:35 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 14:30, Peter Crosthwaite wrote: > > > > On Wed, Feb 8, 2012 at 11:10 PM, Alexander Graf <agraf@suse.de> wrote: > >> >> On 08.02.2012, at 14:04, Peter Crosthwaite wrote: >> >> >> >> On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: >> >>> >>> On 08.02.2012, at 13:27, Paul Brook wrote: >>> >>> >> 2012/2/8 Paul Brook <paul@codesourcery.com> >>> >> >>> >>>>> I suspect we want to replace the arm_load_kernel call with an >>> >>>>> arm_linux_loader device with appropriate properties. >>> >>>> >>> >>>> Ok, so does this mean the machine model would still explicitly >>> >>>> instantiate the bootloader device? >>> >>> >>> >>> Yes. Bootloaders inherently have machine specific knowledge. They >>> need >>> >>> to know ram location, board ID, secondary CPU boot protocols, etc. >>> >>> Requiring the user specify all these things separately from the rest >>> of >>> >>> the machine description is IMO not acceptable. >>> >> >>> >> So what im suggesting here is that machines export these properties >>> to a >>> >> globally accessible location. Perhaps via the machine opts mechanism? >>> Then >>> >> we are in a best of both worls situation where machine models do not >>> need >>> >> bootloader awareness yet bootloaders can still query qemu for >>> ram_size, >>> >> smp#, board_id and friends. >>> > >>> > Hmm, I suppose this might work. I'm not sure what you think the >>> benefit of >>> > this is though. Fact is the machine needs to have bootloader >>> awareness, >>> > whether it be instantating an object or setting magic variables. >>> > Having devices rummage around in global state feels messy. I'd much >>> rather >>> > use actual properties on the device. IMO changing the bootloader is >>> similar >>> > complexity to (say) changing a UART. i.e. it's a board-level change >>> not an >>> > end-user level change. Board-level changes are something that will >>> happen >>> > after QOM conversion, i.e. when we replace machine->init with a board >>> config >>> > file. >>> >>> >>> Yeah, basically the variable flow goes: >>> >>> vl.c -> machine_opts -> machine_init() -> device properties -> >>> device_init() >>> >> >> So that the machine init function that creates the bootloader device >>> enumerates the machine_opts (just like is done in Peter's patches) and then >>> passes those on to the bootloader device as device properties. >>> >>> >> So in patch 4/4 in Peters series where he adds a new bootloader feature >> (the -dtb switch) its done slightly differently, the machine model does not >> handle the machine_opts at all, i.e. The machine model has no awareness of >> this dtb argument. Instead the arm boot loader directly queries the >> machine_opts API itself: >> >> @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct >> arm_boot_info *info) >> exit(1); >> } >> >> + info->dtb_filename = qemu_opt_get(qemu_opts_find( >> qemu_find_opts("machine"), >> + 0), "dtb"); >> + >> >> There is no path through the machine_init for this particular property. >> >> >> Ah, I see. So he derived from the original proposal, oh well :). >> >> > Isn't this the best approach tho? If you changed it over to the other > flow, then you would end up with a change pattern where every machine model > would need to get and pass the new argument. This way the diff is limited > to the command line infrastructure and the bootloader. > > > If you want the smallest diff, just make things globals and call it a day. > This whole thing is a discussion around device architecture, right? > > So if we consider this bootloader a device and its -dtb argument a property of that device, then what you are implying is that every device property of every device in a machine must be managed by the machine model? Isn't the dynamic machine model work that is in progress is trying to get away the approach where fixed machine models have to micromanage all their attached devices? with the ultimate goal of -no-machine how will the bootloader get this dtb argument? > > >> What I am suggesting is that a similar approach is take for machine model >> set properties (such as ram_size), but instead of the command line setting >> the props its done by the machine model. The machine model qemu_opt_set() >> the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did >> this for the key properties related to boot then you would remove the need >> for machines to have awareness of their boot process. >> >> >> But that's exactly what we want. We want the machine to be aware of its >> boot process, because that's the one place that needs to instantiate the >> boot device, no? >> >> > More a case of the reverse isnt it? The bootloader needs to know about the > machine its generating code for, but the machine generation process doesnt > need to know about the software its going to run. The machine being aware > of the bootloader implementation and instantiating it you are putting in > place a policy where you forcing a particular bootflow on every user of > that machine. The hardware is placing policy on what software its running. > > In the case of arm boot you end up with a situation where you are trying > to write a bootloader that can handle every possible scenario, what I am > suggesting is arm_boot.c as it is becomes a linux specific bootloader and > other bootflows such as booting from elfs or raw images (or other unforseen > bootflows) are different bootloader devices. The choice of which bootloader > you use is driven by which -device you put down on command line. > > > Hrm. I wouldn't want to have to select different bootloader types using > -device. Today, we have autodetect code in almost all targets that checks > the binary and figures out what to load from it. So on x86 you just do > -kernel foo and if foo is a Linux kernel, it will run it using that loader, > while if it's a multiboot image, it will load it through that. > > Any reason you can't just upstream your specific bootloader code? > Its currently just a set of hacks to arm_boot.c, but for upstream acceptance you would need to add serveral more options to arm_boot (like -dtb) to be able to do this needed parameteristation. It would be tedious to have to editevery arm platform to pass in a suite of options from the machine model. > We can then worry about replacing the loader device at a different point > in time and model things easily now. > > > Alex > >
On 08.02.2012, at 15:05, Peter Crosthwaite wrote: > > > On Wed, Feb 8, 2012 at 11:35 PM, Alexander Graf <agraf@suse.de> wrote: > > On 08.02.2012, at 14:30, Peter Crosthwaite wrote: > >> >> >> On Wed, Feb 8, 2012 at 11:10 PM, Alexander Graf <agraf@suse.de> wrote: >> >> On 08.02.2012, at 14:04, Peter Crosthwaite wrote: >> >>> >>> >>> On Wed, Feb 8, 2012 at 10:41 PM, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 08.02.2012, at 13:27, Paul Brook wrote: >>> >>> >> 2012/2/8 Paul Brook <paul@codesourcery.com> >>> >> >>> >>>>> I suspect we want to replace the arm_load_kernel call with an >>> >>>>> arm_linux_loader device with appropriate properties. >>> >>>> >>> >>>> Ok, so does this mean the machine model would still explicitly >>> >>>> instantiate the bootloader device? >>> >>> >>> >>> Yes. Bootloaders inherently have machine specific knowledge. They need >>> >>> to know ram location, board ID, secondary CPU boot protocols, etc. >>> >>> Requiring the user specify all these things separately from the rest of >>> >>> the machine description is IMO not acceptable. >>> >> >>> >> So what im suggesting here is that machines export these properties to a >>> >> globally accessible location. Perhaps via the machine opts mechanism? Then >>> >> we are in a best of both worls situation where machine models do not need >>> >> bootloader awareness yet bootloaders can still query qemu for ram_size, >>> >> smp#, board_id and friends. >>> > >>> > Hmm, I suppose this might work. I'm not sure what you think the benefit of >>> > this is though. Fact is the machine needs to have bootloader awareness, >>> > whether it be instantating an object or setting magic variables. >>> > Having devices rummage around in global state feels messy. I'd much rather >>> > use actual properties on the device. IMO changing the bootloader is similar >>> > complexity to (say) changing a UART. i.e. it's a board-level change not an >>> > end-user level change. Board-level changes are something that will happen >>> > after QOM conversion, i.e. when we replace machine->init with a board config >>> > file. >>> >>> >>> Yeah, basically the variable flow goes: >>> >>> vl.c -> machine_opts -> machine_init() -> device properties -> device_init() >>> >>> So that the machine init function that creates the bootloader device enumerates the machine_opts (just like is done in Peter's patches) and then passes those on to the bootloader device as device properties. >>> >>> >>> So in patch 4/4 in Peters series where he adds a new bootloader feature (the -dtb switch) its done slightly differently, the machine model does not handle the machine_opts at all, i.e. The machine model has no awareness of this dtb argument. Instead the arm boot loader directly queries the machine_opts API itself: >>> >>> @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) >>> exit(1); >>> } >>> >>> + info->dtb_filename = qemu_opt_get(qemu_opts_find(qemu_find_opts("machine"), >>> + 0), "dtb"); >>> + >>> >>> There is no path through the machine_init for this particular property. >> >> Ah, I see. So he derived from the original proposal, oh well :). >> >> >> Isn't this the best approach tho? If you changed it over to the other flow, then you would end up with a change pattern where every machine model would need to get and pass the new argument. This way the diff is limited to the command line infrastructure and the bootloader. > > If you want the smallest diff, just make things globals and call it a day. This whole thing is a discussion around device architecture, right? > > > So if we consider this bootloader a device and its -dtb argument a property of that device, then what you are implying is that every device property of every device in a machine must be managed by the machine model? Isn't the dynamic machine model work that is in progress is trying to get away the approach where fixed machine models have to micromanage all their attached devices? with the ultimate goal of -no-machine how will the bootloader get this dtb argument? That is the point really. With a machine model, the machine model creates the device with all its properties (kernel file name, initrd file name, dtb file name) while with -no-machine, you would manually create the bootloader device with -device ...,kernel=..,initrd=... If the device would read machine_opts itself, we couldn't do the -device part. >> >>> What I am suggesting is that a similar approach is take for machine model set properties (such as ram_size), but instead of the command line setting the props its done by the machine model. The machine model qemu_opt_set() the ram_size = whatever. Then the bootloader qemu_opt_get()s it. If you did this for the key properties related to boot then you would remove the need for machines to have awareness of their boot process. >> >> But that's exactly what we want. We want the machine to be aware of its boot process, because that's the one place that needs to instantiate the boot device, no? >> >> >> More a case of the reverse isnt it? The bootloader needs to know about the machine its generating code for, but the machine generation process doesnt need to know about the software its going to run. The machine being aware of the bootloader implementation and instantiating it you are putting in place a policy where you forcing a particular bootflow on every user of that machine. The hardware is placing policy on what software its running. >> >> In the case of arm boot you end up with a situation where you are trying to write a bootloader that can handle every possible scenario, what I am suggesting is arm_boot.c as it is becomes a linux specific bootloader and other bootflows such as booting from elfs or raw images (or other unforseen bootflows) are different bootloader devices. The choice of which bootloader you use is driven by which -device you put down on command line. > > Hrm. I wouldn't want to have to select different bootloader types using -device. Today, we have autodetect code in almost all targets that checks the binary and figures out what to load from it. So on x86 you just do -kernel foo and if foo is a Linux kernel, it will run it using that loader, while if it's a multiboot image, it will load it through that. > > Any reason you can't just upstream your specific bootloader code? > > Its currently just a set of hacks to arm_boot.c, but for upstream acceptance you would need to add serveral more options to arm_boot (like -dtb) to be able to do this needed parameteristation. It would be tedious to have to editevery arm platform to pass in a suite of options from the machine model. Hrm, ok :). So how about we model everything as a single "arm kernel boot loader" device? Get that one layered correctly. And once we have that, you can add your bootloader code to it with automatic detection. Would that work? Alex
> So if we consider this bootloader a device and its -dtb argument a property > of that device, then what you are implying is that every device property of > every device in a machine must be managed by the machine model? Isn't the > dynamic machine model work that is in progress is trying to get away the > approach where fixed machine models have to micromanage all their attached > devices? with the ultimate goal of -no-machine how will the bootloader get > this dtb argument? My expectation is that we have some way for the machine description to tag objects/properties that pap to the generic commandline options. In the same way that we'd probably want to tag the default PCI bus, or UARTs for matching with the -serial commandline. Some of these can be guessed, others you need the machine description to tell you. Paul
2012/2/9 Paul Brook <paul@codesourcery.com> > > So if we consider this bootloader a device and its -dtb argument a > property > > of that device, then what you are implying is that every device property > of > > every device in a machine must be managed by the machine model? Isn't the > > dynamic machine model work that is in progress is trying to get away the > > approach where fixed machine models have to micromanage all their > attached > > devices? with the ultimate goal of -no-machine how will the bootloader > get > > this dtb argument? > > My expectation is that we have some way for the machine description to tag > objects/properties that pap to the generic commandline options. In the > same > way that we'd probably want to tag the default PCI bus, or UARTs for > matching > with the -serial commandline. Some of these can be guessed, others you > need > the machine description to tell you. > > Paul > Ok, so it seems that for command line driven arguments the policy is with standard arguments such as -dtb, the machine model must get it on the device behalf and instantiate the device, and that fits in with this patch. Its the other problem I am more worried about, i.e. when I -device instantiate my bootloader with an existing machine how do I get my ram_size and board_ID? The no machine opts for devices policy makes this impossible such that I would have to pass in board_id and ram_size to the boot-loader on the command line. Is there any acceptable way where the machine model can make something globally available to devices for the purpose of instantiating them with -device?
> Its the other problem I am more worried about, i.e. when I -device > instantiate my bootloader with an existing machine how do I get my ram_size > and board_ID? The no machine opts for devices policy makes this impossible > such that I would have to pass in board_id and ram_size to > the boot-loader on the command line. Is there any acceptable way where the > machine model can make something globally available to devices for the > purpose of instantiating them with -device? I'm not convinved this is a problem worth solving. i.e. is it really worth consirering the bootloader a user-replaceable part of the machine (without actually changing the machine description)? Making our bootloader not suck seems a better option. Paul
So here are some of the problems im trying to solve with the bootloader: Smp bootstrap secondary CPUs while loading an elf (currently elfs will be assumed to be not kernels). Change the kernel, initrd and dtb load address on the command line. Use my own SMP secondary bootloop. My intention with this patch was to set myself to do boot parameterizations on the command line by just adding them as qdev props to the arm_linux_loader and set them using -device instantiation. E.G. -device arm_boot_loader,initrd_addr=0x10000000. But if I take the approach you are suggesting, the for this initrd load address option, I would need to add myself a command line option, fetch that command line option in every arm machine model and then pass it to arm_load_kernel. The change pattern is huge and is intrusive to every arm machine model. Whereas what I am suggesting would limit changes to only the bootloader device. 2012/2/9 Paul Brook <paul@codesourcery.com> > > Its the other problem I am more worried about, i.e. when I -device > > instantiate my bootloader with an existing machine how do I get my > ram_size > > and board_ID? The no machine opts for devices policy makes this > impossible > > such that I would have to pass in board_id and ram_size to > > the boot-loader on the command line. Is there any acceptable way where > the > > machine model can make something globally available to devices for the > > purpose of instantiating them with -device? > > I'm not convinved this is a problem worth solving. i.e. is it really worth > consirering the bootloader a user-replaceable part of the machine (without > actually changing the machine description)? Making our bootloader not suck > seems a better option. > > Paul >
> So here are some of the problems im trying to solve with the bootloader: > > Smp bootstrap secondary CPUs while loading an elf (currently elfs will be > assumed to be not kernels). > Change the kernel, initrd and dtb load address on the command line. > Use my own SMP secondary bootloop. > > My intention with this patch was to set myself to do boot parameterizations > on the command line by just adding them as qdev props to the > arm_linux_loader and set them using -device instantiation. E.G. -device > arm_boot_loader,initrd_addr=0x10000000. But if I take the approach you are > suggesting, the for this initrd load address option, I would need to add > myself a command line option, fetch that command line option in every arm > machine model and then pass it to arm_load_kernel. No. You just set/override properties on the device that the machine created. I thought we already had this, but it looks like the closest we have is -global. Something like "-property /devicepath/propertyname=value". This is something we need anyway. Creating new devices with -device is of very limited value if you can't link existing device properties to that new device. Remember that in the new world order we don't have magic bus-device, everything is done via link properlties. Paul
2012/2/9 Paul Brook <paul@codesourcery.com> > > So here are some of the problems im trying to solve with the bootloader: > > > > Smp bootstrap secondary CPUs while loading an elf (currently elfs will be > > assumed to be not kernels). > > Change the kernel, initrd and dtb load address on the command line. > > Use my own SMP secondary bootloop. > > > > My intention with this patch was to set myself to do boot > parameterizations > > on the command line by just adding them as qdev props to the > > arm_linux_loader and set them using -device instantiation. E.G. -device > > arm_boot_loader,initrd_addr=0x10000000. But if I take the approach you > are > > suggesting, the for this initrd load address option, I would need to add > > myself a command line option, fetch that command line option in every arm > > machine model and then pass it to arm_load_kernel. > > No. You just set/override properties on the device that the machine > created. > I thought we already had this, but it looks like the closest we have is > -global. > > Something like "-property /devicepath/propertyname=value". > This is something we need anyway. Creating new devices with -device is of > very limited value if you can't link existing device properties to that new > device. Remember that in the new world order we don't have magic > bus-device, > everything is done via link properlties. > > Ok, that sounds more workable :). So i would add my initrd_addr property to the bootloader as a qdev prop as I suggested before, then something like: qemu-system-arm -M verstailepb -property /foo/bar/arm_linux_loader.0,initrd_addr=0x10000000 ? Paul > Peter
> Ok, that sounds more workable :). So i would add my initrd_addr property to > the bootloader as a qdev prop as I suggested before, then something like: > > qemu-system-arm -M verstailepb -property > /foo/bar/arm_linux_loader.0,initrd_addr=0x10000000 Yes. There are various implementation/syntax details to resolve, but in principle I think it should work a lot like that. Paul
On 02/08/2012 10:03 AM, Peter Crosthwaite wrote: > 2012/2/9 Paul Brook<paul@codesourcery.com> > >>> So here are some of the problems im trying to solve with the bootloader: >>> >>> Smp bootstrap secondary CPUs while loading an elf (currently elfs will be >>> assumed to be not kernels). >>> Change the kernel, initrd and dtb load address on the command line. >>> Use my own SMP secondary bootloop. >>> >>> My intention with this patch was to set myself to do boot >> parameterizations >>> on the command line by just adding them as qdev props to the >>> arm_linux_loader and set them using -device instantiation. E.G. -device >>> arm_boot_loader,initrd_addr=0x10000000. But if I take the approach you >> are >>> suggesting, the for this initrd load address option, I would need to add >>> myself a command line option, fetch that command line option in every arm >>> machine model and then pass it to arm_load_kernel. >> >> No. You just set/override properties on the device that the machine >> created. >> I thought we already had this, but it looks like the closest we have is >> -global. >> >> Something like "-property /devicepath/propertyname=value". A lot of refactoring is needed to make this a reality. Please read through the various QOM threads for details of what needs to be done. Regards, Anthony Liguori
On 02/08/2012 10:15 AM, Paul Brook wrote: >> Ok, that sounds more workable :). So i would add my initrd_addr property to >> the bootloader as a qdev prop as I suggested before, then something like: >> >> qemu-system-arm -M verstailepb -property >> /foo/bar/arm_linux_loader.0,initrd_addr=0x10000000 > > Yes. > > There are various implementation/syntax details to resolve, but in principle I > think it should work a lot like that. There's already a qom-set that takes a path, property, and value. It works with Visitors. To bridge this to the command line, you would need to make a Visitor that defined some syntax for mapping strings to types (pretty trivial really). But before we can even think about this, we need to refactor the device model such that we have a clean separation between initial creation (including creation of children devices) and initialization that requires properties to be set (realization). I posted a series for the i440fx that started doing this refactoring for the PC. Regards, Anthony Liguori > > Paul >
Alrighty,
So it seems like the bootloader as a device idea has some support, just
need to work out a few implementaiton details. It seems the consensus is
that machine models will instantiate the device. The latest idea is the
machine model will pass some of core props to the bootloader while others
can come over the command line via a yet to be implemented -property
interface.
But pretty much any of the alternatives mentioned here start with
converting the existing bootloader parameters over to qdev props. So here
are the properties of the bootloader (from arm-misc.c):
struct arm_boot_info {
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,
* and the GIC address in the next 3 values, respectively. boards that
* have their own boot functions can use these values as they want.
*/
target_phys_addr_t smp_loader_start;
target_phys_addr_t smp_bootreg_addr;
target_phys_addr_t smp_priv_base;
int nb_cpus;
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
* work, then write_secondary_boot() should write a suitable blob of
* code mimicing the secondary CPU startup process used by the board's
* boot loader/boot ROM code, and secondary_cpu_reset_hook() should
* perform any necessary CPU reset handling and set the PC for thei
* secondary CPUs to point at this boot blob.
*/
void (*write_secondary_boot)(CPUState *env,
const struct arm_boot_info *info);
void (*secondary_cpu_reset_hook)(CPUState *env,
const struct arm_boot_info *info);
};
The addresses, ints and string are easy, but the hard ones are the
callbacks. The qdev ptr is a possible implementation but I see a movement
away from that. I guess the solution is to provide an abstraction through
QOM no? The boards that need to implement their own secondary
boot-loop/reset (theres only 1) or atag boot sequence (again only 1) create
an object that inherits for the arm boot device and instantiate that?
Regards,
Peter
2012/2/9 Anthony Liguori <anthony@codemonkey.ws>
> On 02/08/2012 10:15 AM, Paul Brook wrote:
>
>> Ok, that sounds more workable :). So i would add my initrd_addr property
>>> to
>>> the bootloader as a qdev prop as I suggested before, then something like:
>>>
>>> qemu-system-arm -M verstailepb -property
>>> /foo/bar/arm_linux_loader.0,**initrd_addr=0x10000000
>>>
>>
>> Yes.
>>
>> There are various implementation/syntax details to resolve, but in
>> principle I
>> think it should work a lot like that.
>>
>
> There's already a qom-set that takes a path, property, and value. It
> works with Visitors. To bridge this to the command line, you would need to
> make a Visitor that defined some syntax for mapping strings to types
> (pretty trivial really).
>
> But before we can even think about this, we need to refactor the device
> model such that we have a clean separation between initial creation
> (including creation of children devices) and initialization that requires
> properties to be set (realization).
>
> I posted a series for the i440fx that started doing this refactoring for
> the PC.
>
> Regards,
>
> Anthony Liguori
>
>
>> Paul
>>
>>
>
> /* multicore boards that use the default secondary core boot functions > * can ignore these two function calls. If the default functions won't > * work, then write_secondary_boot() should write a suitable blob of > * code mimicing the secondary CPU startup process used by the board's > * boot loader/boot ROM code, and secondary_cpu_reset_hook() should > * perform any necessary CPU reset handling and set the PC for thei > * secondary CPUs to point at this boot blob. > */ > void (*write_secondary_boot)(CPUState *env, > const struct arm_boot_info *info); > void (*secondary_cpu_reset_hook)(CPUState *env, > const struct arm_boot_info *info); > }; > > The addresses, ints and string are easy, but the hard ones are the > callbacks. The qdev ptr is a possible implementation but I see a movement > away from that. I guess the solution is to provide an abstraction through > QOM no? The boards that need to implement their own secondary > boot-loop/reset (theres only 1) or atag boot sequence (again only 1) create > an object that inherits for the arm boot device and instantiate that? This sounds like an ideal use-case for derived classes and virtual functions. Paul
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,
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 >
On 8 February 2012 13:47, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 02/08/2012 06:41 AM, Alexander Graf wrote: >> Yeah, basically the variable flow goes: >> >> vl.c -> machine_opts -> machine_init() -> device properties -> >> device_init() > > And the rationale is that machine_init() will do nothing other than use QOM > primitives to create a set of expected devices and set up their properties > such that a person (or management tool) could do everything that > machine_init() is doing. So I think this flow is wrong (and indeed I didn't implement it that way in my patches to add machine opts for kernel/initrd/dtb) -- the machine_init() shouldn't have to care about this because we don't want to have to modify a huge set of machine init functions every time we add an extra option that only the boot loader cares about. I don't particularly care how we QOMify arm_boot (it's not exactly at the top of my priority list demanding attention), I do care that (a) we have a sensible user-facing interface [ie command line options] and (b) vl.c can usefully just pass the information from those options straight to the boot loader code. -- PMM
Am 20.02.2012 20:43, schrieb Peter Maydell: > On 8 February 2012 13:47, Anthony Liguori <anthony@codemonkey.ws> wrote: >> On 02/08/2012 06:41 AM, Alexander Graf wrote: >>> Yeah, basically the variable flow goes: >>> >>> vl.c -> machine_opts -> machine_init() -> device properties -> >>> device_init() >> >> And the rationale is that machine_init() will do nothing other than use QOM >> primitives to create a set of expected devices and set up their properties >> such that a person (or management tool) could do everything that >> machine_init() is doing. > > So I think this flow is wrong (and indeed I didn't implement it that way in > my patches to add machine opts for kernel/initrd/dtb) -- the machine_init() > shouldn't have to care about this because we don't want to have to modify > a huge set of machine init functions every time we add an extra option > that only the boot loader cares about. > > I don't particularly care how we QOMify arm_boot (it's not exactly at > the top of my priority list demanding attention), I do care that (a) > we have a sensible user-facing interface [ie command line options] and > (b) vl.c can usefully just pass the information from those options > straight to the boot loader code. A QOM'ified arm_boot could get a "virtual" callback method to check the QemuOpts command line args. That way derived classes can decide what additional options to accept. An alternative would be to expect QOM properties of the same name as the command line parameters and fail if there isn't one of that name. Andreas
On 20 February 2012 19:51, Andreas Färber <afaerber@suse.de> wrote: > Am 20.02.2012 20:43, schrieb Peter Maydell: >> I don't particularly care how we QOMify arm_boot (it's not exactly at >> the top of my priority list demanding attention), I do care that (a) >> we have a sensible user-facing interface [ie command line options] and >> (b) vl.c can usefully just pass the information from those options >> straight to the boot loader code. > > A QOM'ified arm_boot could get a "virtual" callback method to check the > QemuOpts command line args. That way derived classes can decide what > additional options to accept. > > An alternative would be to expect QOM properties of the same name as the > command line parameters and fail if there isn't one of that name. You'd also need vl.c to be able to say "find me the first boot loader of any kind in the qom tree" so it could set the properties on the right thing. But really qomifying arm_boot doesn't excite me very much -- it doesn't really get us anything useful in the way that qomifying SysBus, for instance, does. So my preference is not to have to tie device tree support to it. -- PMM
On Tue, Feb 21, 2012 at 5:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 February 2012 19:51, Andreas Färber <afaerber@suse.de> wrote: >> Am 20.02.2012 20:43, schrieb Peter Maydell: >>> I don't particularly care how we QOMify arm_boot (it's not exactly at >>> the top of my priority list demanding attention), I do care that (a) >>> we have a sensible user-facing interface [ie command line options] and >>> (b) vl.c can usefully just pass the information from those options >>> straight to the boot loader code. So when I trialled this patch I used the -device argument to instantiate the bootloader on the command line: qemu-system-arm -M versatilepb --device arm_linux_loader,kernel_filename=kernel.foo,initrd=image.bar... With this approach I was able to add command line args (if you will) to arm_boot without touching vl.c, qemu_opts or any of the arm machine models. With changing only arm_boot.c you would get your dtb argument: qemu-system-arm -M versatilepb --device arm_linux_loader,kernel_filename=kernel.foo,initrd=image.bar,-dtb=blah.dtb Paul and Andreas reached the conclusion (correct me if I have misunderstood) that the machine model should have still have to explictly instantiate the bootloader "device" (whether that be a QOM device or arm_load_kernel() in its present form), but with the QOM approach apparently --property arguments should allow you to parameterise the bootloader directly, without vl.c or qemu-opts having to know about the arguments at all. Unfortunately --property is a long way from working so we are stuck with the status quo or your machine-opts approach until that happens. >> >> A QOM'ified arm_boot could get a "virtual" callback method to check the >> QemuOpts command line args. That way derived classes can decide what >> additional options to accept. >> >> An alternative would be to expect QOM properties of the same name as the >> command line parameters and fail if there isn't one of that name. > > You'd also need vl.c to be able to say "find me the first boot loader > of any kind in the qom tree" so it could set the properties on the > right thing. > > But really qomifying arm_boot doesn't excite me very much -- it > doesn't really get us anything useful in the way that qomifying > SysBus, for instance, does. So my preference is not to have to > tie device tree support to it. > > -- PMM
On 21 February 2012 09:15, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > On Tue, Feb 21, 2012 at 5:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 20 February 2012 19:51, Andreas Färber <afaerber@suse.de> wrote: >>> Am 20.02.2012 20:43, schrieb Peter Maydell: >>>> I don't particularly care how we QOMify arm_boot (it's not exactly at >>>> the top of my priority list demanding attention), I do care that (a) >>>> we have a sensible user-facing interface [ie command line options] and >>>> (b) vl.c can usefully just pass the information from those options >>>> straight to the boot loader code. > > So when I trialled this patch I used the -device argument to > instantiate the bootloader on the command line: > > qemu-system-arm -M versatilepb --device > arm_linux_loader,kernel_filename=kernel.foo,initrd=image.bar... > > With this approach I was able to add command line args (if you will) > to arm_boot without touching vl.c, qemu_opts or any of the arm machine > models. I think this is wrong, because it means the user has to know what the name of the arm_boot device is (as well as because it means that the machine model can't set the arm_boot properties that it needs to, as Paul and Andreas point out). vl.c should search for "some device, any device, which provides a kernel loading interface" and set the kernel/initrd/cmdline/dtb properties on it, so that the same kind of command line works on different machines even if they implement different boot loader devices, subclass arm_boot or whatever. As you say, the infrastructure to do it properly is kind of missing at the moment; that's why I don't want to get basic DTB tied up with QOMification. -- PMM
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_ = { + .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) 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,