Re: [PATCH v2] xen/tools: Introduce QNX IFS loader

[Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>]

Hi Ian

On Thu, Sep 18, 2014 at 8:39 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2014-09-18 at 16:16 +0300, Oleksandr Tyshchenko wrote:
>> Hi Ian
>>
>> On Thu, Sep 18, 2014 at 4:50 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > On Wed, 2014-09-17 at 10:59 -0700, Julien Grall wrote:
>> >> > +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
>> >> > +{
>> >> > +    struct startup_header *startup_hdr;
>> >> > +    uint32_t start_addr, end_addr;
>> >> > +
>> >> > +    if ( dom->kernel_blob == NULL )
>> >> > +    {
>> >> > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>> >> > +                     "%s: no QNX IFS loaded", __FUNCTION__);
>> >> > +        return -EINVAL;
>> >> > +    }
>> >> > +
>> >> > +    /* Scan 4KB boundaries for the valid OS signature */
>> >
>> > Is this correct? You appear to be scanning at 4 byte boundaries over a
>> > range of 4K.
>> ok.
>> I meant that the code below looks on 4 KB boundaries for the image
>> identifier bytes.
>
> Are you sure it does? It seems to search x..x+0x1000 in 4 byte
> increments. Perhaps you meant "4 byte boundaries"? (that seems to
> correlate with what you say below)
exactly.

>
>> >> > +    start_addr = *(uint32_t *)&dom->kernel_blob;
>> >> > +    end_addr = start_addr + 0x1000;
>> >>
>> >> I took me a couple of minutes to understand where does the "0x1000"
>> >> comes from. I would use "4 << 10" here.
>> >
>> > That's definitely not an improvement.
>> >
>> > PAGE_SIZE might be.
>> ok, but this does not correlate to a page meaning, just identical sizes
>>
>> I would like to say a bit more about this scanning procedure:
>> We need to scan because we have N raw bytes (preboot_size) from the
>> beginning of the image to the startup header,
>> where "startup_vaddr" is located (we have to obtain this entry
>> address). Image starts on 4 byte boundary.
>> This "preboot_size" value depends on how the IFS is created
>> (attributes in buildfile). The image could or couldn't have these N
>> raw bytes.
>> In our case we have only 8 raw bytes with next attributes:
>> [virtual=armle-v7,raw +compress]
>> Although I set ranges to 4 KB as it was mentioned in howto, I do not
>> think that "preboot_size" can be comparable with such size.
>
> I think you are saying that the 4KB limit is just some arbitrary value
> you picked (perhaps with guidance from the HOWTO), is that right?
Yes, it is.

>
> Are these N bytes completely raw or do they have some sort of structure
> to them? IOW could you parse them enough to walk over them rather than
> searching?
I think that I couldn't. These N bytes are completely raw (hereinafter
- "preboot").
When I was trying to answer to your question, I found out why this
"preboot" is needed)

The "preboot" (if present) can contains a small piece of code. The
mkifs utility lets us to create different type of images, so control
"preboot" too)

>From mkifs utility description:
raw.boot
Create a binary image with an instruction sequence at its beginning to
jump to the offset of startup_vaddr within the startup header. The
advantage is that when you download a raw image to memory using a
bootloader, you can then instruct it to run right at the beginning of
the image, rather than having to figure out what the actual
startup_vaddr is each time you modify the startup code.

binary.boot
Create a simple binary image (without the jump instruction that
raw.boot adds). If you build a binary image, and you want to load it
with U-Boot (or some other bootloader), you have to execute mkifs
-vvvv buildfile imagefile, so that you can see what the actual entry
address is, and then pass that entry address to the bootloader when
you start the image. If you modify the startup code, the entry address
may change, so you have to obtain it every time. With a raw image, you
can just have the bootloader jump to the same address that you
downloaded the image to.

As I said above we are using next attr:
[virtual=armle-v7,raw +compress]
It is "raw.boot" case. And as result we have "preboot" = 8 bytes.
I have done some experiments:
1. I dropped all in probe func (there is no need to obtain
startup_vaddr) and passed v_start to dom->parms.virt_entry in parse
func. Result -> QNX loaded (very good).
2. I rebuild IFS with "binary.boot". And as result we don't have
"preboot". Nothing is located before startup header. I dropped only
searching in probe func
(I cast dom->kernel_blob to header as it was done in zimage loader).
Result -> QNX loaded (expected).

Yes, after this knowledge we can impose restrictions how to build IFS
and simplify loader in XEN (drop searching/header structure, etc).
>From my point of view it would be nice to leave searching and not rely
how the IFS was created (to make loader more universal). The
"raw.boot" feature has disadvantage: in case of, for example, invalid
startup_vaddr or corrupted image we will not see any errors in
console.

>
> You seem to start your search at an offset which is read from the first
> 4 bytes, is that right? How does that fit in with what you say above?
No, it isn't. I start search at address "dom->kernel_blob" pointed to.
But, for simplification "scanning procedure" I convert a pointer to integer:
start_addr = *(uint32_t *)&dom->kernel_blob;
That's better):
start_addr = (uint32_t)dom->kernel_blob;

>
> I must say that this seems like a very odd file format, but that's not
> your fault...
>
>> I think that value caused by assumption that this can be multiple OS
>> images (so, the image may have many headers).
>
> Are there some simplifying assumption, such as not supporting multiple
> OS images in this way, which we can make (and document)?
I think that it is an additional overhead, to scan all range at every
load trying to find newest version of image.
Also I've seen the comment that it's uncommon nowadays to store two
version of IFS in the same media).
I need a time to think more about it.

>
> Ian.
>

Thank you


-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com