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

[Julien Grall <julien.grall@linaro.org>]

Hi Oleksandr,

Thank you for your patch.

On 19/08/14 10:51, Oleksandr Tyshchenko wrote:
> Add ability to load QNX IFS image. Based on IPL code (U-Boot for QNX).

Do you have any link to the IPL code? Such as commit ID...

Also, is there any website to explain the QNX IFS image?

[..]

> diff --git a/tools/libxc/xc_dom_qnxifsloader.c b/tools/libxc/xc_dom_qnxifsloader.c

[..]

> +static uint32_t image_addr;

All the below functions can be called concurrently for different 
domains. I would add a new field in xc_dom_image.

> +static uint32_t image_scan(uint32_t start_addr, uint32_t end_addr)
> +{
> +    struct startup_header *startup_hdr;
> +    uint32_t last_addr = 0xffffffff;
> +
> +    for (; start_addr < end_addr; start_addr += 4)

Coding style:

for ( ... )

> +    {
> +        startup_hdr = (struct startup_header *)start_addr;

AFAIU, QNX loader is only for 32 bits guest, right?

You are casting on multiple place 32 bits address to a pointer. This 
won't work on ARM64. Even though you are targeting 32 bits, you still 
have to make this code working on ARM64.

[..]

> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
> +{
> +    struct startup_header *startup_hdr;
> +
> +    if ( dom->kernel_blob == NULL )
> +    {
> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                     "%s: no QNX IFS loaded", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    image_addr = *(uint32_t *)&dom->kernel_blob;
> +    image_addr = image_scan(image_addr, image_addr + 0x200);
> +    if (image_addr == 0xffffffff)

Coding style:

if ( ... )

Regards,

-- 
Julien Grall