Re: [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall

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

Hi Arianna,

On 25/05/14 11:51, Arianna Avanzini wrote:
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 369c3f3..14f9880 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1650,6 +1650,18 @@ int xc_domain_memory_mapping(
>       uint32_t add_mapping)
>   {
>       DECLARE_DOMCTL;
> +    xc_dominfo_t info;
> +
> +    if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
> +    {

You may retrieve the wrong domain here. xc_domain_getinfo return the 
information of the first domain with an ID >= domid (see 
XEN_DOMCTL_getdomaininfo in xen/common/domctl.c).

> +        PERROR("Could not get info for domain");
> +        return -EINVAL;
> +    }
> +    if ( !xc_core_arch_auto_translated_physmap(&info) )
> +    {
> +        PERROR("memory_mapping only available for auto-translated domains");

PERROR doesn't make sense here. Errno is not set by xc_core_arch_auto_*

Futhermore, I won't add any error message here. Otherwise, we may have 
spurious error log for PV with libxl (you are calling this function 
unconditionally in the toolstack).

> +        return 0;
> +    }
>
>       domctl.cmd = XEN_DOMCTL_memory_mapping;
>       domctl.domain = domid;
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 6aa630e..4de0fb2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1113,6 +1113,17 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>                    "failed give dom%d access to iomem range %"PRIx64"-%"PRIx64,
>                    domid, io->start, io->start + io->number - 1);
>               ret = ERROR_FAIL;
> +            continue;

It should be a "goto error_out" rather than a continue here.

I plan to send a patch fixing the other loop (see 
http://lists.xen.org/archives/html/xen-devel/2014-05/msg01831.html), so 
I'm fine if you keep as it is this part.

> +        }
> +        ret = xc_domain_memory_mapping(CTX->xch, domid,
> +                                       io->gfn, io->start,
> +                                       io->number, 1);
> +        if (ret < 0) {
> +            LOGE(ERROR,
> +                 "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64
> +                 " to guest address %"PRIx64,
> +                 domid, io->start, io->start + io->number - 1, io->gfn);
> +            ret = ERROR_FAIL;

Same remark here.

Regards,

-- 
Julien Grall