diff mbox

[v12,2/2] docs: Add a generic loader explanation document

Message ID 0c4600f5c922569a57f4e9a9f1b6b4b5469b2a4c.1475102513.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis Sept. 28, 2016, 10:45 p.m. UTC
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V11:
 - Fix corrections
V10:
 - Split the data loading and PC setting
V9:
 - Clarify the image loading options
V8:
 - Improve documentation
V6:
 - Fixup documentation
V4:
 - Re-write to be more comprehensive

 docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 docs/generic-loader.txt

Comments

Markus Armbruster Sept. 29, 2016, 9:24 a.m. UTC | #1
Alistair Francis <alistair.francis@xilinx.com> writes:

> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> V11:
>  - Fix corrections
> V10:
>  - Split the data loading and PC setting
> V9:
>  - Clarify the image loading options
> V8:
>  - Improve documentation
> V6:
>  - Fixup documentation
> V4:
>  - Re-write to be more comprehensive
>
>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 docs/generic-loader.txt
>
> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> new file mode 100644
> index 0000000..d1f8ce3
> --- /dev/null
> +++ b/docs/generic-loader.txt
> @@ -0,0 +1,81 @@
> +Copyright (c) 2016 Xilinx Inc.
> +
> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
> +the COPYING file in the top-level directory.
> +
> +
> +The 'loader' device allows the user to load multiple images or values into
> +QEMU at startup.
> +
> +Loading Data into Memory Values
> +---------------------
> +The loader device allows memory values to be set from the command line. This
> +can be done by following the syntax below:
> +
> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
> +
> +    <addr>      - The address to store the data in.
> +    <data>      - The value to be written to the address. The maximum size of
> +                  the data is 8 bytes.
> +    <data-len>  - The length of the data in bytes. This argument must be
> +                  included if the data argument is.
> +    <data-be>   - Set to true if the data to be stored on the guest should be
> +                  written as big endian data. The default is to write little
> +                  endian data.
> +    <cpu-num>   - The number of the CPU's address space where the data should
> +                  be loaded. If not specified the address space of the first
> +                  CPU is used.
> +
> +For all values both hex and decimal values are allowed. By default the values
> +will be parsed as decimal. To use hex values the user should prefix the number
> +with a '0x'.

Unless you bypassed QemuOpts number parsing somehow, octal works as
well.  In case you did bypass: don't!  Command line consistency matters.
Follow-up patch reverting the bypass would be required.

Not sure we want to document QemuOpts number syntax everywhere we
explain how a certain feature uses the command line.  A pointer to the
canonical place could be better.  Anyway, not something that needs
fixing before we commit.

> +
> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
> +
> +Setting a CPU's Program Counter
> +---------------------
> +The loader device allows the CPU's PC to be set from the command line. This
> +can be done by following the syntax below:
> +
> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
> +
> +    <addr>      - The value to use as the CPU's PC.
> +    <cpu-num>   - The number of the CPU whose PC should be set to the
> +                  specified value.
> +
> +For all values both hex and decimal values are allowed. By default the values
> +will be parsed as decimal. To use hex values the user should prefix the number
> +with a '0x'.
> +
> +An example of setting CPU 0's PC to 0x8000 is:
> +    -device loader,addr=0x8000,cpu-num=0
> +
> +Loading Files
> +---------------------
> +The loader device also allows files to be loaded into memory. This can be done
> +similarly to setting memory values. The syntax is shown below:
> +
> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
> +
> +    <file>      - A file to be loaded into memory
> +    <addr>      - The addr in memory that the file should be loaded. This is
> +                  ignored if you are using an ELF (unless force-raw is true).
> +                  This is required if you aren't loading an ELF.
> +    <cpu-num>   - This specifies the CPU that should be used. This is an
> +                  optional argument and will cause the CPU's PC to be set to
> +                  where the image is stored or in the case of an ELF file to
> +                  the value in the header. This option should only be used
> +                  for the boot image.
> +                  This will also cause the image to be written to the specified
> +                  CPU's address space. If not specified, the default is CPU 0.

Using @cpu-num both for further specifying the meaning of @addr and for
setting that CPU's PC is awkward.  Are you sure there will never be a
use case where you need to specify the CPU without also setting its PC?

To be clear: while I feel this is a question we must discuss and
resolve, I don't think we need to hold the series for it.

> +    <force-raw> - Forces the file to be treated as a raw image. This can be
> +                  used to specify the load address of ELF files.

"Specifying the load address of an ELF file" sounds like loading a
position-independent ELF file at a particular address.  But I guess this
is actually for loading a file raw even though it is recognized by QEMU
as ELF.

> +
> +For all values both hex and decimal values are allowed. By default the values
> +will be parsed as decimal. To use hex values the user should prefix the number
> +with a '0x'.
> +
> +An example of loading an ELF file which CPU0 will boot is shown below:
> +    -device loader,file=./images/boot.elf,cpu-num=0

Naive question: if you want to more than one thing (where "thing" is one
of the three cases described above), do you need a separate -device for
each, or can you combine them into one?


Again, while my questions may lead to improvements, they can be applied
on top.
Alistair Francis Sept. 30, 2016, 12:25 a.m. UTC | #2
On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> V11:
>>  - Fix corrections
>> V10:
>>  - Split the data loading and PC setting
>> V9:
>>  - Clarify the image loading options
>> V8:
>>  - Improve documentation
>> V6:
>>  - Fixup documentation
>> V4:
>>  - Re-write to be more comprehensive
>>
>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>  create mode 100644 docs/generic-loader.txt
>>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> new file mode 100644
>> index 0000000..d1f8ce3
>> --- /dev/null
>> +++ b/docs/generic-loader.txt
>> @@ -0,0 +1,81 @@
>> +Copyright (c) 2016 Xilinx Inc.
>> +
>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>> +the COPYING file in the top-level directory.
>> +
>> +
>> +The 'loader' device allows the user to load multiple images or values into
>> +QEMU at startup.
>> +
>> +Loading Data into Memory Values
>> +---------------------
>> +The loader device allows memory values to be set from the command line. This
>> +can be done by following the syntax below:
>> +
>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>> +
>> +    <addr>      - The address to store the data in.
>> +    <data>      - The value to be written to the address. The maximum size of
>> +                  the data is 8 bytes.
>> +    <data-len>  - The length of the data in bytes. This argument must be
>> +                  included if the data argument is.
>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>> +                  written as big endian data. The default is to write little
>> +                  endian data.
>> +    <cpu-num>   - The number of the CPU's address space where the data should
>> +                  be loaded. If not specified the address space of the first
>> +                  CPU is used.
>> +
>> +For all values both hex and decimal values are allowed. By default the values
>> +will be parsed as decimal. To use hex values the user should prefix the number
>> +with a '0x'.
>
> Unless you bypassed QemuOpts number parsing somehow, octal works as
> well.  In case you did bypass: don't!  Command line consistency matters.
> Follow-up patch reverting the bypass would be required.
>
> Not sure we want to document QemuOpts number syntax everywhere we
> explain how a certain feature uses the command line.  A pointer to the
> canonical place could be better.  Anyway, not something that needs
> fixing before we commit.

