Re: [Xen-devel] [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
> Currently, the "iomem" domU config option allows to specify a machine
> address range to be mapped to the domU. However, there is no way to
> specify the guest address range used for the mapping. This commit
> extends the iomem option handling code to parse an additional, optional
> parameter: this parameter, if given, specifies the start guest address
> used for the mapping; if no start guest address is given, a 1:1 mapping
> is performed as default.
> 
> Signed-off-by: Arianna Avanzini <avanzini.arianna@xxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Paolo Valente <paolo.valente@xxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>
> 
> ---
> 
>     v5:
>         - Initialize the gfn field of the iomem structure with the
>           value "(uint64_t)-1".
>         - Defer the assignment of the value of "start" to "gfn", if
>           "gfn" has been initialized to the default value, in libxl.
>         - Use a local variable to keep the return value of sscanf()
>           for better code readability.
> 
>     v4:
>         - Add definition of a LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN macro
>           to indicate the availability of the new option.
>         - Simplify the code parsing the new optional parameter by using
>           the switch construct on the return value of the sscanf() function
>           instead of calling the function twice.
>         - Add a short paragraph to the manpage about the use of the iomem
>           option to passthrough devices protected by an IOMMU.
>         - Add comments to the fields of the "iomem" structure.
> 
> ---
>  docs/man/xl.cfg.pod.5       | 13 +++++++++----
>  tools/libxl/libxl.h         | 10 ++++++++++
>  tools/libxl/libxl_create.c  |  6 ++++++
>  tools/libxl/libxl_types.idl |  7 +++++--
>  tools/libxl/xl_cmdimpl.c    | 19 ++++++++++---------
>  5 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d54f601 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,17 @@ is given in hexadecimal and may either a span e.g. 
> C<2f8-2ff>
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
>  
> -=item B<iomem=[ "IOMEM_START,NUM_PAGES", "IOMEM_START,NUM_PAGES", ... ]>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", 
> "IOMEM_START,NUM_PAGES[@GFN]", ... ]>
>  
>  Allow guest to access specific hardware I/O memory pages. B<IOMEM_START>
> -is a physical page number. B<NUM_PAGES> is the number
> -of pages beginning with B<START_PAGE> to allow access. Both values
> -must be given in hexadecimal.
> +is a physical page number. B<NUM_PAGES> is the number of pages beginning
> +with B<START_PAGE> to allow access. B<GFN> specifies the guest frame number
> +where the mapping will start in the domU's address space.

"If B<GFN> is not given then ..."

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index b2c3015..e4a1128 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

s/indicated/indicates/

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..369cd77 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> +        if (b_info->iomem[i].gfn == (uint64_t)-1)

Please can you #define something like LIBXL_INVALID_GFN and use it both
here and in the idl. The value should probably be "~(uint64_t)0" rather
than -1.

Other than those small nits:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel