Re: [PATCH v2 23/25] arm/altp2m: Extend libxl to activate altp2m on ARM.

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Aug 01, 2016 at 07:10:26PM +0200, Sergej Proskurin wrote:
> The current implementation allows to set the parameter HVM_PARAM_ALTP2M.
> This parameter allows further usage of altp2m on ARM. For this, we
> define an additional, common altp2m field as part of the
> libxl_domain_type struct. This field can be set on x86 and on ARM systems
> through the "altp2m" switch in the domain's configuration file (i.e.
> set altp2m=1).
> 
> Note, that the old parameter "altp2mhvm" is still valid for x86. Since
> this commit defines this old parameter as deprecated, libxl will
> generate a warning during processing.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> v2: The macro LIBXL_HAVE_ALTP2M is now valid for x86 and ARM.
> 
>     Moved the field altp2m out of info->u.pv.altp2m into the common
>     field info->altp2m, where it can be accessed independent by the
>     underlying architecture (x86 or ARM). Now, altp2m can be activated
>     by the guest control parameter "altp2m".
> 
>     Adopted initialization routines accordingly.
> ---
>  tools/libxl/libxl.h         |  3 ++-
>  tools/libxl/libxl_create.c  |  8 +++++---
>  tools/libxl/libxl_dom.c     |  4 ++--
>  tools/libxl/libxl_types.idl |  4 +++-
>  tools/libxl/xl_cmdimpl.c    | 26 +++++++++++++++++++++++++-
>  5 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 48a43ce..a2cbd34 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -839,7 +839,8 @@ typedef struct libxl__ctx libxl_ctx;
>  
>  /*
>   * LIBXL_HAVE_ALTP2M
> - * If this is defined, then libxl supports alternate p2m functionality.
> + * If this is defined, then libxl supports alternate p2m functionality for
> + * x86 HVM and ARM PV guests.
>   */
>  #define LIBXL_HAVE_ALTP2M 1

Either you misunderstood or I said something wrong.

These macros have defined semantics that shouldn't be changed because
application code uses them to detect the presence / absence of certain
things.

We need a new macro for ARM altp2m. I think

   LIBXL_HAVE_ARM_ALTP2M

would do.

>  
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d7db9e9..16d3b52 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -319,7 +319,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
>          libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
>          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
> -        libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
>  
> @@ -406,6 +405,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>              libxl_domain_type_to_string(b_info->type));
>          return ERROR_INVAL;
>      }
> +
> +    libxl_defbool_setdefault(&b_info->altp2m, false);
> +
>      return 0;
>  }
>  
> @@ -901,8 +903,8 @@ static void initiate_domain_create(libxl__egc *egc,
>  
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
> -         libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
> -        LOG(ERROR, "nestedhvm and altp2mhvm cannot be used together");
> +         libxl_defbool_val(d_config->b_info.altp2m))) {
> +        LOG(ERROR, "nestedhvm and altp2m cannot be used together");
>          goto error_out;
>      }
>  
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index ec29060..1550ef8 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -291,8 +291,6 @@ static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
>                      libxl_defbool_val(info->u.hvm.vpt_align));
>      xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
>                      libxl_defbool_val(info->u.hvm.nested_hvm));
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2M,
> -                    libxl_defbool_val(info->u.hvm.altp2m));
>  }
>  
>  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> @@ -434,6 +432,8 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>  #endif
>      }
>  
> +    xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M, libxl_defbool_val(info->altp2m));
> +

And the reason for moving this call to this function is?

>      rc = libxl__arch_domain_create(gc, d_config, domid);
>  
>      return rc;
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index ef614be..42e7c95 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -512,7 +512,6 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("mmio_hole_memkb",  MemKB),
>                                         ("timer_mode",       libxl_timer_mode),
>                                         ("nested_hvm",       libxl_defbool),
> -                                       ("altp2m",           libxl_defbool),

No, you can't remove existing field -- that would break old
applications which use the old field.

And please handle compatibility in libxl with old applications in mind.

>                                         ("smbios_firmware",  string),
>                                         ("acpi_firmware",    string),
>                                         ("hdtype",           libxl_hdtype),
> @@ -561,6 +560,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                ])),
> +    # Alternate p2m is not bound to any architecture or guest type, as it is
> +    # supported by x86 HVM and ARM PV guests.

Just "ARM guests" would do. ARM doesn't have notion of PV vs HVM.

> +    ("altp2m", libxl_defbool),
>  
>      ], dir=DIR_IN
>  )
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 51dc7a0..f4a49ee 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1667,7 +1667,12 @@ static void parse_config_data(const char *config_source,
>  
>          xlu_cfg_get_defbool(config, "nestedhvm", &b_info->u.hvm.nested_hvm, 0);
>  
> -        xlu_cfg_get_defbool(config, "altp2mhvm", &b_info->u.hvm.altp2m, 0);
> +        /* The config parameter "altp2mhvm" is considered deprecated, however
> +         * further considered because of legacy reasons. The config parameter
> +         * "altp2m" shall be used instead. */
> +        if (!xlu_cfg_get_defbool(config, "altp2mhvm", &b_info->altp2m, 0))
> +            fprintf(stderr, "WARNING: Specifying \"altp2mhvm\" is deprecated. "
> +                    "Please use a \"altp2m\" instead.\n");

In this case you should:

 if both altp2mhvm and altp2m are present, use the latter.
 if only altp2mhvm is present, honour it.

Note that we have not yet removed the old option. Ideally we would give
users a transition period before removing the option.

Also you need to patch docs/man/xl.pod.1.in for the new option.

>  
>          xlu_cfg_replace_string(config, "smbios_firmware",
>                                 &b_info->u.hvm.smbios_firmware, 0);
> @@ -1727,6 +1732,25 @@ static void parse_config_data(const char *config_source,
>          abort();
>      }
>  
> +    bool altp2m_support = false;
> +#if defined(__i386__) || defined(__x86_64__)
> +    /* Alternate p2m support on x86 is available only for HVM guests. */
> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
> +        altp2m_support = true;
> +#elif defined(__arm__) || defined(__aarch64__)
> +    /* Alternate p2m support on ARM is available for all guests. */
> +    altp2m_support = true;
> +#endif
> +

I don't think you need to care too much about dead option here.
Xl should be able to set altp2m field all the time. And there should be
code in libxl to handle situation when altp2m is not available.

> +    if (altp2m_support) {
> +        /* The config parameter "altp2m" replaces the parameter "altp2mhvm".
> +         * For legacy reasons, both parameters are accepted on x86 HVM guests
> +         * (only "altp2m" is accepted on ARM guests). If both parameters are
> +         * given, it must be considered that the config parameter "altp2m" will
> +         * always have priority over "altp2mhvm". */
> +        xlu_cfg_get_defbool(config, "altp2m", &b_info->altp2m, 0);
> +    }
> +

As always, if what I said above doesn't make sense to you, feel free to
ask.

Wei.

>      if (!xlu_cfg_get_list(config, "ioports", &ioports, &num_ioports, 0)) {
>          b_info->num_ioports = num_ioports;
>          b_info->ioports = calloc(num_ioports, sizeof(*b_info->ioports));
> -- 
> 2.9.0
> 

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