I didn't bypass it, octal should work as well. I have clarified that a
bit in the doc.

>
>> +
>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>> +
>> +Setting a CPU's Program Counter
>> +---------------------
>> +The loader device allows the CPU's PC to be set from the command line. This
>> +can be done by following the syntax below:
>> +
>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>> +
>> +    <addr>      - The value to use as the CPU's PC.
>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>> +                  specified value.
>> +
>> +For all values both hex and decimal values are allowed. By default the values
>> +will be parsed as decimal. To use hex values the user should prefix the number
>> +with a '0x'.
>> +
>> +An example of setting CPU 0's PC to 0x8000 is:
>> +    -device loader,addr=0x8000,cpu-num=0
>> +
>> +Loading Files
>> +---------------------
>> +The loader device also allows files to be loaded into memory. This can be done
>> +similarly to setting memory values. The syntax is shown below:
>> +
>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>> +
>> +    <file>      - A file to be loaded into memory
>> +    <addr>      - The addr in memory that the file should be loaded. This is
>> +                  ignored if you are using an ELF (unless force-raw is true).
>> +                  This is required if you aren't loading an ELF.
>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>> +                  optional argument and will cause the CPU's PC to be set to
>> +                  where the image is stored or in the case of an ELF file to
>> +                  the value in the header. This option should only be used
>> +                  for the boot image.
>> +                  This will also cause the image to be written to the specified
>> +                  CPU's address space. If not specified, the default is CPU 0.
>
> Using @cpu-num both for further specifying the meaning of @addr and for
> setting that CPU's PC is awkward.  Are you sure there will never be a
> use case where you need to specify the CPU without also setting its PC?
>
> To be clear: while I feel this is a question we must discuss and
> resolve, I don't think we need to hold the series for it.

I agree that this can occur. Internally in the loader framework is a
set_pc variable.

In the future we can make this user accessible and then allow that to
decide if the PC should be set or not.

>
>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>> +                  used to specify the load address of ELF files.
>
> "Specifying the load address of an ELF file" sounds like loading a
> position-independent ELF file at a particular address.  But I guess this
> is actually for loading a file raw even though it is recognized by QEMU
> as ELF.

This option basically does make an ELF file position-independent as
the user can control where it is loaded.

>
>> +
>> +For all values both hex and decimal values are allowed. By default the values
>> +will be parsed as decimal. To use hex values the user should prefix the number
>> +with a '0x'.
>> +
>> +An example of loading an ELF file which CPU0 will boot is shown below:
>> +    -device loader,file=./images/boot.elf,cpu-num=0
>
> Naive question: if you want to more than one thing (where "thing" is one
> of the three cases described above), do you need a separate -device for
> each, or can you combine them into one?

You can't really squash them together. If you wanted to set two
registers, you would need two commands.

Thanks,

Alistair

>
>
> Again, while my questions may lead to improvements, they can be applied
> on top.
>
Markus Armbruster Sept. 30, 2016, 5:36 a.m. UTC | #3
Alistair Francis <alistair.francis@xilinx.com> writes:

> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> V11:
>>>  - Fix corrections
>>> V10:
>>>  - Split the data loading and PC setting
>>> V9:
>>>  - Clarify the image loading options
>>> V8:
>>>  - Improve documentation
>>> V6:
>>>  - Fixup documentation
>>> V4:
>>>  - Re-write to be more comprehensive
>>>
>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 81 insertions(+)
>>>  create mode 100644 docs/generic-loader.txt
>>>
>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>> new file mode 100644
>>> index 0000000..d1f8ce3
>>> --- /dev/null
>>> +++ b/docs/generic-loader.txt
>>> @@ -0,0 +1,81 @@
>>> +Copyright (c) 2016 Xilinx Inc.
>>> +
>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>> +the COPYING file in the top-level directory.
>>> +
>>> +
>>> +The 'loader' device allows the user to load multiple images or values into
>>> +QEMU at startup.
>>> +
>>> +Loading Data into Memory Values
>>> +---------------------
>>> +The loader device allows memory values to be set from the command line. This
>>> +can be done by following the syntax below:
>>> +
>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>> +
>>> +    <addr>      - The address to store the data in.
>>> +    <data>      - The value to be written to the address. The maximum size of
>>> +                  the data is 8 bytes.
>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>> +                  included if the data argument is.
>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>> +                  written as big endian data. The default is to write little
>>> +                  endian data.
>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>> +                  be loaded. If not specified the address space of the first
>>> +                  CPU is used.
>>> +
>>> +For all values both hex and decimal values are allowed. By default the values
>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>> +with a '0x'.
>>
>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>> well.  In case you did bypass: don't!  Command line consistency matters.
>> Follow-up patch reverting the bypass would be required.
>>
>> Not sure we want to document QemuOpts number syntax everywhere we
>> explain how a certain feature uses the command line.  A pointer to the
>> canonical place could be better.  Anyway, not something that needs
>> fixing before we commit.
>
> I didn't bypass it, octal should work as well. I have clarified that a
> bit in the doc.

Thanks.

>>> +
>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>> +
>>> +Setting a CPU's Program Counter
>>> +---------------------
>>> +The loader device allows the CPU's PC to be set from the command line. This
>>> +can be done by following the syntax below:
>>> +
>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>> +
>>> +    <addr>      - The value to use as the CPU's PC.
>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>> +                  specified value.
>>> +
>>> +For all values both hex and decimal values are allowed. By default the values
>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>> +with a '0x'.
>>> +
>>> +An example of setting CPU 0's PC to 0x8000 is:
>>> +    -device loader,addr=0x8000,cpu-num=0
>>> +
>>> +Loading Files
>>> +---------------------
>>> +The loader device also allows files to be loaded into memory. This can be done
>>> +similarly to setting memory values. The syntax is shown below:
>>> +
>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>> +
>>> +    <file>      - A file to be loaded into memory
>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>> +                  This is required if you aren't loading an ELF.
>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>> +                  optional argument and will cause the CPU's PC to be set to
>>> +                  where the image is stored or in the case of an ELF file to
>>> +                  the value in the header. This option should only be used
>>> +                  for the boot image.
>>> +                  This will also cause the image to be written to the specified
>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>
>> Using @cpu-num both for further specifying the meaning of @addr and for
>> setting that CPU's PC is awkward.  Are you sure there will never be a
>> use case where you need to specify the CPU without also setting its PC?
>>
>> To be clear: while I feel this is a question we must discuss and
>> resolve, I don't think we need to hold the series for it.
>
> I agree that this can occur. Internally in the loader framework is a
> set_pc variable.
>
> In the future we can make this user accessible and then allow that to
> decide if the PC should be set or not.

If you can't do it right away, please document it as restriction, and
add a TODO comment to lift it.

>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>> +                  used to specify the load address of ELF files.
>>
>> "Specifying the load address of an ELF file" sounds like loading a
>> position-independent ELF file at a particular address.  But I guess this
>> is actually for loading a file raw even though it is recognized by QEMU
>> as ELF.
>
> This option basically does make an ELF file position-independent as
> the user can control where it is loaded.

Aha.  Then the name "force-raw" is confusing.

>>> +
>>> +For all values both hex and decimal values are allowed. By default the values
>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>> +with a '0x'.
>>> +
>>> +An example of loading an ELF file which CPU0 will boot is shown below:
>>> +    -device loader,file=./images/boot.elf,cpu-num=0
>>
>> Naive question: if you want to more than one thing (where "thing" is one
>> of the three cases described above), do you need a separate -device for
>> each, or can you combine them into one?
>
> You can't really squash them together. If you wanted to set two
> registers, you would need two commands.

That's okay.  It just isn't quite obvious to me in the text.

>
> Thanks,
>
> Alistair
>
>>
>>
>> Again, while my questions may lead to improvements, they can be applied
>> on top.
>>
Alistair Francis Oct. 3, 2016, 5:28 p.m. UTC | #4
On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>> V11:
>>>>  - Fix corrections
>>>> V10:
>>>>  - Split the data loading and PC setting
>>>> V9:
>>>>  - Clarify the image loading options
>>>> V8:
>>>>  - Improve documentation
>>>> V6:
>>>>  - Fixup documentation
>>>> V4:
>>>>  - Re-write to be more comprehensive
>>>>
>>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 81 insertions(+)
>>>>  create mode 100644 docs/generic-loader.txt
>>>>
>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>> new file mode 100644
>>>> index 0000000..d1f8ce3
>>>> --- /dev/null
>>>> +++ b/docs/generic-loader.txt
>>>> @@ -0,0 +1,81 @@
>>>> +Copyright (c) 2016 Xilinx Inc.
>>>> +
>>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>>> +the COPYING file in the top-level directory.
>>>> +
>>>> +
>>>> +The 'loader' device allows the user to load multiple images or values into
>>>> +QEMU at startup.
>>>> +
>>>> +Loading Data into Memory Values
>>>> +---------------------
>>>> +The loader device allows memory values to be set from the command line. This
>>>> +can be done by following the syntax below:
>>>> +
>>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>>> +
>>>> +    <addr>      - The address to store the data in.
>>>> +    <data>      - The value to be written to the address. The maximum size of
>>>> +                  the data is 8 bytes.
>>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>>> +                  included if the data argument is.
>>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>>> +                  written as big endian data. The default is to write little
>>>> +                  endian data.
>>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>>> +                  be loaded. If not specified the address space of the first
>>>> +                  CPU is used.
>>>> +
>>>> +For all values both hex and decimal values are allowed. By default the values
>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>> +with a '0x'.
>>>
>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>> Follow-up patch reverting the bypass would be required.
>>>
>>> Not sure we want to document QemuOpts number syntax everywhere we
>>> explain how a certain feature uses the command line.  A pointer to the
>>> canonical place could be better.  Anyway, not something that needs
>>> fixing before we commit.
>>
>> I didn't bypass it, octal should work as well. I have clarified that a
>> bit in the doc.
>
> Thanks.
>
>>>> +
>>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>>> +
>>>> +Setting a CPU's Program Counter
>>>> +---------------------
>>>> +The loader device allows the CPU's PC to be set from the command line. This
>>>> +can be done by following the syntax below:
>>>> +
>>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>>> +
>>>> +    <addr>      - The value to use as the CPU's PC.
>>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>>> +                  specified value.
>>>> +
>>>> +For all values both hex and decimal values are allowed. By default the values
>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>> +with a '0x'.
>>>> +
>>>> +An example of setting CPU 0's PC to 0x8000 is:
>>>> +    -device loader,addr=0x8000,cpu-num=0
>>>> +
>>>> +Loading Files
>>>> +---------------------
>>>> +The loader device also allows files to be loaded into memory. This can be done
>>>> +similarly to setting memory values. The syntax is shown below:
>>>> +
>>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>>> +
>>>> +    <file>      - A file to be loaded into memory
>>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>>> +                  This is required if you aren't loading an ELF.
>>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>>> +                  optional argument and will cause the CPU's PC to be set to
>>>> +                  where the image is stored or in the case of an ELF file to
>>>> +                  the value in the header. This option should only be used
>>>> +                  for the boot image.
>>>> +                  This will also cause the image to be written to the specified
>>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>>
>>> Using @cpu-num both for further specifying the meaning of @addr and for
>>> setting that CPU's PC is awkward.  Are you sure there will never be a
>>> use case where you need to specify the CPU without also setting its PC?
>>>
>>> To be clear: while I feel this is a question we must discuss and
>>> resolve, I don't think we need to hold the series for it.
>>
>> I agree that this can occur. Internally in the loader framework is a
>> set_pc variable.
>>
>> In the future we can make this user accessible and then allow that to
>> decide if the PC should be set or not.
>
> If you can't do it right away, please document it as restriction, and
> add a TODO comment to lift it.

I have a patch that adds known restrictions.

>
>>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>>> +                  used to specify the load address of ELF files.
>>>
>>> "Specifying the load address of an ELF file" sounds like loading a
>>> position-independent ELF file at a particular address.  But I guess this
>>> is actually for loading a file raw even though it is recognized by QEMU
>>> as ELF.
>>
>> This option basically does make an ELF file position-independent as
>> the user can control where it is loaded.
>
> Aha.  Then the name "force-raw" is confusing.

I disagree. It tells QEMU to treat the image as just a dumb blob,
instead of loading it as and ELF file. I thin force-raw makes sense as
the user is telling QEMU that the image should be treated as a raw
image, no matter what it actually is.

Thanks,

Alistair

>
>>>> +
>>>> +For all values both hex and decimal values are allowed. By default the values
>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>> +with a '0x'.
>>>> +
>>>> +An example of loading an ELF file which CPU0 will boot is shown below:
>>>> +    -device loader,file=./images/boot.elf,cpu-num=0
>>>
>>> Naive question: if you want to more than one thing (where "thing" is one
>>> of the three cases described above), do you need a separate -device for
>>> each, or can you combine them into one?
>>
>> You can't really squash them together. If you wanted to set two
>> registers, you would need two commands.
>
> That's okay.  It just isn't quite obvious to me in the text.
>
>>
>> Thanks,
>>
>> Alistair
>>
>>>
>>>
>>> Again, while my questions may lead to improvements, they can be applied
>>> on top.
>>>
>
Markus Armbruster Oct. 4, 2016, 7:56 a.m. UTC | #5
Alistair Francis <alistair.francis@xilinx.com> writes:

> On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>
>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>> ---
>>>>> V11:
>>>>>  - Fix corrections
>>>>> V10:
>>>>>  - Split the data loading and PC setting
>>>>> V9:
>>>>>  - Clarify the image loading options
>>>>> V8:
>>>>>  - Improve documentation
>>>>> V6:
>>>>>  - Fixup documentation
>>>>> V4:
>>>>>  - Re-write to be more comprehensive
>>>>>
>>>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 81 insertions(+)
>>>>>  create mode 100644 docs/generic-loader.txt
>>>>>
>>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>>> new file mode 100644
>>>>> index 0000000..d1f8ce3
>>>>> --- /dev/null
>>>>> +++ b/docs/generic-loader.txt
>>>>> @@ -0,0 +1,81 @@
>>>>> +Copyright (c) 2016 Xilinx Inc.
>>>>> +
>>>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>>>> +the COPYING file in the top-level directory.
>>>>> +
>>>>> +
>>>>> +The 'loader' device allows the user to load multiple images or values into
>>>>> +QEMU at startup.
>>>>> +
>>>>> +Loading Data into Memory Values
>>>>> +---------------------
>>>>> +The loader device allows memory values to be set from the command line. This
>>>>> +can be done by following the syntax below:
>>>>> +
>>>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>>>> +
>>>>> +    <addr>      - The address to store the data in.
>>>>> +    <data>      - The value to be written to the address. The maximum size of
>>>>> +                  the data is 8 bytes.
>>>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>>>> +                  included if the data argument is.
>>>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>>>> +                  written as big endian data. The default is to write little
>>>>> +                  endian data.
>>>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>>>> +                  be loaded. If not specified the address space of the first
>>>>> +                  CPU is used.
>>>>> +
>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>> +with a '0x'.
>>>>
>>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>>> Follow-up patch reverting the bypass would be required.
>>>>
>>>> Not sure we want to document QemuOpts number syntax everywhere we
>>>> explain how a certain feature uses the command line.  A pointer to the
>>>> canonical place could be better.  Anyway, not something that needs
>>>> fixing before we commit.
>>>
>>> I didn't bypass it, octal should work as well. I have clarified that a
>>> bit in the doc.
>>
>> Thanks.
>>
>>>>> +
>>>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>>>> +
>>>>> +Setting a CPU's Program Counter
>>>>> +---------------------
>>>>> +The loader device allows the CPU's PC to be set from the command line. This
>>>>> +can be done by following the syntax below:
>>>>> +
>>>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>>>> +
>>>>> +    <addr>      - The value to use as the CPU's PC.
>>>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>>>> +                  specified value.
>>>>> +
>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>> +with a '0x'.
>>>>> +
>>>>> +An example of setting CPU 0's PC to 0x8000 is:
>>>>> +    -device loader,addr=0x8000,cpu-num=0
>>>>> +
>>>>> +Loading Files
>>>>> +---------------------
>>>>> +The loader device also allows files to be loaded into memory. This can be done
>>>>> +similarly to setting memory values. The syntax is shown below:
>>>>> +
>>>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>>>> +
>>>>> +    <file>      - A file to be loaded into memory
>>>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>>>> +                  This is required if you aren't loading an ELF.
>>>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>>>> +                  optional argument and will cause the CPU's PC to be set to
>>>>> +                  where the image is stored or in the case of an ELF file to
>>>>> +                  the value in the header. This option should only be used
>>>>> +                  for the boot image.
>>>>> +                  This will also cause the image to be written to the specified
>>>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>>>
>>>> Using @cpu-num both for further specifying the meaning of @addr and for
>>>> setting that CPU's PC is awkward.  Are you sure there will never be a
>>>> use case where you need to specify the CPU without also setting its PC?
>>>>
>>>> To be clear: while I feel this is a question we must discuss and
>>>> resolve, I don't think we need to hold the series for it.
>>>
>>> I agree that this can occur. Internally in the loader framework is a
>>> set_pc variable.
>>>
>>> In the future we can make this user accessible and then allow that to
>>> decide if the PC should be set or not.
>>
>> If you can't do it right away, please document it as restriction, and
>> add a TODO comment to lift it.
>
> I have a patch that adds known restrictions.
>
>>
>>>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>>>> +                  used to specify the load address of ELF files.
>>>>
>>>> "Specifying the load address of an ELF file" sounds like loading a
>>>> position-independent ELF file at a particular address.  But I guess this
>>>> is actually for loading a file raw even though it is recognized by QEMU
>>>> as ELF.
>>>
>>> This option basically does make an ELF file position-independent as
>>> the user can control where it is loaded.
>>
>> Aha.  Then the name "force-raw" is confusing.
>
> I disagree. It tells QEMU to treat the image as just a dumb blob,
> instead of loading it as and ELF file. I thin force-raw makes sense as
> the user is telling QEMU that the image should be treated as a raw
> image, no matter what it actually is.

