Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-07 Thread Li Guang

Peter Crosthwaite wrote:

On Mon, Jan 6, 2014 at 10:13 PM, Paolo Bonzinipbonz...@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

 


   





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-06 Thread Peter Maydell
On 6 January 2014 07:56, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote:
 On Mon, Jan 6, 2014 at 5:41 PM, Peter Maydell peter.mayd...@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



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-06 Thread Paolo Bonzini
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



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-06 Thread Paolo Bonzini
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



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

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!



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

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!





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Peter Crosthwaite
On Mon, Jan 6, 2014 at 1:55 PM, Li Guang lig.f...@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!






Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

Peter Crosthwaite wrote:

On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!



 


   





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Peter Crosthwaite
On Mon, Jan 6, 2014 at 2:22 PM, Li Guang lig.f...@cn.fujitsu.com wrote:
 Peter Crosthwaite wrote:

 On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!













Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

Peter Crosthwaite wrote:

On Mon, Jan 6, 2014 at 2:22 PM, Li Guanglig.f...@cn.fujitsu.com  wrote:
   

Peter Crosthwaite wrote:
 

On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!




 



   



 


   





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

Li Guang wrote:

Peter Crosthwaite wrote:
On Mon, Jan 6, 2014 at 2:22 PM, Li Guanglig.f...@cn.fujitsu.com  
wrote:

Peter Crosthwaite wrote:
On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!




Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Peter Crosthwaite
On Mon, Jan 6, 2014 at 3:24 PM, Li Guang lig.f...@cn.fujitsu.com wrote:
 Li Guang wrote:

 Peter Crosthwaite wrote:

 On Mon, Jan 6, 2014 at 2:22 PM, Li Guanglig.f...@cn.fujitsu.com  wrote:

 Peter Crosthwaite wrote:

 On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Li Guang

Peter Crosthwaite wrote:

On Mon, Jan 6, 2014 at 3:24 PM, Li Guanglig.f...@cn.fujitsu.com  wrote:
   

Li Guang wrote:
 

Peter Crosthwaite wrote:
   

On Mon, Jan 6, 2014 at 2:22 PM, Li Guanglig.f...@cn.fujitsu.com   wrote:
 

Peter Crosthwaite wrote:
   

On Mon, Jan 6, 2014 at 1:55 PM, Li Guanglig.f...@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!


 


   





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Peter Maydell
On 2 January 2014 05:50, Peter Crosthwaite peter.crosthwa...@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



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-05 Thread Peter Crosthwaite
On Mon, Jan 6, 2014 at 5:41 PM, Peter Maydell peter.mayd...@linaro.org wrote:
 On 2 January 2014 05:50, Peter Crosthwaite peter.crosthwa...@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




Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-02 Thread Paolo Bonzini
Il 02/01/2014 06:50, Peter Crosthwaite ha scritto:
 On Thu, Jan 2, 2014 at 3:35 PM, Li Guang lig.f...@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.crosthwa...@xilinx.com
 
 Signed-off-by: Li Guang lig.f...@cn.fujitsu.com
 ---
 it can be used now for allwinner-a10, like:
 -device blob-loader,addr=0x4300,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 000..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


 
 




Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-02 Thread Peter Crosthwaite
On Thu, Jan 2, 2014 at 6:21 PM, Paolo Bonzini pbonz...@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.f...@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.crosthwa...@xilinx.com

 Signed-off-by: Li Guang lig.f...@cn.fujitsu.com
 ---
 it can be used now for allwinner-a10, like:
 -device blob-loader,addr=0x4300,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 000..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









Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-02 Thread Paolo Bonzini
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



Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-01 Thread Peter Crosthwaite
On Thu, Jan 2, 2014 at 3:35 PM, Li Guang lig.f...@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.crosthwa...@xilinx.com

 Signed-off-by: Li Guang lig.f...@cn.fujitsu.com
 ---
 it can be used now for allwinner-a10, like:
 -device blob-loader,addr=0x4300,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 000..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





Re: [Qemu-devel] [PATCH] hw/misc/blob-loader: add a generic blob loader

2014-01-01 Thread Li Guang

Peter Crosthwaite wrote:

On Thu, Jan 2, 2014 at 3:35 PM, Li Guanglig.f...@cn.fujitsu.com  wrote:
   

this blob loader will be used to load a specified
blob into a specified RAM address.

 

Suggested-by: Peter Crosthwaitepeter.crosthwa...@xilinx.com

   

Signed-off-by: Li Guanglig.f...@cn.fujitsu.com
---
it can be used now for allwinner-a10, like:
-device blob-loader,addr=0x4300,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 000..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