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

[Ian Campbell <ian.campbell@citrix.com>]

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)

> >> > +    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?

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?

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?

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)?

Ian.