I'm still confused then.

I can see two possible features here, and based on your documentation
and commentary, I can't tell which one you implemented:

0. QEMU can load raw files and ELF executable files.  Raw files are
loaded verbatim at a the specified address.  ELF executable files are
loaded by an ELF loader, which loads the program header table's loadable
segments.

1. force-raw overrides the ELF detection, to let you load an ELF
executable file verbatim, as if it was raw.

2. force-raw lets you overrides the ELF file's load address: the ELF
loader uses the specified address instead of the address contained in
the ELF file.

Which one is it?  Your latest answer strongly suggests 1, but prior
answers have made me suspect 2.
Alistair Francis Oct. 4, 2016, 3:45 p.m. UTC | #6
On Tue, Oct 4, 2016 at 12:56 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>>
>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>>> ---
>>>>>> V11:
>>>>>>  - Fix corrections
>>>>>> V10:
>>>>>>  - Split the data loading and PC setting
>>>>>> V9:
>>>>>>  - Clarify the image loading options
>>>>>> V8:
>>>>>>  - Improve documentation
>>>>>> V6:
>>>>>>  - Fixup documentation
>>>>>> V4:
>>>>>>  - Re-write to be more comprehensive
>>>>>>
>>>>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 81 insertions(+)
>>>>>>  create mode 100644 docs/generic-loader.txt
>>>>>>
>>>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..d1f8ce3
>>>>>> --- /dev/null
>>>>>> +++ b/docs/generic-loader.txt
>>>>>> @@ -0,0 +1,81 @@
>>>>>> +Copyright (c) 2016 Xilinx Inc.
>>>>>> +
>>>>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>>>>> +the COPYING file in the top-level directory.
>>>>>> +
>>>>>> +
>>>>>> +The 'loader' device allows the user to load multiple images or values into
>>>>>> +QEMU at startup.
>>>>>> +
>>>>>> +Loading Data into Memory Values
>>>>>> +---------------------
>>>>>> +The loader device allows memory values to be set from the command line. This
>>>>>> +can be done by following the syntax below:
>>>>>> +
>>>>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>>>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>>>>> +
>>>>>> +    <addr>      - The address to store the data in.
>>>>>> +    <data>      - The value to be written to the address. The maximum size of
>>>>>> +                  the data is 8 bytes.
>>>>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>>>>> +                  included if the data argument is.
>>>>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>>>>> +                  written as big endian data. The default is to write little
>>>>>> +                  endian data.
>>>>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>>>>> +                  be loaded. If not specified the address space of the first
>>>>>> +                  CPU is used.
>>>>>> +
>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>> +with a '0x'.
>>>>>
>>>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>>>> Follow-up patch reverting the bypass would be required.
>>>>>
>>>>> Not sure we want to document QemuOpts number syntax everywhere we
>>>>> explain how a certain feature uses the command line.  A pointer to the
>>>>> canonical place could be better.  Anyway, not something that needs
>>>>> fixing before we commit.
>>>>
>>>> I didn't bypass it, octal should work as well. I have clarified that a
>>>> bit in the doc.
>>>
>>> Thanks.
>>>
>>>>>> +
>>>>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>>>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>>>>> +
>>>>>> +Setting a CPU's Program Counter
>>>>>> +---------------------
>>>>>> +The loader device allows the CPU's PC to be set from the command line. This
>>>>>> +can be done by following the syntax below:
>>>>>> +
>>>>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>>>>> +
>>>>>> +    <addr>      - The value to use as the CPU's PC.
>>>>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>>>>> +                  specified value.
>>>>>> +
>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>> +with a '0x'.
>>>>>> +
>>>>>> +An example of setting CPU 0's PC to 0x8000 is:
>>>>>> +    -device loader,addr=0x8000,cpu-num=0
>>>>>> +
>>>>>> +Loading Files
>>>>>> +---------------------
>>>>>> +The loader device also allows files to be loaded into memory. This can be done
>>>>>> +similarly to setting memory values. The syntax is shown below:
>>>>>> +
>>>>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>>>>> +
>>>>>> +    <file>      - A file to be loaded into memory
>>>>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>>>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>>>>> +                  This is required if you aren't loading an ELF.
>>>>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>>>>> +                  optional argument and will cause the CPU's PC to be set to
>>>>>> +                  where the image is stored or in the case of an ELF file to
>>>>>> +                  the value in the header. This option should only be used
>>>>>> +                  for the boot image.
>>>>>> +                  This will also cause the image to be written to the specified
>>>>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>>>>
>>>>> Using @cpu-num both for further specifying the meaning of @addr and for
>>>>> setting that CPU's PC is awkward.  Are you sure there will never be a
>>>>> use case where you need to specify the CPU without also setting its PC?
>>>>>
>>>>> To be clear: while I feel this is a question we must discuss and
>>>>> resolve, I don't think we need to hold the series for it.
>>>>
>>>> I agree that this can occur. Internally in the loader framework is a
>>>> set_pc variable.
>>>>
>>>> In the future we can make this user accessible and then allow that to
>>>> decide if the PC should be set or not.
>>>
>>> If you can't do it right away, please document it as restriction, and
>>> add a TODO comment to lift it.
>>
>> I have a patch that adds known restrictions.
>>
>>>
>>>>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>>>>> +                  used to specify the load address of ELF files.
>>>>>
>>>>> "Specifying the load address of an ELF file" sounds like loading a
>>>>> position-independent ELF file at a particular address.  But I guess this
>>>>> is actually for loading a file raw even though it is recognized by QEMU
>>>>> as ELF.
>>>>
>>>> This option basically does make an ELF file position-independent as
>>>> the user can control where it is loaded.
>>>
>>> Aha.  Then the name "force-raw" is confusing.
>>
>> I disagree. It tells QEMU to treat the image as just a dumb blob,
>> instead of loading it as and ELF file. I thin force-raw makes sense as
>> the user is telling QEMU that the image should be treated as a raw
>> image, no matter what it actually is.
>
> I'm still confused then.
>
> I can see two possible features here, and based on your documentation
> and commentary, I can't tell which one you implemented:
>
> 0. QEMU can load raw files and ELF executable files.  Raw files are
> loaded verbatim at a the specified address.  ELF executable files are
> loaded by an ELF loader, which loads the program header table's loadable
> segments.
>
> 1. force-raw overrides the ELF detection, to let you load an ELF
> executable file verbatim, as if it was raw.

