| Message ID | 1388640940-12782-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 2, 2014 at 3:35 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: > this blob loader will be used to load a specified > blob into a specified RAM address. > Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Li Guang <lig.fnst@cn.fujitsu.com> > --- > it can be used now for allwinner-a10, like: > "-device blob-loader,addr=0x43000000,file=/path/script.bin" > > reference: > http://linux-sunxi.org/Sunxi-tools > > script file address: > http://dl.dbank.com/c00aonvlmw > > Thanks to Peter Crosthwaite for the idea! > --- > default-configs/arm-softmmu.mak | 2 + > hw/misc/Makefile.objs | 2 + > hw/misc/blob-loader.c | 75 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 79 insertions(+), 0 deletions(-) > create mode 100644 hw/misc/blob-loader.c > > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak > index ce1d620..50c71a6 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -87,3 +87,5 @@ CONFIG_INTEGRATOR_DEBUG=y > CONFIG_ALLWINNER_A10_PIT=y > CONFIG_ALLWINNER_A10_PIC=y > CONFIG_ALLWINNER_A10=y > + > +CONFIG_BLOB_LOADER=y This shouldn't be arm specific. I would make the argument that the blob loader has global validity. > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > index f674365..df28288 100644 > --- a/hw/misc/Makefile.objs > +++ b/hw/misc/Makefile.objs > @@ -42,3 +42,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o > obj-$(CONFIG_ZYNQ) += zynq_slcr.o > > obj-$(CONFIG_PVPANIC) += pvpanic.o > + > +obj-$(CONFIG_BLOB_LOADER) += blob-loader.o > diff --git a/hw/misc/blob-loader.c b/hw/misc/blob-loader.c > new file mode 100644 > index 0000000..d7f1408 > --- /dev/null > +++ b/hw/misc/blob-loader.c > @@ -0,0 +1,75 @@ > +#include "hw/sysbus.h" > +#include "hw/devices.h" > +#include "hw/loader.h" > +#include "qemu/error-report.h" > + > +typedef struct BlobLoaderState { /*< private >/* > + DeviceState qdev; parent_obj; /*< public >/* > + uint32_t addr; uint64_t or hwaddr. Blob loading shouldn't be limited to 32 bit. > + char *file; > +} BlobLoaderState; > + > +#define TYPE_BLOB_LOADER "blob-loader" > +#define BLOB_LOADER(obj) OBJECT_CHECK(BlobLoaderState, (obj), TYPE_BLOB_LOADER) > + > +static Property blob_loader_props[] = { > + DEFINE_PROP_UINT32("addr", BlobLoaderState, addr, 0), > + DEFINE_PROP_STRING("file", BlobLoaderState, file), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void do_blob_load(BlobLoaderState *s) > +{ > + char *file_name; > + int file_size; > + > + if (s->file == NULL) { > + error_report("please spicify a file for blob loader\n"); > + return; Should you exit(1)? Better yet, return true for error and .. > + } > + file_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->file); > + if (file_name == NULL) { > + error_report("can't find %s\n", s->file); > + return; > + } > + file_size = get_image_size(file_name); > + if (file_size < 0) { > + error_report("can't get file size of %s\n", file_name); > + return; > + } > + if (load_image_targphys(file_name, s->addr, file_size) < 0) { > + error_report("can't load %s\n", file_name); > + return; > + } > +} > + > +static int blob_loader_init(DeviceState *dev) > +{ > + BlobLoaderState *s = BLOB_LOADER(dev); > + > + do_blob_load(s); exit(1) here. > + return 0; > +} > + > +static void blob_loader_class_init(ObjectClass *klass, void *data) s/klass/oc. > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->props = blob_loader_props; > + dc->desc = "blob loader"; > + dc->init = blob_loader_init; I'm wondering whether blob loading is actually a reset step not an init. Will doing it at init will play foul with VMSD, as your blob loader will trample non-reset state on machine restore? Regards, Peter > +} > + > +static TypeInfo blob_loader_info = { > + .name = TYPE_BLOB_LOADER, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(BlobLoaderState), > + .class_init = blob_loader_class_init, > +}; > + > +static void blob_loader_register_type(void) > +{ > + type_register_static(&blob_loader_info); > +} > + > +type_init(blob_loader_register_type) > -- > 1.7.2.5 > >
Peter Crosthwaite wrote: > On Thu, Jan 2, 2014 at 3:35 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: > >> this blob loader will be used to load a specified >> blob into a specified RAM address. >> >> > Suggested-by: Peter Crosthwaite<peter.crosthwaite@xilinx.com> > > >> Signed-off-by: Li Guang<lig.fnst@cn.fujitsu.com> >> --- >> it can be used now for allwinner-a10, like: >> "-device blob-loader,addr=0x43000000,file=/path/script.bin" >> >> reference: >> http://linux-sunxi.org/Sunxi-tools >> >> script file address: >> http://dl.dbank.com/c00aonvlmw >> >> Thanks to Peter Crosthwaite for the idea! >> --- >> default-configs/arm-softmmu.mak | 2 + >> hw/misc/Makefile.objs | 2 + >> hw/misc/blob-loader.c | 75 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 79 insertions(+), 0 deletions(-) >> create mode 100644 hw/misc/blob-loader.c >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index ce1d620..50c71a6 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -87,3 +87,5 @@ CONFIG_INTEGRATOR_DEBUG=y >> CONFIG_ALLWINNER_A10_PIT=y >> CONFIG_ALLWINNER_A10_PIC=y >> CONFIG_ALLWINNER_A10=y >> + >> +CONFIG_BLOB_LOADER=y >> > This shouldn't be arm specific. I would make the argument that the > blob loader has global validity. > > >> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs >> index f674365..df28288 100644 >> --- a/hw/misc/Makefile.objs >> +++ b/hw/misc/Makefile.objs >> @@ -42,3 +42,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o >> obj-$(CONFIG_ZYNQ) += zynq_slcr.o >> >> obj-$(CONFIG_PVPANIC) += pvpanic.o >> + >> +obj-$(CONFIG_BLOB_LOADER) += blob-loader.o >> diff --git a/hw/misc/blob-loader.c b/hw/misc/blob-loader.c >> new file mode 100644 >> index 0000000..d7f1408 >> --- /dev/null >> +++ b/hw/misc/blob-loader.c >> @@ -0,0 +1,75 @@ >> +#include "hw/sysbus.h" >> +#include "hw/devices.h" >> +#include "hw/loader.h" >> +#include "qemu/error-report.h" >> + >> +typedef struct BlobLoaderState { >> > /*< private>/* > > >> + DeviceState qdev; >> > parent_obj; > > /*< public>/* > > >> + uint32_t addr; >> > uint64_t or hwaddr. Blob loading shouldn't be limited to 32 bit. > > >> + char *file; >> +} BlobLoaderState; >> + >> +#define TYPE_BLOB_LOADER "blob-loader" >> +#define BLOB_LOADER(obj) OBJECT_CHECK(BlobLoaderState, (obj), TYPE_BLOB_LOADER) >> + >> +static Property blob_loader_props[] = { >> + DEFINE_PROP_UINT32("addr", BlobLoaderState, addr, 0), >> + DEFINE_PROP_STRING("file", BlobLoaderState, file), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void do_blob_load(BlobLoaderState *s) >> +{ >> + char *file_name; >> + int file_size; >> + >> + if (s->file == NULL) { >> + error_report("please spicify a file for blob loader\n"); >> + return; >> > Should you exit(1)? Better yet, return true for error and .. > > >> + } >> + file_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->file); >> + if (file_name == NULL) { >> + error_report("can't find %s\n", s->file); >> + return; >> + } >> + file_size = get_image_size(file_name); >> + if (file_size< 0) { >> + error_report("can't get file size of %s\n", file_name); >> + return; >> + } >> + if (load_image_targphys(file_name, s->addr, file_size)< 0) { >> + error_report("can't load %s\n", file_name); >> + return; >> + } >> +} >> + >> +static int blob_loader_init(DeviceState *dev) >> +{ >> + BlobLoaderState *s = BLOB_LOADER(dev); >> + >> + do_blob_load(s); >> > exit(1) here. > > >> + return 0; >> +} >> + >> +static void blob_loader_class_init(ObjectClass *klass, void *data) >> > s/klass/oc. > > will fix all above, >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->props = blob_loader_props; >> + dc->desc = "blob loader"; >> + dc->init = blob_loader_init; >> > I'm wondering whether blob loading is actually a reset step not an > init. Will doing it at init will play foul with VMSD, as your blob > loader will trample non-reset state on machine restore? > > > why will trample non-reset state on machine restore? Thanks! >> +} >> + >> +static TypeInfo blob_loader_info = { >> + .name = TYPE_BLOB_LOADER, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(BlobLoaderState), >> + .class_init = blob_loader_class_init, >> +}; >> + >> +static void blob_loader_register_type(void) >> +{ >> + type_register_static(&blob_loader_info); >> +} >> + >> +type_init(blob_loader_register_type) >> -- >> 1.7.2.5 >> >> >> > >
Il 02/01/2014 06:50, Peter Crosthwaite ha scritto: > On Thu, Jan 2, 2014 at 3:35 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: >> this blob loader will be used to load a specified >> blob into a specified RAM address. >> > > Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > >> Signed-off-by: Li Guang <lig.fnst@cn.fujitsu.com> >> --- >> it can be used now for allwinner-a10, like: >> "-device blob-loader,addr=0x43000000,file=/path/script.bin" >> >> reference: >> http://linux-sunxi.org/Sunxi-tools >> >> script file address: >> http://dl.dbank.com/c00aonvlmw >> >> Thanks to Peter Crosthwaite for the idea! >> --- >> default-configs/arm-softmmu.mak | 2 + >> hw/misc/Makefile.objs | 2 + >> hw/misc/blob-loader.c | 75 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 79 insertions(+), 0 deletions(-) >> create mode 100644 hw/misc/blob-loader.c >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index ce1d620..50c71a6 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -87,3 +87,5 @@ CONFIG_INTEGRATOR_DEBUG=y >> CONFIG_ALLWINNER_A10_PIT=y >> CONFIG_ALLWINNER_A10_PIC=y >> CONFIG_ALLWINNER_A10=y >> + >> +CONFIG_BLOB_LOADER=y > > This shouldn't be arm specific. I would make the argument that the > blob loader has global validity. > >> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs >> index f674365..df28288 100644 >> --- a/hw/misc/Makefile.objs >> +++ b/hw/misc/Makefile.objs >> @@ -42,3 +42,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o >> obj-$(CONFIG_ZYNQ) += zynq_slcr.o >> >> obj-$(CONFIG_PVPANIC) += pvpanic.o >> + >> +obj-$(CONFIG_BLOB_LOADER) += blob-loader.o >> diff --git a/hw/misc/blob-loader.c b/hw/misc/blob-loader.c >> new file mode 100644 >> index 0000000..d7f1408 >> --- /dev/null >> +++ b/hw/misc/blob-loader.c >> @@ -0,0 +1,75 @@ >> +#include "hw/sysbus.h" >> +#include "hw/devices.h" >> +#include "hw/loader.h" >> +#include "qemu/error-report.h" >> + >> +typedef struct BlobLoaderState { > > /*< private >/* > >> + DeviceState qdev; > > parent_obj; > > /*< public >/* > >> + uint32_t addr; > > uint64_t or hwaddr. Blob loading shouldn't be limited to 32 bit. > >> + char *file; >> +} BlobLoaderState; >> + >> +#define TYPE_BLOB_LOADER "blob-loader" >> +#define BLOB_LOADER(obj) OBJECT_CHECK(BlobLoaderState, (obj), TYPE_BLOB_LOADER) >> + >> +static Property blob_loader_props[] = { >> + DEFINE_PROP_UINT32("addr", BlobLoaderState, addr, 0), >> + DEFINE_PROP_STRING("file", BlobLoaderState, file), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void do_blob_load(BlobLoaderState *s) >> +{ >> + char *file_name; >> + int file_size; >> + >> + if (s->file == NULL) { >> + error_report("please spicify a file for blob loader\n"); >> + return; > > Should you exit(1)? Better yet, return true for error and .. > >> + } >> + file_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->file); >> + if (file_name == NULL) { >> + error_report("can't find %s\n", s->file); >> + return; >> + } >> + file_size = get_image_size(file_name); >> + if (file_size < 0) { >> + error_report("can't get file size of %s\n", file_name); >> + return; >> + } >> + if (load_image_targphys(file_name, s->addr, file_size) < 0) { >> + error_report("can't load %s\n", file_name); >> + return; >> + } >> +} >> + >> +static int blob_loader_init(DeviceState *dev) >> +{ >> + BlobLoaderState *s = BLOB_LOADER(dev); >> + >> + do_blob_load(s); > > exit(1) here. No, please use "realize" and avoid init. This way you can use an Error* to report the error. Also, the actual load_image_targphys call probably should be done in a reset handler, not at realize time. Paolo >> + return 0; >> +} >> + >> +static void blob_loader_class_init(ObjectClass *klass, void *data) > > s/klass/oc. Actually klass is quite commonly used. Paolo >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->props = blob_loader_props; >> + dc->desc = "blob loader"; >> + dc->init = blob_loader_init; > > I'm wondering whether blob loading is actually a reset step not an > init. Will doing it at init will play foul with VMSD, as your blob > loader will trample non-reset state on machine restore? > > Regards, > Peter > >> +} >> + >> +static TypeInfo blob_loader_info = { >> + .name = TYPE_BLOB_LOADER, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(BlobLoaderState), >> + .class_init = blob_loader_class_init, >> +}; >> + >> +static void blob_loader_register_type(void) >> +{ >> + type_register_static(&blob_loader_info); >> +} >> + >> +type_init(blob_loader_register_type) >> -- >> 1.7.2.5 >> >> > >
On Thu, Jan 2, 2014 at 6:21 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 02/01/2014 06:50, Peter Crosthwaite ha scritto: >> On Thu, Jan 2, 2014 at 3:35 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: >>> this blob loader will be used to load a specified >>> blob into a specified RAM address. >>> >> >> Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >>> Signed-off-by: Li Guang <lig.fnst@cn.fujitsu.com> >>> --- >>> it can be used now for allwinner-a10, like: >>> "-device blob-loader,addr=0x43000000,file=/path/script.bin" >>> >>> reference: >>> http://linux-sunxi.org/Sunxi-tools >>> >>> script file address: >>> http://dl.dbank.com/c00aonvlmw >>> >>> Thanks to Peter Crosthwaite for the idea! >>> --- >>> default-configs/arm-softmmu.mak | 2 + >>> hw/misc/Makefile.objs | 2 + >>> hw/misc/blob-loader.c | 75 +++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 79 insertions(+), 0 deletions(-) >>> create mode 100644 hw/misc/blob-loader.c >>> >>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >>> index ce1d620..50c71a6 100644 >>> --- a/default-configs/arm-softmmu.mak >>> +++ b/default-configs/arm-softmmu.mak >>> @@ -87,3 +87,5 @@ CONFIG_INTEGRATOR_DEBUG=y >>> CONFIG_ALLWINNER_A10_PIT=y >>> CONFIG_ALLWINNER_A10_PIC=y >>> CONFIG_ALLWINNER_A10=y >>> + >>> +CONFIG_BLOB_LOADER=y >> >> This shouldn't be arm specific. I would make the argument that the >> blob loader has global validity. >> >>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs >>> index f674365..df28288 100644 >>> --- a/hw/misc/Makefile.objs >>> +++ b/hw/misc/Makefile.objs >>> @@ -42,3 +42,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o >>> obj-$(CONFIG_ZYNQ) += zynq_slcr.o >>> >>> obj-$(CONFIG_PVPANIC) += pvpanic.o >>> + >>> +obj-$(CONFIG_BLOB_LOADER) += blob-loader.o >>> diff --git a/hw/misc/blob-loader.c b/hw/misc/blob-loader.c >>> new file mode 100644 >>> index 0000000..d7f1408 >>> --- /dev/null >>> +++ b/hw/misc/blob-loader.c >>> @@ -0,0 +1,75 @@ >>> +#include "hw/sysbus.h" >>> +#include "hw/devices.h" >>> +#include "hw/loader.h" >>> +#include "qemu/error-report.h" >>> + >>> +typedef struct BlobLoaderState { >> >> /*< private >/* >> >>> + DeviceState qdev; >> >> parent_obj; >> >> /*< public >/* >> >>> + uint32_t addr; >> >> uint64_t or hwaddr. Blob loading shouldn't be limited to 32 bit. >> >>> + char *file; >>> +} BlobLoaderState; >>> + >>> +#define TYPE_BLOB_LOADER "blob-loader" >>> +#define BLOB_LOADER(obj) OBJECT_CHECK(BlobLoaderState, (obj), TYPE_BLOB_LOADER) >>> + >>> +static Property blob_loader_props[] = { >>> + DEFINE_PROP_UINT32("addr", BlobLoaderState, addr, 0), >>> + DEFINE_PROP_STRING("file", BlobLoaderState, file), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> +static void do_blob_load(BlobLoaderState *s) >>> +{ >>> + char *file_name; >>> + int file_size; >>> + >>> + if (s->file == NULL) { >>> + error_report("please spicify a file for blob loader\n"); >>> + return; >> >> Should you exit(1)? Better yet, return true for error and .. >> >>> + } >>> + file_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->file); >>> + if (file_name == NULL) { >>> + error_report("can't find %s\n", s->file); >>> + return; >>> + } >>> + file_size = get_image_size(file_name); >>> + if (file_size < 0) { >>> + error_report("can't get file size of %s\n", file_name); >>> + return; >>> + } >>> + if (load_image_targphys(file_name, s->addr, file_size) < 0) { >>> + error_report("can't load %s\n", file_name); >>> + return; >>> + } >>> +} >>> + >>> +static int blob_loader_init(DeviceState *dev) >>> +{ >>> + BlobLoaderState *s = BLOB_LOADER(dev); >>> + >>> + do_blob_load(s); >> >> exit(1) here. > > No, please use "realize" and avoid init. This way you can use an Error* > to report the error. > > Also, the actual load_image_targphys call probably should be done in a > reset handler, not at realize time. > Ok I think that settles it. The actual blobbing needs to happen at reset time. Perhaps the correct approach is to do as much as possible (file-path / address sanitsation etc) at realize time, then only the actual blob load happens at reset. Going on what Paolo said, I think for this device ::init is actually a nop. > Paolo > >>> + return 0; >>> +} >>> + >>> +static void blob_loader_class_init(ObjectClass *klass, void *data) >> >> s/klass/oc. > > Actually klass is quite commonly used. > Yeh, I remember being told off for that one though :) Regards, Peter > Paolo > >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + dc->props = blob_loader_props; >>> + dc->desc = "blob loader"; >>> + dc->init = blob_loader_init; >> >> I'm wondering whether blob loading is actually a reset step not an >> init. Will doing it at init will play foul with VMSD, as your blob >> loader will trample non-reset state on machine restore? >> >> Regards, >> Peter >> >>> +} >>> + >>> +static TypeInfo blob_loader_info = { >>> + .name = TYPE_BLOB_LOADER, >>> + .parent = TYPE_DEVICE, >>> + .instance_size = sizeof(BlobLoaderState), >>> + .class_init = blob_loader_class_init, >>> +}; >>> + >>> +static void blob_loader_register_type(void) >>> +{ >>> + type_register_static(&blob_loader_info); >>> +} >>> + >>> +type_init(blob_loader_register_type) >>> -- >>> 1.7.2.5 >>> >>> >> >> > >
Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >> > No, please use "realize" and avoid init. This way you can use an Error* >> > to report the error. >> > >> > Also, the actual load_image_targphys call probably should be done in a >> > reset handler, not at realize time. >> > > Ok I think that settles it. The actual blobbing needs to happen at > reset time. Perhaps the correct approach is to do as much as possible > (file-path / address sanitsation etc) at realize time, then only the > actual blob load happens at reset. Going on what Paolo said, I think > for this device ::init is actually a nop. Yeah, also because init is in fact a legacy interface to realize. Paolo
Paolo Bonzini wrote: > Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: > >>>> No, please use "realize" and avoid init. This way you can use an Error* >>>> to report the error. >>>> >>>> Also, the actual load_image_targphys call probably should be done in a >>>> reset handler, not at realize time. >>>> >>>> >> Ok I think that settles it. The actual blobbing needs to happen at >> reset time. Perhaps the correct approach is to do as much as possible >> (file-path / address sanitsation etc) at realize time, then only the >> actual blob load happens at reset. Going on what Paolo said, I think >> for this device ::init is actually a nop. >> > Yeah, also because init is in fact a legacy interface to realize. > > Paolo > > Ok, thanks!
Li Guang wrote: > Paolo Bonzini wrote: >> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>> No, please use "realize" and avoid init. This way you can use an >>>>> Error* >>>>> to report the error. >>>>> >>>>> Also, the actual load_image_targphys call probably should be done >>>>> in a >>>>> reset handler, not at realize time. >>>>> >>> Ok I think that settles it. The actual blobbing needs to happen at >>> reset time. Perhaps the correct approach is to do as much as possible >>> (file-path / address sanitsation etc) at realize time, then only the >>> actual blob load happens at reset. Going on what Paolo said, I think >>> for this device ::init is actually a nop. >> Yeah, also because init is in fact a legacy interface to realize. >> >> Paolo >> > Ok, thanks! > > Sorry, seems load blob at reset handler can't do the right job, while the same action can play very well at init or realize. any suggestion to figure out this problem? Thanks!
On Mon, Jan 6, 2014 at 1:55 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: > Li Guang wrote: >> >> Paolo Bonzini wrote: >>> >>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>>> >>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>> Error* >>>>>> to report the error. >>>>>> >>>>>> Also, the actual load_image_targphys call probably should be done in a >>>>>> reset handler, not at realize time. >>>>>> >>>> Ok I think that settles it. The actual blobbing needs to happen at >>>> reset time. Perhaps the correct approach is to do as much as possible >>>> (file-path / address sanitsation etc) at realize time, then only the >>>> actual blob load happens at reset. Going on what Paolo said, I think >>>> for this device ::init is actually a nop. >>> >>> Yeah, also because init is in fact a legacy interface to realize. >>> >>> Paolo >>> >> Ok, thanks! >> >> > Sorry, seems load blob at reset handler can't do the right job, > while the same action can play very well at init or realize. What's the exact problem with the reset idea? Regards, Peter > any suggestion to figure out this problem? > > Thanks! > > >
Peter Crosthwaite wrote: > On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: > >> Li Guang wrote: >> >>> Paolo Bonzini wrote: >>> >>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>> >>>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>>> Error* >>>>>>> to report the error. >>>>>>> >>>>>>> Also, the actual load_image_targphys call probably should be done in a >>>>>>> reset handler, not at realize time. >>>>>>> >>>>>>> >>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>> reset time. Perhaps the correct approach is to do as much as possible >>>>> (file-path / address sanitsation etc) at realize time, then only the >>>>> actual blob load happens at reset. Going on what Paolo said, I think >>>>> for this device ::init is actually a nop. >>>>> >>>> Yeah, also because init is in fact a legacy interface to realize. >>>> >>>> Paolo >>>> >>>> >>> Ok, thanks! >>> >>> >>> >> Sorry, seems load blob at reset handler can't do the right job, >> while the same action can play very well at init or realize. >> > What's the exact problem with the reset idea? > > > code snippet: static void blob_loader_reset(DeviceState *dev) { BlobLoaderState *s = BLOB_LOADER(dev); if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE) < 0) { error_report("can't load %s\n", s->file); exit(1); } } if it is device reset handler, no result, kernel doesn't find and parse blob, if it is called at device realize phase, it works. Thanks! >> any suggestion to figure out this problem? >> >> Thanks! >> >> >> >> > >
On Mon, Jan 6, 2014 at 2:22 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: > Peter Crosthwaite wrote: >> >> On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: >> >>> >>> Li Guang wrote: >>> >>>> >>>> Paolo Bonzini wrote: >>>> >>>>> >>>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>> >>>>>>>> >>>>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>>>> Error* >>>>>>>> to report the error. >>>>>>>> >>>>>>>> Also, the actual load_image_targphys call probably should be done in >>>>>>>> a >>>>>>>> reset handler, not at realize time. >>>>>>>> >>>>>>>> >>>>>> >>>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>>> reset time. Perhaps the correct approach is to do as much as possible >>>>>> (file-path / address sanitsation etc) at realize time, then only the >>>>>> actual blob load happens at reset. Going on what Paolo said, I think >>>>>> for this device ::init is actually a nop. >>>>>> >>>>> >>>>> Yeah, also because init is in fact a legacy interface to realize. >>>>> >>>>> Paolo >>>>> >>>>> >>>> >>>> Ok, thanks! >>>> >>>> >>>> >>> >>> Sorry, seems load blob at reset handler can't do the right job, >>> while the same action can play very well at init or realize. >>> >> >> What's the exact problem with the reset idea? >> >> >> > > code snippet: > static void blob_loader_reset(DeviceState *dev) > { > BlobLoaderState *s = BLOB_LOADER(dev); > > if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE) < 0) { > error_report("can't load %s\n", s->file); > exit(1); > } > } > > if it is device reset handler, no result, > kernel doesn't find and parse blob, > if it is called at device realize phase, > it works. > Need to figure out why I think. There's no fundamental problem here AFAIK. It must be a bug somewhere. > Thanks! > > >>> any suggestion to figure out this problem? Run QEMU in GDB and break on your new reset function to see if it is ever called. If not have a look into QOM/qdev to see how resets work and how that plays with -device args. If yes, have a look into load_image_targphys and see why that's not working. Regards, Peter >>> >>> Thanks! >>> >>> >>> >>> >> >> >> > > >
Peter Crosthwaite wrote: > On Mon, Jan 6, 2014 at 2:22 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: > >> Peter Crosthwaite wrote: >> >>> On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: >>> >>> >>>> Li Guang wrote: >>>> >>>> >>>>> Paolo Bonzini wrote: >>>>> >>>>> >>>>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>>> >>>>>> >>>>>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>>>>> Error* >>>>>>>>> to report the error. >>>>>>>>> >>>>>>>>> Also, the actual load_image_targphys call probably should be done in >>>>>>>>> a >>>>>>>>> reset handler, not at realize time. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>>>> reset time. Perhaps the correct approach is to do as much as possible >>>>>>> (file-path / address sanitsation etc) at realize time, then only the >>>>>>> actual blob load happens at reset. Going on what Paolo said, I think >>>>>>> for this device ::init is actually a nop. >>>>>>> >>>>>>> >>>>>> Yeah, also because init is in fact a legacy interface to realize. >>>>>> >>>>>> Paolo >>>>>> >>>>>> >>>>>> >>>>> Ok, thanks! >>>>> >>>>> >>>>> >>>>> >>>> Sorry, seems load blob at reset handler can't do the right job, >>>> while the same action can play very well at init or realize. >>>> >>>> >>> What's the exact problem with the reset idea? >>> >>> >>> >>> >> code snippet: >> static void blob_loader_reset(DeviceState *dev) >> { >> BlobLoaderState *s = BLOB_LOADER(dev); >> >> if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE)< 0) { >> error_report("can't load %s\n", s->file); >> exit(1); >> } >> } >> >> if it is device reset handler, no result, >> kernel doesn't find and parse blob, >> if it is called at device realize phase, >> it works. >> >> > Need to figure out why I think. There's no fundamental problem here > AFAIK. It must be a bug somewhere. > > >> Thanks! >> >> >> >>>> any suggestion to figure out this problem? >>>> > Run QEMU in GDB and break on your new reset function to see if it is > ever called. If not have a look into QOM/qdev to see how resets work > and how that plays with -device args. If yes, have a look into > load_image_targphys and see why that's not working. > > > reset handler definitely be called, and load_image_targphys has no problem, I guess it may be impacted by other RAM related codes, just can't figure out it quickly. Thanks! >>>> Thanks! >>>> >>>> >>>> >>>> >>>> >>> >>> >>> >> >> >> > >
Li Guang wrote: > Peter Crosthwaite wrote: >> On Mon, Jan 6, 2014 at 2:22 PM, Li Guang<lig.fnst@cn.fujitsu.com> >> wrote: >>> Peter Crosthwaite wrote: >>>> On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> >>>> wrote: >>>> >>>>> Li Guang wrote: >>>>> >>>>>> Paolo Bonzini wrote: >>>>>> >>>>>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>>>> >>>>>>>>>> No, please use "realize" and avoid init. This way you can >>>>>>>>>> use an >>>>>>>>>> Error* >>>>>>>>>> to report the error. >>>>>>>>>> >>>>>>>>>> Also, the actual load_image_targphys call probably should be >>>>>>>>>> done in >>>>>>>>>> a >>>>>>>>>> reset handler, not at realize time. >>>>>>>>>> >>>>>>>>>> >>>>>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>>>>> reset time. Perhaps the correct approach is to do as much as >>>>>>>> possible >>>>>>>> (file-path / address sanitsation etc) at realize time, then >>>>>>>> only the >>>>>>>> actual blob load happens at reset. Going on what Paolo said, I >>>>>>>> think >>>>>>>> for this device ::init is actually a nop. >>>>>>>> >>>>>>> Yeah, also because init is in fact a legacy interface to realize. >>>>>>> >>>>>>> Paolo >>>>>>> >>>>>>> >>>>>> Ok, thanks! >>>>>> >>>>>> >>>>>> >>>>> Sorry, seems load blob at reset handler can't do the right job, >>>>> while the same action can play very well at init or realize. >>>>> >>>> What's the exact problem with the reset idea? >>>> >>>> >>>> >>> code snippet: >>> static void blob_loader_reset(DeviceState *dev) >>> { >>> BlobLoaderState *s = BLOB_LOADER(dev); >>> >>> if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE)< 0) { >>> error_report("can't load %s\n", s->file); >>> exit(1); >>> } >>> } >>> >>> if it is device reset handler, no result, >>> kernel doesn't find and parse blob, >>> if it is called at device realize phase, >>> it works. >>> >> Need to figure out why I think. There's no fundamental problem here >> AFAIK. It must be a bug somewhere. >> >>> Thanks! >>> >>> >>>>> any suggestion to figure out this problem? >> Run QEMU in GDB and break on your new reset function to see if it is >> ever called. If not have a look into QOM/qdev to see how resets work >> and how that plays with -device args. If yes, have a look into >> load_image_targphys and see why that's not working. >> >> > > reset handler definitely be called, and > load_image_targphys has no problem, > I guess it may be impacted by other RAM related codes, > just can't figure out it quickly. > Ok, I found the problem finally, load_image_targphys depends on rom_reset to take effect, if it's located at reset handler, rom_reset is called before it, it surely failed to do the right job. so, I have to write my own code to load blob into RAM. Thanks!
On Mon, Jan 6, 2014 at 3:24 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote: > Li Guang wrote: >> >> Peter Crosthwaite wrote: >>> >>> On Mon, Jan 6, 2014 at 2:22 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: >>>> >>>> Peter Crosthwaite wrote: >>>>> >>>>> On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> >>>>> wrote: >>>>> >>>>>> Li Guang wrote: >>>>>> >>>>>>> Paolo Bonzini wrote: >>>>>>> >>>>>>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>>>>> >>>>>>>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>>>>>>> Error* >>>>>>>>>>> to report the error. >>>>>>>>>>> >>>>>>>>>>> Also, the actual load_image_targphys call probably should be done >>>>>>>>>>> in >>>>>>>>>>> a >>>>>>>>>>> reset handler, not at realize time. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>>>>>> reset time. Perhaps the correct approach is to do as much as >>>>>>>>> possible >>>>>>>>> (file-path / address sanitsation etc) at realize time, then only >>>>>>>>> the >>>>>>>>> actual blob load happens at reset. Going on what Paolo said, I >>>>>>>>> think >>>>>>>>> for this device ::init is actually a nop. >>>>>>>>> >>>>>>>> Yeah, also because init is in fact a legacy interface to realize. >>>>>>>> >>>>>>>> Paolo >>>>>>>> >>>>>>>> >>>>>>> Ok, thanks! >>>>>>> >>>>>>> >>>>>>> >>>>>> Sorry, seems load blob at reset handler can't do the right job, >>>>>> while the same action can play very well at init or realize. >>>>>> >>>>> What's the exact problem with the reset idea? >>>>> >>>>> >>>>> >>>> code snippet: >>>> static void blob_loader_reset(DeviceState *dev) >>>> { >>>> BlobLoaderState *s = BLOB_LOADER(dev); >>>> >>>> if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE)< 0) { >>>> error_report("can't load %s\n", s->file); >>>> exit(1); >>>> } >>>> } >>>> >>>> if it is device reset handler, no result, >>>> kernel doesn't find and parse blob, >>>> if it is called at device realize phase, >>>> it works. >>>> >>> Need to figure out why I think. There's no fundamental problem here >>> AFAIK. It must be a bug somewhere. >>> >>>> Thanks! >>>> >>>> >>>>>> any suggestion to figure out this problem? >>> >>> Run QEMU in GDB and break on your new reset function to see if it is >>> ever called. If not have a look into QOM/qdev to see how resets work >>> and how that plays with -device args. If yes, have a look into >>> load_image_targphys and see why that's not working. >>> >>> >> >> reset handler definitely be called, and >> load_image_targphys has no problem, >> I guess it may be impacted by other RAM related codes, >> just can't figure out it quickly. >> > > Ok, I found the problem finally, > load_image_targphys depends on rom_reset to take effect, > if it's located at reset handler, rom_reset is called before it, > it surely failed to do the right job. > > so, I have to write my own code to load blob into RAM. > dma_memory_write() work? Regards, Peter > Thanks! > >
Peter Crosthwaite wrote: > On Mon, Jan 6, 2014 at 3:24 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: > >> Li Guang wrote: >> >>> Peter Crosthwaite wrote: >>> >>>> On Mon, Jan 6, 2014 at 2:22 PM, Li Guang<lig.fnst@cn.fujitsu.com> wrote: >>>> >>>>> Peter Crosthwaite wrote: >>>>> >>>>>> On Mon, Jan 6, 2014 at 1:55 PM, Li Guang<lig.fnst@cn.fujitsu.com> >>>>>> wrote: >>>>>> >>>>>> >>>>>>> Li Guang wrote: >>>>>>> >>>>>>> >>>>>>>> Paolo Bonzini wrote: >>>>>>>> >>>>>>>> >>>>>>>>> Il 02/01/2014 11:51, Peter Crosthwaite ha scritto: >>>>>>>>> >>>>>>>>> >>>>>>>>>>>> No, please use "realize" and avoid init. This way you can use an >>>>>>>>>>>> Error* >>>>>>>>>>>> to report the error. >>>>>>>>>>>> >>>>>>>>>>>> Also, the actual load_image_targphys call probably should be done >>>>>>>>>>>> in >>>>>>>>>>>> a >>>>>>>>>>>> reset handler, not at realize time. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> Ok I think that settles it. The actual blobbing needs to happen at >>>>>>>>>> reset time. Perhaps the correct approach is to do as much as >>>>>>>>>> possible >>>>>>>>>> (file-path / address sanitsation etc) at realize time, then only >>>>>>>>>> the >>>>>>>>>> actual blob load happens at reset. Going on what Paolo said, I >>>>>>>>>> think >>>>>>>>>> for this device ::init is actually a nop. >>>>>>>>>> >>>>>>>>>> >>>>>>>>> Yeah, also because init is in fact a legacy interface to realize. >>>>>>>>> >>>>>>>>> Paolo >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> Ok, thanks! >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> Sorry, seems load blob at reset handler can't do the right job, >>>>>>> while the same action can play very well at init or realize. >>>>>>> >>>>>>> >>>>>> What's the exact problem with the reset idea? >>>>>> >>>>>> >>>>>> >>>>>> >>>>> code snippet: >>>>> static void blob_loader_reset(DeviceState *dev) >>>>> { >>>>> BlobLoaderState *s = BLOB_LOADER(dev); >>>>> >>>>> if (load_image_targphys(s->file, s->hwaddr, MAX_BLOB_SIZE)< 0) { >>>>> error_report("can't load %s\n", s->file); >>>>> exit(1); >>>>> } >>>>> } >>>>> >>>>> if it is device reset handler, no result, >>>>> kernel doesn't find and parse blob, >>>>> if it is called at device realize phase, >>>>> it works. >>>>> >>>>> >>>> Need to figure out why I think. There's no fundamental problem here >>>> AFAIK. It must be a bug somewhere. >>>> >>>> >>>>> Thanks! >>>>> >>>>> >>>>> >>>>>>> any suggestion to figure out this problem? >>>>>>> >>>> Run QEMU in GDB and break on your new reset function to see if it is >>>> ever called. If not have a look into QOM/qdev to see how resets work >>>> and how that plays with -device args. If yes, have a look into >>>> load_image_targphys and see why that's not working. >>>> >>>> >>>> >>> reset handler definitely be called, and >>> load_image_targphys has no problem, >>> I guess it may be impacted by other RAM related codes, >>> just can't figure out it quickly. >>> >>> >> Ok, I found the problem finally, >> load_image_targphys depends on rom_reset to take effect, >> if it's located at reset handler, rom_reset is called before it, >> it surely failed to do the right job. >> >> so, I have to write my own code to load blob into RAM. >> >> > dma_memory_write() work? > > > what about cpu_physical_memory_write_rom ? >> Thanks! >> >> >> > >
On 2 January 2014 05:50, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > I'm wondering whether blob loading is actually a reset step not an > init. load_image_targphys() uses the rom blob loader code from hw/core/loader.c, which already supports reloading the blob on machine reset if necessary. That raises some more general design questions: * how is this expected to interact with rom blob loading, -machine firmware=, etc? What are the guidelines for when to use one or the other? Should this be rolled into the rom blob code rather than being a bolt-on extra? * I'm unconvinced by the general approach of having a device with an address property * would it be useful if the rom blob loading was extended/reworked so that you could load a rom blob into a MemoryRegion? We want that for the SPARC TCX ROM blobs as well; maybe it would help here too, though I haven't thought much about it. thanks -- PMM
On Mon, Jan 6, 2014 at 5:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 2 January 2014 05:50, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> I'm wondering whether blob loading is actually a reset step not an >> init. > > load_image_targphys() uses the rom blob loader code from hw/core/loader.c, > which already supports reloading the blob on machine reset if necessary. > > That raises some more general design questions: > * how is this expected to interact with rom blob loading, -machine firmware=, > etc? I guess it's not. The behavior of "-machine firmware=" is machine specific, so a machine specific undefined behavior will occur if you have overlap issues. The two should not in any way inhibit each other. They are independent mechanisms. > What are the guidelines for when to use one or the other? "-machine firmware=" if you want to load a firmware blob in a board specific way. This if you want to place a blob in memory at an arbitrary location on reset. > Should this be rolled into the rom blob code rather than being a bolt-on extra? The QOMification is nice. And bolt-on'ness makes it repeatable so you can do whatever and as many blobs as you want. > * I'm unconvinced by the general approach of having a device with an > address property That is what makes it generically reusable. > * would it be useful if the rom blob loading was extended/reworked so that > you could load a rom blob into a MemoryRegion? Yes. Edgar's work on per-master AS may be related and applicable. Regards, Peter > We want that for > the SPARC TCX ROM blobs as well; maybe it would help here too, > though I haven't thought much about it. > > thanks > -- PMM >
On 6 January 2014 07:56, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, Jan 6, 2014 at 5:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> That raises some more general design questions: >> * how is this expected to interact with rom blob loading, -machine firmware=, >> etc? > > I guess it's not. The behavior of "-machine firmware=" is machine > specific, so a machine specific undefined behavior will occur if you > have overlap issues. The two should not in any way inhibit each other. > They are independent mechanisms. > >> What are the guidelines for when to use one or the other? > > "-machine firmware=" if you want to load a firmware blob in a board > specific way. This if you want to place a blob in memory at an > arbitrary location on reset. I guess I don't much like the way we're solving a problem by adding yet another mechanism here rather than trying to produce a design for blob loading that works for all the use cases we have. thanks -- PMM
Il 06/01/2014 06:36, Li Guang ha scritto: >> dma_memory_write() work? > > what about cpu_physical_memory_write_rom ? Sorry, I missed that load_image_targphys is already doing the right thing on reset. Paolo
Il 06/01/2014 08:56, Peter Crosthwaite ha scritto: > > What are the guidelines for when to use one or the other? > > "-machine firmware=" if you want to load a firmware blob in a board > specific way. This if you want to place a blob in memory at an > arbitrary location on reset. "-machine firmware=" is also a pretty bad design because it's not extensible and doesn't apply to most boards. We really should get per-board -machine options, so that you can have a less generic name than "firmware". Paolo
On Mon, Jan 6, 2014 at 10:13 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 06/01/2014 08:56, Peter Crosthwaite ha scritto: >> > What are the guidelines for when to use one or the other? >> >> "-machine firmware=" if you want to load a firmware blob in a board >> specific way. This if you want to place a blob in memory at an >> arbitrary location on reset. > > "-machine firmware=" is also a pretty bad design because it's not > extensible and doesn't apply to most boards. We really should get > per-board -machine options, so that you can have a less generic name > than "firmware". > Then we have even more divergence in boot flow between boards. It's already a bit of a Zoo out there when it comes to bootloaders. We expressed dislike for the Allwinner/FEX board specific bootloader due it its mainline Linux non-acceptance. This generic solution facilitates this case among many others and the reason its able to achieve that goal is it has no reliance on software policy. OTOH if we need to do everything board specific then we need to start deciding software policy for each and every board. For allwinner, FEX was a nack, which pretty much hangs out users of that linux kernel to dry. Looking at QEMU ARM, we are in a position now where you can only boot systems two ways: 1: Exactly as real HW (have to use your boards bootrom, BIOS, storage media etc). 2. ARM Linux exactly to the letter of the mainline boot process. With generic tools like this device, you at least let the users do a few flexible things for boots that do not fit these two limited use cases. And the device is completely unobtrusive on existing code. If developers in the future come along with their weird and wonderful board specific bootflows that we don't like we can now tell them to use the generic blob loader for their bits and pieces and everybody wins. If anything, the boards implementing -machine firware="" should implement it by layer ontop of this device. Regards, Peter > Paolo >
Peter Crosthwaite wrote: > On Mon, Jan 6, 2014 at 10:13 PM, Paolo Bonzini<pbonzini@redhat.com> wrote: > >> Il 06/01/2014 08:56, Peter Crosthwaite ha scritto: >> >>>> What are the guidelines for when to use one or the other? >>>> >>> "-machine firmware=" if you want to load a firmware blob in a board >>> specific way. This if you want to place a blob in memory at an >>> arbitrary location on reset. >>> >> "-machine firmware=" is also a pretty bad design because it's not >> extensible and doesn't apply to most boards. We really should get >> per-board -machine options, so that you can have a less generic name >> than "firmware". >> >> > Then we have even more divergence in boot flow between boards. It's > already a bit of a Zoo out there when it comes to bootloaders. > > We expressed dislike for the Allwinner/FEX board specific bootloader > due it its mainline Linux non-acceptance. This generic solution > facilitates this case among many others and the reason its able to > achieve that goal is it has no reliance on software policy. OTOH if we > need to do everything board specific then we need to start deciding > software policy for each and every board. For allwinner, FEX was a > nack, which pretty much hangs out users of that linux kernel to dry. > > Looking at QEMU ARM, we are in a position now where you can only boot > systems two ways: > > 1: Exactly as real HW (have to use your boards bootrom, BIOS, storage > media etc). > 2. ARM Linux exactly to the letter of the mainline boot process. > > With generic tools like this device, you at least let the users do a > few flexible things for boots that do not fit these two limited use > cases. And the device is completely unobtrusive on existing code. If > developers in the future come along with their weird and wonderful > board specific bootflows that we don't like we can now tell them to > use the generic blob loader for their bits and pieces and everybody > wins. > > If anything, the boards implementing -machine firware="" should > implement it by layer ontop of this device. > > > agree, further more, "-machine firmware=" is really not generic, we may have to deprecate it. because if use it, every boards should do specific things for it. Thanks! > >> Paolo >> >> > >
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index ce1d620..50c71a6 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -87,3 +87,5 @@ CONFIG_INTEGRATOR_DEBUG=y CONFIG_ALLWINNER_A10_PIT=y CONFIG_ALLWINNER_A10_PIC=y CONFIG_ALLWINNER_A10=y + +CONFIG_BLOB_LOADER=y diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index f674365..df28288 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -42,3 +42,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o obj-$(CONFIG_ZYNQ) += zynq_slcr.o obj-$(CONFIG_PVPANIC) += pvpanic.o + +obj-$(CONFIG_BLOB_LOADER) += blob-loader.o diff --git a/hw/misc/blob-loader.c b/hw/misc/blob-loader.c new file mode 100644 index 0000000..d7f1408 --- /dev/null +++ b/hw/misc/blob-loader.c @@ -0,0 +1,75 @@ +#include "hw/sysbus.h" +#include "hw/devices.h" +#include "hw/loader.h" +#include "qemu/error-report.h" + +typedef struct BlobLoaderState { + DeviceState qdev; + uint32_t addr; + char *file; +} BlobLoaderState; + +#define TYPE_BLOB_LOADER "blob-loader" +#define BLOB_LOADER(obj) OBJECT_CHECK(BlobLoaderState, (obj), TYPE_BLOB_LOADER) + +static Property blob_loader_props[] = { + DEFINE_PROP_UINT32("addr", BlobLoaderState, addr, 0), + DEFINE_PROP_STRING("file", BlobLoaderState, file), + DEFINE_PROP_END_OF_LIST(), +}; + +static void do_blob_load(BlobLoaderState *s) +{ + char *file_name; + int file_size; + + if (s->file == NULL) { + error_report("please spicify a file for blob loader\n"); + return; + } + file_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->file); + if (file_name == NULL) { + error_report("can't find %s\n", s->file); + return; + } + file_size = get_image_size(file_name); + if (file_size < 0) { + error_report("can't get file size of %s\n", file_name); + return; + } + if (load_image_targphys(file_name, s->addr, file_size) < 0) { + error_report("can't load %s\n", file_name); + return; + } +} + +static int blob_loader_init(DeviceState *dev) +{ + BlobLoaderState *s = BLOB_LOADER(dev); + + do_blob_load(s); + return 0; +} + +static void blob_loader_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->props = blob_loader_props; + dc->desc = "blob loader"; + dc->init = blob_loader_init; +} + +static TypeInfo blob_loader_info = { + .name = TYPE_BLOB_LOADER, + .parent = TYPE_DEVICE, + .instance_size = sizeof(BlobLoaderState), + .class_init = blob_loader_class_init, +}; + +static void blob_loader_register_type(void) +{ + type_register_static(&blob_loader_info); +} + +type_init(blob_loader_register_type)
this blob loader will be used to load a specified blob into a specified RAM address. Signed-off-by: Li Guang <lig.fnst@cn.fujitsu.com> --- it can be used now for allwinner-a10, like: "-device blob-loader,addr=0x43000000,file=/path/script.bin" reference: http://linux-sunxi.org/Sunxi-tools script file address: http://dl.dbank.com/c00aonvlmw Thanks to Peter Crosthwaite for the idea! --- default-configs/arm-softmmu.mak | 2 + hw/misc/Makefile.objs | 2 + hw/misc/blob-loader.c | 75 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 0 deletions(-) create mode 100644 hw/misc/blob-loader.c