From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v2] xen/tools: Introduce QNX IFS loader
Date: Wed, 17 Sep 2014 10:59:01 -0700
Message-ID: <5419CBE5.2030205@linaro.org>
References: <1410971686-28977-1-git-send-email-oleksandr.tyshchenko@globallogic.com>
	<1410971686-28977-2-git-send-email-oleksandr.tyshchenko@globallogic.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1410971686-28977-2-git-send-email-oleksandr.tyshchenko@globallogic.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>, xen-devel@lists.xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org
List-Id: xen-devel@lists.xenproject.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