[Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Stefan Hajnoczi]

It turns out that GNU binutils emits START_SEG_ADDR_RECORD when the start
address is within the first megabyte (< 0x100000).  Therefore we must
handle this record type.

Originally we thought this record type was x86-specific, but binutils
also emits it on non-x86 architectures.

Based-on: <1527161340-3200-1-git-send-email-suhang16@mails.ucas.ac.cn>
Cc: Su Hang <suhang16@mails.ucas.ac.cn>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Su Hang: Feel free to squash this into the next revision of your hex
loader patch.  Don't worry about the authorship information.

 hw/core/loader.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 3c0202caa8..7843b487b2 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1423,8 +1423,14 @@ static int handle_record_type(HexParser *parser)
         break;
 
     case START_SEG_ADDR_RECORD:
-        /* TODO: START_SEG_ADDR_RECORD is x86-specific */
-        return -1;
+        if (line->byte_count != 4 && line->address != 0) {
+            return -1;
+        }
+
+        /* x86 16-bit CS:IP segmented addressing */
+        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
+                                (line->data[2] << 8) | line->data[3];
+        break;
 
     case START_LINEAR_ADDR_RECORD:
         if (line->byte_count != 4 && line->address != 0) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Su Hang]

Sure, Thanks for remind me of this.
One thing I must point out, in current code logic, if "START_SEG_ADDR_RECORD"
occured multiple times, only the last one works. I don't know whether GNU
binutils would emit 'The Record' many times.

Best,
SU Hang


> -----Original Messages-----
> From: "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent Time: 2018-06-10 00:32:52 (Sunday)
> To: qemu-devel@nongnu.org
> Cc: "Su Hang" <suhang16@mails.ucas.ac.cn>, jusual@mail.ru, jim@groklearning.com, joel@jms.id.au, qemu.ml@steffen-goertz.de, "Stefan Hajnoczi" <stefanha@redhat.com>
> Subject: [PATCH] loader: implement START_SEG_ADDR_RECORD
> 
> It turns out that GNU binutils emits START_SEG_ADDR_RECORD when the start
> address is within the first megabyte (< 0x100000).  Therefore we must
> handle this record type.
> 
> Originally we thought this record type was x86-specific, but binutils
> also emits it on non-x86 architectures.
> 
> Based-on: <1527161340-3200-1-git-send-email-suhang16@mails.ucas.ac.cn>
> Cc: Su Hang <suhang16@mails.ucas.ac.cn>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Su Hang: Feel free to squash this into the next revision of your hex
> loader patch.  Don't worry about the authorship information.
> 
>  hw/core/loader.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 3c0202caa8..7843b487b2 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1423,8 +1423,14 @@ static int handle_record_type(HexParser *parser)
>          break;
>  
>      case START_SEG_ADDR_RECORD:
> -        /* TODO: START_SEG_ADDR_RECORD is x86-specific */
> -        return -1;
> +        if (line->byte_count != 4 && line->address != 0) {
> +            return -1;
> +        }
> +
> +        /* x86 16-bit CS:IP segmented addressing */
> +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
> +                                (line->data[2] << 8) | line->data[3];
> +        break;
>  
>      case START_LINEAR_ADDR_RECORD:
>          if (line->byte_count != 4 && line->address != 0) {
> -- 
> 2.17.1

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

On Sun, Jun 10, 2018 at 10:04:32AM +0800, Su Hang wrote:
> Sure, Thanks for remind me of this.
> One thing I must point out, in current code logic, if "START_SEG_ADDR_RECORD"
> occured multiple times, only the last one works. I don't know whether GNU
> binutils would emit 'The Record' many times.

It does not.  The behavior you described seems fine.

Do you have time to send a new revision of your patch series addressing
the comments from last time?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Su Hang]

I do have time, the function mentioned in last comments isn't difficult to
implement, but I wonder how to write corresponding qtest-case for cortex-m3. You
know, becuase #current# QEMU doesn't surpport the cortex-m3 instruction, I don't
know how to prove correctness of hex loader. For example, in my past patch
serias, "Hello World!" will be printed out, if Hex File get successfully loaded.

Julia has told me a method, I haven't tried it. I will do it as quick as I
could.

Best,
Su Hang


> -----Original Messages-----
> From: "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent Time: 2018-06-11 21:55:07 (Monday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: qemu-devel@nongnu.org, jusual@mail.ru, jim@groklearning.com, joel@jms.id.au, qemu.ml@steffen-goertz.de
> Subject: Re: [PATCH] loader: implement START_SEG_ADDR_RECORD
> 
> On Sun, Jun 10, 2018 at 10:04:32AM +0800, Su Hang wrote:
> > Sure, Thanks for remind me of this.
> > One thing I must point out, in current code logic, if "START_SEG_ADDR_RECORD"
> > occured multiple times, only the last one works. I don't know whether GNU
> > binutils would emit 'The Record' many times.
> 
> It does not.  The behavior you described seems fine.
> 
> Do you have time to send a new revision of your patch series addressing
> the comments from last time?
> 
> Thanks,
> Stefan

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Tue, Jun 12, 2018 at 01:46:34PM +0800, Su Hang wrote:
> 
> I do have time, the function mentioned in last comments isn't difficult to
> implement, but I wonder how to write corresponding qtest-case for cortex-m3. You
> know, becuase #current# QEMU doesn't surpport the cortex-m3 instruction, I don't
> know how to prove correctness of hex loader. For example, in my past patch
> serias, "Hello World!" will be printed out, if Hex File get successfully loaded.
> 
> Julia has told me a method, I haven't tried it. I will do it as quick as I
> could.

Thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Su Hang]

Sorry, Julia and Stefan. My school mentor assign an emergency task to me, he ask
me to finish it within two weeks. I'm afraid I may not able to implement the
function mentioned in last comments in this two weeks. If the new function is
in urgent need, please feel free to take over the patches; if not, I will
implement it after my task get finished.

Sincerely sorry for the trouble I might made.

SU Hang

> -----Original Messages-----
> From: "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent Time: 2018-06-11 21:55:07 (Monday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: qemu-devel@nongnu.org, jusual@mail.ru, jim@groklearning.com, joel@jms.id.au, qemu.ml@steffen-goertz.de
> Subject: Re: [PATCH] loader: implement START_SEG_ADDR_RECORD
> 
> On Sun, Jun 10, 2018 at 10:04:32AM +0800, Su Hang wrote:
> > Sure, Thanks for remind me of this.
> > One thing I must point out, in current code logic, if "START_SEG_ADDR_RECORD"
> > occured multiple times, only the last one works. I don't know whether GNU
> > binutils would emit 'The Record' many times.
> 
> It does not.  The behavior you described seems fine.
> 
> Do you have time to send a new revision of your patch series addressing
> the comments from last time?
> 
> Thanks,
> Stefan

Re: [Qemu-devel] [PATCH] loader: implement START_SEG_ADDR_RECORD

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On Thu, Jun 14, 2018 at 02:43:11PM +0800, Su Hang wrote:
> 
> Sorry, Julia and Stefan. My school mentor assign an emergency task to me, he ask
> me to finish it within two weeks. I'm afraid I may not able to implement the
> function mentioned in last comments in this two weeks. If the new function is
> in urgent need, please feel free to take over the patches; if not, I will
> implement it after my task get finished.

Thanks for letting us know.  If someone takes over they will CC you on
patches so you'll know if someone is working on it.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]