This is what the option does. It forces the image to be treated as a raw image.

I'm sorry if I implied it was option 2 instead.

Thanks,

Alistair

>
> 2. force-raw lets you overrides the ELF file's load address: the ELF
> loader uses the specified address instead of the address contained in
> the ELF file.
>
> Which one is it?  Your latest answer strongly suggests 1, but prior
> answers have made me suspect 2.
>
Markus Armbruster Oct. 5, 2016, 7:44 a.m. UTC | #7
Alistair Francis <alistair.francis@xilinx.com> writes:

> On Tue, Oct 4, 2016 at 12:56 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>
>>>>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>>>
>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>>>> ---
>>>>>>> V11:
>>>>>>>  - Fix corrections
>>>>>>> V10:
>>>>>>>  - Split the data loading and PC setting
>>>>>>> V9:
>>>>>>>  - Clarify the image loading options
>>>>>>> V8:
>>>>>>>  - Improve documentation
>>>>>>> V6:
>>>>>>>  - Fixup documentation
>>>>>>> V4:
>>>>>>>  - Re-write to be more comprehensive
>>>>>>>
>>>>>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 81 insertions(+)
>>>>>>>  create mode 100644 docs/generic-loader.txt
>>>>>>>
>>>>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..d1f8ce3
>>>>>>> --- /dev/null
>>>>>>> +++ b/docs/generic-loader.txt
>>>>>>> @@ -0,0 +1,81 @@
>>>>>>> +Copyright (c) 2016 Xilinx Inc.
>>>>>>> +
>>>>>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>>>>>> +the COPYING file in the top-level directory.
>>>>>>> +
>>>>>>> +
>>>>>>> +The 'loader' device allows the user to load multiple images or values into
>>>>>>> +QEMU at startup.
>>>>>>> +
>>>>>>> +Loading Data into Memory Values
>>>>>>> +---------------------
>>>>>>> +The loader device allows memory values to be set from the command line. This
>>>>>>> +can be done by following the syntax below:
>>>>>>> +
>>>>>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>>>>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>>>>>> +
>>>>>>> +    <addr>      - The address to store the data in.
>>>>>>> +    <data>      - The value to be written to the address. The maximum size of
>>>>>>> +                  the data is 8 bytes.
>>>>>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>>>>>> +                  included if the data argument is.
>>>>>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>>>>>> +                  written as big endian data. The default is to write little
>>>>>>> +                  endian data.
>>>>>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>>>>>> +                  be loaded. If not specified the address space of the first
>>>>>>> +                  CPU is used.
>>>>>>> +
>>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>>> +with a '0x'.
>>>>>>
>>>>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>>>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>>>>> Follow-up patch reverting the bypass would be required.
>>>>>>
>>>>>> Not sure we want to document QemuOpts number syntax everywhere we
>>>>>> explain how a certain feature uses the command line.  A pointer to the
>>>>>> canonical place could be better.  Anyway, not something that needs
>>>>>> fixing before we commit.
>>>>>
>>>>> I didn't bypass it, octal should work as well. I have clarified that a
>>>>> bit in the doc.
>>>>
>>>> Thanks.
>>>>
>>>>>>> +
>>>>>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>>>>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>>>>>> +
>>>>>>> +Setting a CPU's Program Counter
>>>>>>> +---------------------
>>>>>>> +The loader device allows the CPU's PC to be set from the command line. This
>>>>>>> +can be done by following the syntax below:
>>>>>>> +
>>>>>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>>>>>> +
>>>>>>> +    <addr>      - The value to use as the CPU's PC.
>>>>>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>>>>>> +                  specified value.
>>>>>>> +
>>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>>> +with a '0x'.
>>>>>>> +
>>>>>>> +An example of setting CPU 0's PC to 0x8000 is:
>>>>>>> +    -device loader,addr=0x8000,cpu-num=0
>>>>>>> +
>>>>>>> +Loading Files
>>>>>>> +---------------------
>>>>>>> +The loader device also allows files to be loaded into memory. This can be done
>>>>>>> +similarly to setting memory values. The syntax is shown below:
>>>>>>> +
>>>>>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>>>>>> +
>>>>>>> +    <file>      - A file to be loaded into memory
>>>>>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>>>>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>>>>>> +                  This is required if you aren't loading an ELF.
>>>>>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>>>>>> +                  optional argument and will cause the CPU's PC to be set to
>>>>>>> +                  where the image is stored or in the case of an ELF file to
>>>>>>> +                  the value in the header. This option should only be used
>>>>>>> +                  for the boot image.
>>>>>>> +                  This will also cause the image to be written to the specified
>>>>>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>>>>>
>>>>>> Using @cpu-num both for further specifying the meaning of @addr and for
>>>>>> setting that CPU's PC is awkward.  Are you sure there will never be a
>>>>>> use case where you need to specify the CPU without also setting its PC?
>>>>>>
>>>>>> To be clear: while I feel this is a question we must discuss and
>>>>>> resolve, I don't think we need to hold the series for it.
>>>>>
>>>>> I agree that this can occur. Internally in the loader framework is a
>>>>> set_pc variable.
>>>>>
>>>>> In the future we can make this user accessible and then allow that to
>>>>> decide if the PC should be set or not.
>>>>
>>>> If you can't do it right away, please document it as restriction, and
>>>> add a TODO comment to lift it.
>>>
>>> I have a patch that adds known restrictions.
>>>
>>>>
>>>>>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>>>>>> +                  used to specify the load address of ELF files.
>>>>>>
>>>>>> "Specifying the load address of an ELF file" sounds like loading a
>>>>>> position-independent ELF file at a particular address.  But I guess this
>>>>>> is actually for loading a file raw even though it is recognized by QEMU
>>>>>> as ELF.
>>>>>
>>>>> This option basically does make an ELF file position-independent as
>>>>> the user can control where it is loaded.
>>>>
>>>> Aha.  Then the name "force-raw" is confusing.
>>>
>>> I disagree. It tells QEMU to treat the image as just a dumb blob,
>>> instead of loading it as and ELF file. I thin force-raw makes sense as
>>> the user is telling QEMU that the image should be treated as a raw
>>> image, no matter what it actually is.
>>
>> I'm still confused then.
>>
>> I can see two possible features here, and based on your documentation
>> and commentary, I can't tell which one you implemented:
>>
>> 0. QEMU can load raw files and ELF executable files.  Raw files are
>> loaded verbatim at a the specified address.  ELF executable files are
>> loaded by an ELF loader, which loads the program header table's loadable
>> segments.
>>
>> 1. force-raw overrides the ELF detection, to let you load an ELF
>> executable file verbatim, as if it was raw.
>
> This is what the option does. It forces the image to be treated as a raw image.

