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

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

[Oleksandr Tyshchenko]

The following patch adds ability to load OS image filesystem.
The patch was developed according to instruction:
http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
and tested on omap5_uevm/dra7xx_evm boards with compressed/uncompressed images.
The image was downloaded from:
http://community.qnx.com/sf/wiki/do/viewPage/projects.bsp/wiki/TexasInstrumentsOMAP5432EVM
and modified to run on top of XEN.

Changes in v02:
- Rewrite code and changed license to LGPL
- Addressed Julien's technical review comments
- Added image validatation step by performing a checksum calculation

Oleksandr Tyshchenko (1):
  xen/tools: Introduce QNX IFS loader

 tools/libxc/Makefile              |   1 +
 tools/libxc/xc_dom.h              |   3 +
 tools/libxc/xc_dom_qnxifsloader.c | 199 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 tools/libxc/xc_dom_qnxifsloader.c

-- 
1.9.1

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

[Oleksandr Tyshchenko]

Add ability to load QNX IFS image. The loader probe function
based on "Writing an IPL Program" howto from qnx.com and performs
image validation and startup entry point location.
Because of the fact that the image is already in place some
customizations have been done. Also in case of multiple images
(very seldom case) only the first image will be loaded.

Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
---
 tools/libxc/Makefile              |   1 +
 tools/libxc/xc_dom.h              |   3 +
 tools/libxc/xc_dom_qnxifsloader.c | 199 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 tools/libxc/xc_dom_qnxifsloader.c

diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 2cca2b2..1462f53 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -66,6 +66,7 @@ GUEST_SRCS-y                 += xc_dom_elfloader.c
 GUEST_SRCS-$(CONFIG_X86)     += xc_dom_bzimageloader.c
 GUEST_SRCS-$(CONFIG_X86)     += xc_dom_decompress_lz4.c
 GUEST_SRCS-$(CONFIG_ARM)     += xc_dom_armzimageloader.c
