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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

Ian.


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