Re: [Xen-devel] [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Sat, 2014-03-15 at 21:11 +0100, 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 config option 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>
> ---
>  docs/man/xl.cfg.pod.5       |  7 ++++---
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    | 22 +++++++++++++++-------
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d9684f2 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,13 @@ 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.
> +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. All of these values must be given in hexadecimal.
>  
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 612645c..f0bdb09 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
>  libxl_iomem_range = Struct("iomem_range", [
>      ("start", uint64),
>      ("number", uint64),
> +    ("gfn", uint64),

The existing name "start" is unfortunate since it doesn't really
indicate what it is the start of. Can you please add comments to "gfn"
and "start" (python syntax so # comment after each is fine) to clarify
that one is the host address and the other is the guest address.

>      ])
>  
>  libxl_vga_interface_info = Struct("vga_interface_info", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 6b1ebfa..cf8d71f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1220,13 +1220,21 @@ static void parse_config_data(const char 
> *config_source,
>                          "xl: Unable to get element %d in iomem list\n", i);
>                  exit(1);
>              }
> -            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);
> +            if(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
> +                      &b_info->iomem[i].start,
> +                      &b_info->iomem[i].number,
> +                      &b_info->iomem[i].gfn)
> +                  != 3) {
> +                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);

sscanf returns the number of entries it successfully matched and
assigned, so 
        sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
will return 2 or 3 depending on what it found. So I think you can
simplify this by switching on the return value of sscanf rather than
reparsing it.

        swtich (sscanf(...)) {
                case 3: break;
                case 2:
                        ...[i].gfn = ...[i].start;
                        break;
                default:
                        fpritnf();
                        exit
        }
(you get the idea)

Ian.


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