Okay, makes sense now.

> I'm sorry if I implied it was option 2 instead.

No problem.  Suggest to clarify docs/generic-loader.txt, perhaps like
this:

    Loading Files
    -------------

    The loader device also allows files to be loaded into memory.  It
    can load raw files and ELF executable files.  Raw files are loaded
    verbatim.  ELF executable files are loaded by an ELF loader.  The
    syntax is shown below:

[...]
        <force-raw> - force-raw=on forces the file to be treated as a raw
                      image.  This can be used to load ELF files as if
                      they were raw.
Alistair Francis Oct. 5, 2016, 9:31 p.m. UTC | #8
On Wed, Oct 5, 2016 at 12:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> On Tue, Oct 4, 2016 at 12:56 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>>> On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>>
>>>>>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>>>>>
>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>>>>> ---
>>>>>>>> V11:
>>>>>>>>  - Fix corrections
>>>>>>>> V10:
>>>>>>>>  - Split the data loading and PC setting
>>>>>>>> V9:
>>>>>>>>  - Clarify the image loading options
>>>>>>>> V8:
>>>>>>>>  - Improve documentation
>>>>>>>> V6:
>>>>>>>>  - Fixup documentation
>>>>>>>> V4:
>>>>>>>>  - Re-write to be more comprehensive
>>>>>>>>
>>>>>>>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 81 insertions(+)
>>>>>>>>  create mode 100644 docs/generic-loader.txt
>>>>>>>>
>>>>>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..d1f8ce3
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/docs/generic-loader.txt
>>>>>>>> @@ -0,0 +1,81 @@
>>>>>>>> +Copyright (c) 2016 Xilinx Inc.
>>>>>>>> +
>>>>>>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>>>>>>>> +the COPYING file in the top-level directory.
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +The 'loader' device allows the user to load multiple images or values into
>>>>>>>> +QEMU at startup.
>>>>>>>> +
>>>>>>>> +Loading Data into Memory Values
>>>>>>>> +---------------------
>>>>>>>> +The loader device allows memory values to be set from the command line. This
>>>>>>>> +can be done by following the syntax below:
>>>>>>>> +
>>>>>>>> +     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
>>>>>>>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>>>>>>>> +
>>>>>>>> +    <addr>      - The address to store the data in.
>>>>>>>> +    <data>      - The value to be written to the address. The maximum size of
>>>>>>>> +                  the data is 8 bytes.
>>>>>>>> +    <data-len>  - The length of the data in bytes. This argument must be
>>>>>>>> +                  included if the data argument is.
>>>>>>>> +    <data-be>   - Set to true if the data to be stored on the guest should be
>>>>>>>> +                  written as big endian data. The default is to write little
>>>>>>>> +                  endian data.
>>>>>>>> +    <cpu-num>   - The number of the CPU's address space where the data should
>>>>>>>> +                  be loaded. If not specified the address space of the first
>>>>>>>> +                  CPU is used.
>>>>>>>> +
>>>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>>>> +with a '0x'.
>>>>>>>
>>>>>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>>>>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>>>>>> Follow-up patch reverting the bypass would be required.
>>>>>>>
>>>>>>> Not sure we want to document QemuOpts number syntax everywhere we
>>>>>>> explain how a certain feature uses the command line.  A pointer to the
>>>>>>> canonical place could be better.  Anyway, not something that needs
>>>>>>> fixing before we commit.
>>>>>>
>>>>>> I didn't bypass it, octal should work as well. I have clarified that a
>>>>>> bit in the doc.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>>>> +
>>>>>>>> +An example of loading value 0x8000000e to address 0xfd1a0104 is:
>>>>>>>> +    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>>>>>>> +
>>>>>>>> +Setting a CPU's Program Counter
>>>>>>>> +---------------------
>>>>>>>> +The loader device allows the CPU's PC to be set from the command line. This
>>>>>>>> +can be done by following the syntax below:
>>>>>>>> +
>>>>>>>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>>>>>>>> +
>>>>>>>> +    <addr>      - The value to use as the CPU's PC.
>>>>>>>> +    <cpu-num>   - The number of the CPU whose PC should be set to the
>>>>>>>> +                  specified value.
>>>>>>>> +
>>>>>>>> +For all values both hex and decimal values are allowed. By default the values
>>>>>>>> +will be parsed as decimal. To use hex values the user should prefix the number
>>>>>>>> +with a '0x'.
>>>>>>>> +
>>>>>>>> +An example of setting CPU 0's PC to 0x8000 is:
>>>>>>>> +    -device loader,addr=0x8000,cpu-num=0
>>>>>>>> +
>>>>>>>> +Loading Files
>>>>>>>> +---------------------
>>>>>>>> +The loader device also allows files to be loaded into memory. This can be done
>>>>>>>> +similarly to setting memory values. The syntax is shown below:
>>>>>>>> +
>>>>>>>> +    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
>>>>>>>> +
>>>>>>>> +    <file>      - A file to be loaded into memory
>>>>>>>> +    <addr>      - The addr in memory that the file should be loaded. This is
>>>>>>>> +                  ignored if you are using an ELF (unless force-raw is true).
>>>>>>>> +                  This is required if you aren't loading an ELF.
>>>>>>>> +    <cpu-num>   - This specifies the CPU that should be used. This is an
>>>>>>>> +                  optional argument and will cause the CPU's PC to be set to
>>>>>>>> +                  where the image is stored or in the case of an ELF file to
>>>>>>>> +                  the value in the header. This option should only be used
>>>>>>>> +                  for the boot image.
>>>>>>>> +                  This will also cause the image to be written to the specified
>>>>>>>> +                  CPU's address space. If not specified, the default is CPU 0.
>>>>>>>
>>>>>>> Using @cpu-num both for further specifying the meaning of @addr and for
>>>>>>> setting that CPU's PC is awkward.  Are you sure there will never be a
>>>>>>> use case where you need to specify the CPU without also setting its PC?
>>>>>>>
>>>>>>> To be clear: while I feel this is a question we must discuss and
>>>>>>> resolve, I don't think we need to hold the series for it.
>>>>>>
>>>>>> I agree that this can occur. Internally in the loader framework is a
>>>>>> set_pc variable.
>>>>>>
>>>>>> In the future we can make this user accessible and then allow that to
>>>>>> decide if the PC should be set or not.
>>>>>
>>>>> If you can't do it right away, please document it as restriction, and
>>>>> add a TODO comment to lift it.
>>>>
>>>> I have a patch that adds known restrictions.
>>>>
>>>>>
>>>>>>>> +    <force-raw> - Forces the file to be treated as a raw image. This can be
>>>>>>>> +                  used to specify the load address of ELF files.
>>>>>>>
>>>>>>> "Specifying the load address of an ELF file" sounds like loading a
>>>>>>> position-independent ELF file at a particular address.  But I guess this
>>>>>>> is actually for loading a file raw even though it is recognized by QEMU
>>>>>>> as ELF.
>>>>>>
>>>>>> This option basically does make an ELF file position-independent as
>>>>>> the user can control where it is loaded.
>>>>>
>>>>> Aha.  Then the name "force-raw" is confusing.
>>>>
>>>> I disagree. It tells QEMU to treat the image as just a dumb blob,
>>>> instead of loading it as and ELF file. I thin force-raw makes sense as
>>>> the user is telling QEMU that the image should be treated as a raw
>>>> image, no matter what it actually is.
>>>
>>> I'm still confused then.
>>>
>>> I can see two possible features here, and based on your documentation
>>> and commentary, I can't tell which one you implemented:
>>>
>>> 0. QEMU can load raw files and ELF executable files.  Raw files are
>>> loaded verbatim at a the specified address.  ELF executable files are
>>> loaded by an ELF loader, which loads the program header table's loadable
>>> segments.
>>>
>>> 1. force-raw overrides the ELF detection, to let you load an ELF
>>> executable file verbatim, as if it was raw.
>>
>> This is what the option does. It forces the image to be treated as a raw image.
>
> Okay, makes sense now.
>
>> I'm sorry if I implied it was option 2 instead.
>
> No problem.  Suggest to clarify docs/generic-loader.txt, perhaps like
> this:

Too easy. I sent a V3 of my docs updating patch which fixes this.

It's called: docs/generic-loader: Update the document

Thanks,

Alistair

>
>     Loading Files
>     -------------
>
>     The loader device also allows files to be loaded into memory.  It
>     can load raw files and ELF executable files.  Raw files are loaded
>     verbatim.  ELF executable files are loaded by an ELF loader.  The
>     syntax is shown below:
>
> [...]
>         <force-raw> - force-raw=on forces the file to be treated as a raw
>                       image.  This can be used to load ELF files as if
>                       they were raw.
>
diff mbox

Patch

diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
new file mode 100644
index 0000000..d1f8ce3
--- /dev/null
+++ b/docs/generic-loader.txt
@@ -0,0 +1,81 @@ 
+Copyright (c) 2016 Xilinx Inc.
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+the COPYING file in the top-level directory.
+
+
+The 'loader' device allows the user to load multiple images or values into
+QEMU at startup.
+
+Loading Data into Memory Values
+---------------------
+The loader device allows memory values to be set from the command line. This
+can be done by following the syntax below:
+
+     -device loader,addr=<addr>,data=<data>,data-len=<data-len>
+                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
+
+    <addr>      - The address to store the data in.
+    <data>      - The value to be written to the address. The maximum size of
+                  the data is 8 bytes.
+    <data-len>  - The length of the data in bytes. This argument must be
+                  included if the data argument is.
+    <data-be>   - Set to true if the data to be stored on the guest should be
+                  written as big endian data. The default is to write little
+                  endian data.
+    <cpu-num>   - The number of the CPU's address space where the data should
+                  be loaded. If not specified the address space of the first
+                  CPU is used.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading value 0x8000000e to address 0xfd1a0104 is:
+    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
+
+Setting a CPU's Program Counter
+---------------------
+The loader device allows the CPU's PC to be set from the command line. This
+can be done by following the syntax below:
+
+     -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+    <addr>      - The value to use as the CPU's PC.
+    <cpu-num>   - The number of the CPU whose PC should be set to the
+                  specified value.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of setting CPU 0's PC to 0x8000 is:
+    -device loader,addr=0x8000,cpu-num=0
+
+Loading Files
+---------------------
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+
+    -device loader,file=<file>[,addr=<addr>][,cpu-num=<cpu-num>][,force-raw=<raw>]
+
+    <file>      - A file to be loaded into memory
+    <addr>      - The addr in memory that the file should be loaded. This is
+                  ignored if you are using an ELF (unless force-raw is true).
+                  This is required if you aren't loading an ELF.
+    <cpu-num>   - This specifies the CPU that should be used. This is an
+                  optional argument and will cause the CPU's PC to be set to
+                  where the image is stored or in the case of an ELF file to
+                  the value in the header. This option should only be used
+                  for the boot image.
+                  This will also cause the image to be written to the specified
+                  CPU's address space. If not specified, the default is CPU 0.
+    <force-raw> - Forces the file to be treated as a raw image. This can be
+                  used to specify the load address of ELF files.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+    -device loader,file=./images/boot.elf,cpu-num=0