+GUEST_SRCS-$(CONFIG_ARM)     += xc_dom_qnxifsloader.c
 GUEST_SRCS-y                 += xc_dom_binloader.c
 GUEST_SRCS-y                 += xc_dom_compat_linux.c
 
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 92db8d0..bf9ea3c 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -151,6 +151,9 @@ struct xc_dom_image {
     struct xc_dom_arch *arch_hooks;
     /* allocate up to virt_alloc_end */
     int (*allocate) (struct xc_dom_image * dom, xen_vaddr_t up_to);
+
+    /* qnx loader */
+    uint32_t startup_vaddr;
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_qnxifsloader.c b/tools/libxc/xc_dom_qnxifsloader.c
new file mode 100644
index 0000000..3137897
--- /dev/null
+++ b/tools/libxc/xc_dom_qnxifsloader.c
@@ -0,0 +1,199 @@
+/*
+ * Xen domain builder -- QNX IFS bits
+ *
+ * Parse and load QNX IFS image.
+ *
+ * Copyright (C) 2014, Globallogic.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <inttypes.h>
+
+#include "xg_private.h"
+#include "xc_dom.h"
+
+struct startup_header {
+    uint32_t signature;        /* Header sig */
+    uint16_t version;          /* Header vers */
+    uint8_t flags1;            /* Misc flags */
+    uint8_t flags2;            /* No flags defined yet */
+    uint16_t header_size;      /* sizeof(struct startup_header) */
+    uint16_t machine;          /* Machine type */
+    uint32_t startup_vaddr;    /* Virtual Address to transfer */
+                               /* to after IPL is done */
+    uint32_t paddr_bias;       /* Value to add to physical address */
+                               /* to get a value to put into a */
+                               /* pointer and indirected through */
+    uint32_t image_paddr;      /* Physical address of image */
+    uint32_t ram_paddr;        /* Physical address of RAM to copy */
+                               /* image to (startup_size bytes copied) */
+    uint32_t ram_size;         /* Amount of RAM used by the startup */
+                               /* program and executables contained */
+                               /* in the file system */
+    uint32_t startup_size;     /* Size of startup (never compressed) */
+    uint32_t stored_size;      /* Size of entire image */
+    uint32_t imagefs_paddr;    /* Set by IPL to where the imagefs is */
+                               /* when startup runs */
+    uint32_t imagefs_size;     /* Size of uncompressed imagefs */
+    uint16_t preboot_size;     /* Size of loaded before header */
+    uint16_t zero0;            /* Zeros */
+    uint32_t zero[3];          /* Zeros */
+    uint32_t info[48];         /* Array of startup_info* structures */
+};
+
+#define STARTUP_HDR_SIGNATURE    0x00ff7eeb
+
+static int calc_checksum(uint32_t addr, uint32_t size)
+{
+    int sum = 0;
+    uint32_t *ptr = (uint32_t *)addr;
+
+    while ( size > 0 )
+    {
+        sum += *ptr++;
+        size -= 4;
+    }
+
+    return sum;
+}
+
+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 */
+    start_addr = *(uint32_t *)&dom->kernel_blob;
+    end_addr = start_addr + 0x1000;
+    while ( start_addr < end_addr )
+    {
+        startup_hdr = (struct startup_header *)(uintptr_t)start_addr;
+        if ( startup_hdr->signature == STARTUP_HDR_SIGNATURE )
+            break;
+        start_addr += 4;
+	}
+
+    if ( start_addr >= end_addr )
+    {
+        xc_dom_printf(dom->xch, "%s: image is not a QNX IFS", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    /* Performs a checksums on the startup and the OS image filesystem */
+    if ( (calc_checksum(start_addr, startup_hdr->startup_size) != 0) ||
+         (calc_checksum(start_addr + startup_hdr->startup_size,
+          startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
+    {
+        xc_dom_printf(dom->xch, "%s: QNX IFS has wrong checksum", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    if ( (startup_hdr->stored_size + startup_hdr->preboot_size) != dom->kernel_size )
+    {
+        xc_dom_printf(dom->xch, "%s: QNX IFS has wrong size", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    dom->startup_vaddr = startup_hdr->startup_vaddr;
+
+    return 0;
+}
+
+static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
+{
+    uint64_t v_start, v_end;
+    uint64_t rambase = GUEST_RAM_BASE;
+
+    DOMPRINTF_CALLED(dom->xch);
+
+    dom->rambase_pfn = rambase >> XC_PAGE_SHIFT;
+
+    /* Do not load kernel at the very first RAM address */
+    v_start = rambase + 0x8000;
+    v_end = v_start + dom->kernel_size;
+
+    /* find kernel segment */
+    dom->kernel_seg.vstart = v_start;
+    dom->kernel_seg.vend   = v_end;
+
+    dom->parms.virt_entry = dom->startup_vaddr;
+    dom->parms.virt_base = rambase;
+
+    dom->guest_type = "xen-3.0-armv7l";
+    DOMPRINTF("%s: %s: RAM starts at %"PRI_xen_pfn,
+              __FUNCTION__, dom->guest_type, dom->rambase_pfn);
+    DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
+              __FUNCTION__, dom->guest_type,
+              dom->kernel_seg.vstart, dom->kernel_seg.vend);
+
+    return 0;
+}
+
+static int xc_dom_load_qnx_ifs(struct xc_dom_image *dom)
+{
+    void *dst;
+
+    DOMPRINTF_CALLED(dom->xch);
+
+    dst = xc_dom_seg_to_ptr(dom, &dom->kernel_seg);
+    if ( dst == NULL )
+    {
+        DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->kernel_seg) => NULL",
+                  __func__);
+        return -1;
+    }
+
+    DOMPRINTF("%s: kernel seg %#"PRIx64"-%#"PRIx64,
+              __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend);
+    DOMPRINTF("%s: copy %zd bytes from blob %p to dst %p",
+              __func__, dom->kernel_size, dom->kernel_blob, dst);
+
+    memcpy(dst, dom->kernel_blob, dom->kernel_size);
+
+    return 0;
+}
+
+static struct xc_dom_loader qnx_ifs_loader = {
+    .name = "QNX IFS",
+    .probe = xc_dom_probe_qnx_ifs,
+    .parser = xc_dom_parse_qnx_ifs,
+    .loader = xc_dom_load_qnx_ifs,
+};
+
+static void __init register_loader(void)
+{
+    xc_dom_register_loader(&qnx_ifs_loader);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1

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

[Julien Grall]

Hi Oleksandr,

On 17/09/14 09:34, Oleksandr Tyshchenko wrote:
> +#define STARTUP_HDR_SIGNATURE    0x00ff7eeb
> +
> +static int calc_checksum(uint32_t addr, uint32_t size)
> +{
> +    int sum = 0;
> +    uint32_t *ptr = (uint32_t *)addr;

You are casting an uint32_t address to a pointer, this will break 
compilation on ARM64.

> +
> +    while ( size > 0 )
> +    {
> +        sum += *ptr++;
> +        size -= 4;
> +    }
> +
> +    return sum;
> +}
> +
> +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 */
> +    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.

[..]

> +static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
> +{
> +    uint64_t v_start, v_end;
> +    uint64_t rambase = GUEST_RAM_BASE;
> +
> +    DOMPRINTF_CALLED(dom->xch);
> +
> +    dom->rambase_pfn = rambase >> XC_PAGE_SHIFT;

dom->rambase_pfn is already set earlier (see xc_dom_rambase_init). You 
don't need to reset it.

Please you the value of this variable here, rather than using the 
hardcoding GUEST_RAM_BASE.

> +    /* Do not load kernel at the very first RAM address */
> +    v_start = rambase + 0x8000;
> +    v_end = v_start + dom->kernel_size;
> +
> +    /* find kernel segment */
> +    dom->kernel_seg.vstart = v_start;
> +    dom->kernel_seg.vend   = v_end;
> +
> +    dom->parms.virt_entry = dom->startup_vaddr;

Looking to the QNX header, the field start_vaddr should contain a 
virtual address.

But virt_entry will contain that entry physical address (yes, I know 
it's confusing :)).

So, how can this work?

Also, it looks like that with this solution, the QNX image will be tight 
to a specific version of Xen. Indeed, you have to specify the physical 
address in the QNX image.

-- 
Julien Grall

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

[Ian Campbell]

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.

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

The code also needs to take more care not to run off the end of the
kernel image, e.g. a maliciously short one, or one with a malicious
start_addr.

It also needs to not trust any of the values read from the header and
range check them all etc. The patches from XSA-95 have some examples of
the sorts of checks which are needed for this sort of thing, plus the
zImage loader generally ought to serve as an example.

Ian.

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

[Oleksandr Tyshchenko]

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.

>
>> > +    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 that value caused by assumption that this can be multiple OS
images (so, the image may have many headers).

>
> The code also needs to take more care not to run off the end of the
> kernel image, e.g. a maliciously short one, or one with a malicious
> start_addr.
ok, I will add a check

>
> It also needs to not trust any of the values read from the header and
> range check them all etc. The patches from XSA-95 have some examples of
> the sorts of checks which are needed for this sort of thing, plus the
> zImage loader generally ought to serve as an example.
ok, I will think about it

>
> Ian.
>

-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

[Ian Campbell]

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.

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

[Oleksandr Tyshchenko]

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

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

[Ian Campbell]

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@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).

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.

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

[Oleksandr Tyshchenko]

Hi Ian

On Mon, Sep 22, 2014 at 4:09 PM, Ian Campbell <Ian.Campbell@citrix.com> 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@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).
>
> 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

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

[Oleksandr Tyshchenko]

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@globallogic.com> wrote:
> Hi Ian
>
> On Mon, Sep 22, 2014 at 4:09 PM, Ian Campbell <Ian.Campbell@citrix.com> 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@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).
>>
>> 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

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

[Ian Campbell]

On Mon, 2014-09-22 at 18:09 +0300, Oleksandr Tyshchenko wrote:
> 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.

I don't mind at all, please go for it!

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

[Oleksandr Tyshchenko]

On Wed, Sep 17, 2014 at 8:59 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 17/09/14 09:34, Oleksandr Tyshchenko wrote:
>>
>> +#define STARTUP_HDR_SIGNATURE    0x00ff7eeb
>> +
>> +static int calc_checksum(uint32_t addr, uint32_t size)
>> +{
>> +    int sum = 0;
>> +    uint32_t *ptr = (uint32_t *)addr;
>
>
> You are casting an uint32_t address to a pointer, this will break
> compilation on ARM64.
ok

>
>
>> +
>> +    while ( size > 0 )
>> +    {
>> +        sum += *ptr++;
>> +        size -= 4;
>> +    }
>> +
>> +    return sum;
>> +}
>> +
>> +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 */
>> +    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.
ok

>
> [..]
>
>> +static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
>> +{
>> +    uint64_t v_start, v_end;
>> +    uint64_t rambase = GUEST_RAM_BASE;
>> +
>> +    DOMPRINTF_CALLED(dom->xch);
>> +
>> +    dom->rambase_pfn = rambase >> XC_PAGE_SHIFT;
>
>
> dom->rambase_pfn is already set earlier (see xc_dom_rambase_init). You don't
> need to reset it.
>
> Please you the value of this variable here, rather than using the hardcoding
> GUEST_RAM_BASE.
ok

>
>> +    /* Do not load kernel at the very first RAM address */
>> +    v_start = rambase + 0x8000;
>> +    v_end = v_start + dom->kernel_size;
>> +
>> +    /* find kernel segment */
>> +    dom->kernel_seg.vstart = v_start;
>> +    dom->kernel_seg.vend   = v_end;
>> +
>> +    dom->parms.virt_entry = dom->startup_vaddr;
>
>
> Looking to the QNX header, the field start_vaddr should contain a virtual
> address.
>
> But virt_entry will contain that entry physical address (yes, I know it's
> confusing :)).
>
> So, how can this work?
>
> Also, it looks like that with this solution, the QNX image will be tight to
> a specific version of Xen. Indeed, you have to specify the physical address
> in the QNX image.

I just specify the starting address in buildfile which shiped with
BSP. The starting address is the base address of the image.
In our case we have next image attribute:
[image=0x80008000]
This is one of the required actions for QNX to load on top of XEN.
Yes, if the rambase addr changes in XEN, I will need to change the
image attr in QNX.

>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

[Julien Grall]

Hi Oleksandr,

On 18/09/14 03:55, Oleksandr Tyshchenko wrote:
>> Also, it looks like that with this solution, the QNX image will be tight to
>> a specific version of Xen. Indeed, you have to specify the physical address
>> in the QNX image.
>
> I just specify the starting address in buildfile which shiped with
> BSP. The starting address is the base address of the image.
> In our case we have next image attribute:
> [image=0x80008000]

This RAM base address has changed in Xen 4.5. I suspect you are trying 
this patch on top of Xen 4.4, right?

If so, it would be nice if you can try this patch also on Xen 4.5.

> This is one of the required actions for QNX to load on top of XEN.
> Yes, if the rambase addr changes in XEN, I will need to change the
> image attr in QNX.

Is it possible to make QNX position independent?

If not, I would add a check that the load address is effectively part of 
the RAM and throw an error if it's not the case.

Regards,

-- 
Julien Grall

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

[Oleksandr Tyshchenko]

On Thu, Sep 18, 2014 at 10:11 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 18/09/14 03:55, Oleksandr Tyshchenko wrote:
>>>
>>> Also, it looks like that with this solution, the QNX image will be tight
>>> to
>>> a specific version of Xen. Indeed, you have to specify the physical
>>> address
>>> in the QNX image.
>>
>>
>> I just specify the starting address in buildfile which shiped with
>> BSP. The starting address is the base address of the image.
>> In our case we have next image attribute:
>> [image=0x80008000]
>
>
> This RAM base address has changed in Xen 4.5. I suspect you are trying this
> patch on top of Xen 4.4, right?
Right.
I know about RAM base address, I keep in mind it.
As I know there are other significant differences between these versions.

>
> If so, it would be nice if you can try this patch also on Xen 4.5.
Sure, I will try.

>
>> This is one of the required actions for QNX to load on top of XEN.
>> Yes, if the rambase addr changes in XEN, I will need to change the
>> image attr in QNX.
>
>
> Is it possible to make QNX position independent?
Unfortunately, I was trying but I couldn't make.

More detailed:
we have base address of the image: [image=0x80008000]
startup_vaddr is based on "base address"
domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x800085bc

I dropped base address from IFS buildfile:
domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x000005bc
and tried to do next thing:
dom->parms.virt_entry = v_start + startup_hdr->startup_vaddr;
But this didn't work. QNX didn't boot.

Perhaps, the QNX uses this image attribute for his own purposes
(and checks somethings at early stages).
Need to investigate more.

>
> If not, I would add a check that the load address is effectively part of the
> RAM and throw an error if it's not the case.
Yes.
Something like this:

    /* Performs a sanity check for a valid startup entry point */
    if ( (dom->startup_vaddr < v_start) || (dom->startup_vaddr > v_end) )
    {
        xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup entry
point", __FUNCTION__);
        return -EINVAL;
    }

Are you OK with it?

>
> Regards,
>
> --
> Julien Grall

Thank you

-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

[Ian Campbell]

On Fri, 2014-09-19 at 19:45 +0300, Oleksandr Tyshchenko wrote:

> > Is it possible to make QNX position independent?
> Unfortunately, I was trying but I couldn't make.
> 
> More detailed:
> we have base address of the image: [image=0x80008000]
> startup_vaddr is based on "base address"
> domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x800085bc
> 
> I dropped base address from IFS buildfile:
> domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x000005bc
> and tried to do next thing:
> dom->parms.virt_entry = v_start + startup_hdr->startup_vaddr;
> But this didn't work. QNX didn't boot.

There's more to making something position independent than just dropping
the base address, it needs the code in question to be carefully
written/compiled to be position independent, at least up to the point
where it controls its own virtual address space mappings.

e.g. Xen on ARM is very careful throughout xen/arch/arm/*/head.S to
handle this, once we hit C we are running on our page tables and can
assume a 2MB base address.

I'd not be especially surprised if qnx, which is targeting the embedded
space, had no support for being built as a position independent binary.

The upshot of that is that I expect you would need to do a bunch of core
QNX work to make it position independent, or more likely you would
accept that a given QNX binary may only run on one version of Xen and
upgrading Xen would have knock on affects on your guest VMs (given your
market I don't expect you will be upgrading Xen in the field, so perhaps
you are fine with this).

Ian.

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

[Oleksandr Tyshchenko]

Hi Ian

On Mon, Sep 22, 2014 at 4:03 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-09-19 at 19:45 +0300, Oleksandr Tyshchenko wrote:
>
>> > Is it possible to make QNX position independent?
>> Unfortunately, I was trying but I couldn't make.
>>
>> More detailed:
>> we have base address of the image: [image=0x80008000]
>> startup_vaddr is based on "base address"
>> domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x800085bc
>>
>> I dropped base address from IFS buildfile:
>> domainbuilder: detail: xc_dom_probe_qnx_ifs: startup_vaddr: 0x000005bc
>> and tried to do next thing:
>> dom->parms.virt_entry = v_start + startup_hdr->startup_vaddr;
>> But this didn't work. QNX didn't boot.
>
> There's more to making something position independent than just dropping
> the base address, it needs the code in question to be carefully
> written/compiled to be position independent, at least up to the point
> where it controls its own virtual address space mappings.
>
> e.g. Xen on ARM is very careful throughout xen/arch/arm/*/head.S to
> handle this, once we hit C we are running on our page tables and can
> assume a 2MB base address.
>
> I'd not be especially surprised if qnx, which is targeting the embedded
> space, had no support for being built as a position independent binary.
>
> The upshot of that is that I expect you would need to do a bunch of core
> QNX work to make it position independent, or more likely you would
> accept that a given QNX binary may only run on one version of Xen and
> upgrading Xen would have knock on affects on your guest VMs (given your
> market I don't expect you will be upgrading Xen in the field, so perhaps
> you are fine with this).
It is clear. For now, I OK with it. The loader must to throw an error
if QNX IFS has wrong hardcoded address.

>
> Ian.
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

[Julien Grall]

Hi Oleksandr,

Thank your for this series.

On 17/09/14 09:34, Oleksandr Tyshchenko wrote:
> The following patch adds ability to load OS image filesystem.
> The patch was developed according to instruction:
> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html

I think this link would be useful in the commit message for the other 
developers. Can you add it?

Regards,

-- 
Julien Grall

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

[Oleksandr Tyshchenko]

On Wed, Sep 17, 2014 at 8:36 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Oleksandr,
Hi Julien

>
> Thank your for this series.
>
> On 17/09/14 09:34, Oleksandr Tyshchenko wrote:
>>
>> The following patch adds ability to load OS image filesystem.
>> The patch was developed according to instruction:
>>
>> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
>
>
> I think this link would be useful in the commit message for the other
> developers. Can you add it?
sure

>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com