Re: [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option

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

Hi Arianna,

Thank you for the patch.

On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 06bbca6..13f5fe7 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -95,6 +95,16 @@
>  #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
>  
>  /*
> + * LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN indicated that it is possible
> + * to specify the start guest frame number used to map a range of I/O
> + * memory machine frame numbers via the 'gfn' field (of type uint64)
> + * of the 'iomem' structure. An array of iomem structures is embedded
> + * in libxl_domain_build_info and used to map the indicated memory
> + * ranges during domain build.
> + */
> +#define LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..4462586 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -169,8 +169,9 @@ libxl_ioport_range = Struct("ioport_range", [
>      ])
>  
>  libxl_iomem_range = Struct("iomem_range", [
> -    ("start", uint64),
> -    ("number", uint64),
> +    ("start", uint64),  # start host frame number to be mapped to the guest
> +    ("number", uint64), # number of frames to be mapped
> +    ("gfn", uint64),    # guest frame number used as a start for the mapping
>      ])

When this structure will be used by another toolstack (e.g not xl), the
default value for gfn will be 0. This is wrong because we won't be able
to differentiate a user that will want to map to the GFN 0 and a 1:1
mapping. -1 seems the best default value for now.

You can use init_val in the IDL to set this value.

>  libxl_vga_interface_info = Struct("vga_interface_info", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 4fc46eb..99a0958 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1208,13 +1208,20 @@ static void parse_config_data(const char *config_source,
>                          "xl: Unable to get element %d in iomem list\n", i);
>                  exit(1);
>              }

It's a bug on the current code. We have to init correctly iomem before
with libxl_iomem_range_init(&b_info->iomem);

> -            if(sscanf(buf, "%" SCNx64",%" SCNx64,
> -                     &b_info->iomem[i].start,
> -                     &b_info->iomem[i].number)
> -                  != 2) {
> -               fprintf(stderr,
> -                       "xl: Invalid argument parsing iomem: %s\n", buf);
> -               exit(1);
> +            switch(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
> +                          &b_info->iomem[i].start,
> +                          &b_info->iomem[i].number,
> +                          &b_info->iomem[i].gfn)) {

I don't like the switch(sscanf(... it's hard to read.

I would prefer:

ret = sscanf(...);
switch (ret) {
}

> +                case 3: break;
> +                case 2:
> +                        /* default to 1:1 mapping */
> +                        b_info->iomem[i].gfn = b_info->iomem[i].start;
> +                        break;

If the iomem_range is initialized with the default value. You can defer
this initialization in libxl.

The result code here will be nicer:

ret = sscanf(...)

if ( ret < 2 )
  fprintf(stderr, ...);

Regards,

-- 
Julien Grall