Re: [Qemu-devel] [PATCH v4 1/2] Implement .hex file loader

[Su Hang]

Sorry I didn't reply in detai on last weekend.
Here are some points I want to discuss.

> parse_hex_blob() must handle input files that touch large ranges of
> memory.  At the moment it assumes bin_buf will be large enough for the
> memory regions described by the input file.  Since Intel HEX files
> support 32-bit addressing, that means processing a file in this way
> could require 4 GB of RAM!  Three solutions:
> 1. Reject files that have large gaps in their address ranges.
> 2. Return a list of data blobs, each with its own start address.
> 3. Set up the ROM structs directly within parse_hex_blob() instead of
> returning a linear buffer.

I think solution 3 is more natural and should be the best in this case.
I will read the source code to learn how to set up ROM structs correctly,
then try to apply solution 3.

> Have you considered using fscanf() instead?  It removes the need for
> HexLine, hex_buf, and the character parsing loop:
>
>   if (fscanf(fp, ":%02hhx%04hx%02hhx", &byte_count, &address,
> &record_type) != 3) {

fscanf() indeed works, but I worry it will be much slower than handle it
manually, especially when you mentioned the input file can be as large as 4GB.

> Why is the size hex_blob_size * 2?
I must confess I did it in a rough way, in order to quickly write
a runnable prototype, I didn't take safety into consideration.
Sorry, I will keep safety point in mind in my future work.
Why I set size = hex_blob_size * 2, because I assumed it would
satisfy bin_buf requirement.(Of course, deliberately constructed
malicious user input can easily break this hypothesis.)

> > +            case START_SEG_ADDR_RECORD:
> > +                /* TODO */
>
> The function should return an error if an unsupported record is encountered.

I read DAPLink code(and few others hex to bin converter's source code),
they just simply ignore this record type and do nothing.
The hex file generated by official online compiler do contain this kind of
record type.

I vimdiff the hex file and bin file(generated by
`arm-none-eabi-objcopy -I ihex microbit.hex -O binary microbit.bin`,
then use '%!xxd' in vim),
I noticed 'arm-none-eabi-objcopy' didn't generate any code corresponding to
START_SEG_ADDR_RECORD record type.

1) So should I return an error for 'Start Segment Address' and
   'Start Linear Address' type?

2) Or maybe I should insert some ARM assembly like
'''
mov     r0, #0      @ Set r0 to 0.
b       target      @ Jump forward to 'target'.
''' to bin_buf?

3) Or should I just ignore this two types like DAPLink and
   'arm-none-eabi-objcopy' do?

> > +            line.address = bswap16(line.address);
>
> This assumes the host CPU is little-endian.  Please use be16_to_cpu()
> instead so it works on big-endian host CPUs too.

I have tried to use be16_to_cpu() the first time you suggested it to me,
but qemu crashed...
I have read other places call this function to verify I use it correctly,
but it did crash(that's why I choose bswap16()), It's strange...
I'll report more detail about this case tomorrow.


Thank you for your patience with my ignorance!

With sincerest gratitude,
Su Hang


> -----Original Messages-----
> From: "Stefan Hajnoczi" <address@hidden>
> Sent Time: 2018-04-15 09:25:30 (Sunday)
> To: "Su Hang" <address@hidden>
> Cc: "jim mussared" <address@hidden>, qemu-devel <address@hidden>, "joel 
> stanley" <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v4 1/2] Implement .hex file loader
> 
> On Sat, Apr 14, 2018 at 11:44 PM, Su Hang <address@hidden> wrote:
> > Thanks for your detailed reply! I  will carefully treat
> > all the content that you mentioned, and apply them in v5.
> 
> By the way, if you are wondering why the parser needs to validate
> everything, here is an example scenario:
> 
> QEMU may be used to host an online micro:bit simulator where the user
> can provide a .hex file.  In that case QEMU is running on a server and
> the .hex file is provided by an untrusted user.  That user must not be
> able to crash QEMU (and exploit security holes).
> 
> Stefan