Re: [Qemu-devel] [PATCH v12 2/2] docs: Add a generic loader explanation document

[Markus Armbruster]

Alistair Francis <address@hidden> writes:

> On Tue, Oct 4, 2016 at 12:56 AM, Markus Armbruster <address@hidden> wrote:
>> Alistair Francis <address@hidden> writes:
>>
>>> On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster <address@hidden> wrote:
>>>> Alistair Francis <address@hidden> writes:
>>>>
>>>>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster <address@hidden> wrote:
>>>>>> Alistair Francis <address@hidden> writes:
>>>>>>
>>>>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>>>>> Reviewed-by: Peter Maydell <address@hidden>
>>>>>>> ---
>>>>>>> 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.