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

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

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