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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

Hi Ian

On Mon, Sep 22, 2014 at 4:09 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Fri, 2014-09-19 at 18:45 +0300, Oleksandr Tyshchenko wrote:
>> Hi Ian
>>
>> On Thu, Sep 18, 2014 at 8:39 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> 
>> 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@xxxxxxxxxx> 
>> >> 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).
>
> I prefer this binary.boot approach with no searching. The benefits of
> the raw.boot thing don't really apply to us because we have an
> "intelligent" bootloader (aka domain builder) which will (after your
> patch) understand the IFS format sufficiently well enough to do the
> right thing. It seems to me that raw.boot is really a workaround for
> bootloaders which do not understand IFS.
>
>> 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;
>
> So it is, I misread it the first time around.
>
> In general I would prefer something like the second, perhaps with an
> uintptr_t in there somewhere, or even better make start_addr a "uint32_t
> *" (and adjust the associated maths/increments).
>
> Or best still, as discussed above, drop this searching stuff and use
> binary.boot instead.
OK, I agree with all your comments. I will rewrite.

>
> Ian.
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel