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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

I have tried loader on Xen 4.5 (with RAM base address 0x40000000).
All works as expected. When I tried to load QNX IFS with "wrong" RAM
base (0x80000000) a got a error from loader (I added a check for valid
entry point). When I changed RAM base to 0x40000000 I saw that QNX
booted.

If you don't mind I would like to push patch ver 3 where all (I hope)
previous comments were addressed.

On Mon, Sep 22, 2014 at 5:23 PM, Oleksandr Tyshchenko
<oleksandr.tyshchenko@xxxxxxxxxxxxxxx> wrote:
> 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



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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