| Message ID | 20160801171028.11615-24-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
Hi Wei, On 08/02/2016 01:59 PM, Wei Liu wrote: > 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. Sorry, this is entirely my fault. Although I have explicitly asked whether we need two different LIBXL_HAVE_* macros, I somehow omitted that one. I will fix that right now and provide two LIBXL_HAVE_* macros in the next patch. >> >> 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? Since this implementation removes the field info->u.hvm.altp2m and rather uses the common field info->altp2m, I wanted to set the altp2m parameter outside of a function that is associated with an HVM domain (as the function hvm_set_conf_params is called only if the field info->type == LIBXL_DOMAIN_TYPE_HVM). The idea was to have only one call to xc_hvm_param_set independent of the domain type, as we do not distinguish between the underlying architecture anymore.If you believe that we nevertheless need two calls in the code, I will move the function call in question back to hvm_set_conf_params and add an additional call to xc_hvm_param_set for the general field info->altp2m. Yet, IMHO the architecture would benefit if we would have only one call to xc_hvm_param_set. >> 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. I did not expect other applications using this field outside of libxl but of course you are right. My next patch will contain the legacy info->u.hvm.altp2m field in addition to the general/common field info->altp2m. Thank you for pointing that out. >> ("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. I will change that, thank you. I mentioned ARM PV as currently it is the type that is registered for guests on ARM. >> + ("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. This is exactly the behavior right now (see comment below): + /* 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". */ The warning is just displayed at this point; "altp2mhvm" is considered as a valid parameter. > > 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. I cannot find any entry concerning the current "altp2mhvm" option. Please correct me if I am wrong, but as far as I understand, this document holds information about the "xl" tool. Since altp2m is currently not controlled through the xl tool, I am actually not sure whether it is the right place for it. I believe you meant the file docs/man/xl.cfg.pod.5. If yes, I will gladly extend it, thank you. >> >> 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. I am actually not sure what you mean by the dead option. Could you be more specific, please? Also, in the last patch, we came to the agreement to guard the altp2m functionality solely through the HVM param (instead of the additional Xen cmd-line option altp2m), which is set through libxl. Because of this, the entire domu initialization routines depend on this option to be set at the moment of domain creation -- not after it. That is, being able to set the altp2m option at all time would actually break the system. > And there should be > code in libxl to handle situation when altp2m is not available. I am not so sure about that either: Currently, altp2m is supported on x86 HVM and on ARM. * on x86, altp2m depends on HW to support altp2m. Therefore, the cmd-line option "altp2m" is used do activate altp2m. All libxl interaction with the altp2m subsystem will be discarded at this point. * on ARM, altp2m is implemented purely in SW. That is, we do not have ARM architectures, that would not support altp2m -- so the idea. * All other architectures should not be able to activate altp2m, which is why I chose the #defines (__arm__, __x86_64__, ...) to guard the upper option. >> + 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. Thank you very much for your review. Best regards, ~Sergej
Sorry for the late reply. On Tue, Aug 02, 2016 at 04:07:53PM +0200, Sergej Proskurin wrote: > Hi Wei, [...] > >> @@ -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? > > Since this implementation removes the field info->u.hvm.altp2m and > rather uses the common field info->altp2m, I wanted to set the altp2m > parameter outside of a function that is associated with an HVM domain > (as the function hvm_set_conf_params is called only if the field > info->type == LIBXL_DOMAIN_TYPE_HVM). The idea was to have only one call > to xc_hvm_param_set independent of the domain type, as we do not > distinguish between the underlying architecture anymore.If you believe > that we nevertheless need two calls in the code, I will move the > function call in question back to hvm_set_conf_params and add an > additional call to xc_hvm_param_set for the general field info->altp2m. > Yet, IMHO the architecture would benefit if we would have only one call > to xc_hvm_param_set. > No problem. I'm fine with your arrangement. But you do need to wrap the line properly. > >> 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. > > I did not expect other applications using this field outside of libxl > but of course you are right. My next patch will contain the legacy > info->u.hvm.altp2m field in addition to the general/common field > info->altp2m. Thank you for pointing that out. > > >> ("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), But you do need to wrap the line properly. > >> ])), > >> + # 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. > > I will change that, thank you. I mentioned ARM PV as currently it is the > type that is registered for guests on ARM. > > >> + ("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. > > This is exactly the behavior right now (see comment below): > > + /* 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". */ > > The warning is just displayed at this point; "altp2mhvm" is considered > as a valid parameter. > > > > > 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. > > I cannot find any entry concerning the current "altp2mhvm" option. > Please correct me if I am wrong, but as far as I understand, this > document holds information about the "xl" tool. Since altp2m is > currently not controlled through the xl tool, I am actually not sure > whether it is the right place for it. I believe you meant the file > docs/man/xl.cfg.pod.5. If yes, I will gladly extend it, thank you. > Yes, I meant xl.cfg.pod.5.in. Sorry for the typo. > >> > >> 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. > > I am actually not sure what you mean by the dead option. Could you be > more specific, please? > > Also, in the last patch, we came to the agreement to guard the altp2m > functionality solely through the HVM param (instead of the additional > Xen cmd-line option altp2m), which is set through libxl. Because of > this, the entire domu initialization routines depend on this option to > be set at the moment of domain creation -- not after it. That is, being > able to set the altp2m option at all time would actually break the system. > Why is this? It's possible we're talking past each other. > > And there should be > > code in libxl to handle situation when altp2m is not available. > > I am not so sure about that either: > > Currently, altp2m is supported on x86 HVM and on ARM. > * on x86, altp2m depends on HW to support altp2m. Therefore, the > cmd-line option "altp2m" is used do activate altp2m. All libxl > interaction with the altp2m subsystem will be discarded at this point. > * on ARM, altp2m is implemented purely in SW. That is, we do not have > ARM architectures, that would not support altp2m -- so the idea. > * All other architectures should not be able to activate altp2m, which > is why I chose the #defines (__arm__, __x86_64__, ...) to guard the > upper option. > > >> + 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); > >> + } > >> + What I meant was: We don't care xl (the application) sets altp2m or not. It's entirely possible that the xl on its own sets that field. The validation should be done inside libxl. If a particular configuration is not supported by libxl, it should reject that. Libxl shouldn't rely on xl (the application) to pass in fully sanitised data, it should sanitise the input itself. Basically that means, in xl, you only need: + xlu_cfg_get_defbool(config, "altp2m", &b_info->altp2m, 0); And then in libxl:initiate_domain_create you check if the configuration is valid. You can see a bunch of other checks are already there. Does this make sense to you? Wei. > > As always, if what I said above doesn't make sense to you, feel free to > > ask. > > Thank you very much for your review. > > Best regards, > ~Sergej >
Hi Wei, On 08/11/2016 06:00 PM, Wei Liu wrote: > Sorry for the late reply. > No worries, it's all good :) Thanks for your reply. [...] >>>> ("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), > > But you do need to wrap the line properly. I am not sure whether this comment was intended, as the upper line containing the fields arch_arm and gic_version were not changed by the patch. >>>> ])), >>>> + # 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. >> >> I will change that, thank you. I mentioned ARM PV as currently it is the >> type that is registered for guests on ARM. >> >>>> + ("altp2m", libxl_defbool), >>>> >>>> ], dir=DIR_IN >>>> ) [...] >>>> >>>> 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. >> >> I am actually not sure what you mean by the dead option. Could you be >> more specific, please? >> >> Also, in the last patch, we came to the agreement to guard the altp2m >> functionality solely through the HVM param (instead of the additional >> Xen cmd-line option altp2m), which is set through libxl. Because of >> this, the entire domu initialization routines depend on this option to >> be set at the moment of domain creation -- not after it. That is, being >> able to set the altp2m option at all time would actually break the system. >> > > Why is this? It's possible we're talking past each other. > I believe I was not entirely clear in my upper comment. By "we", I meant all parties involved into the discussion concerning altp2m on ARM in general. Sorry, for the confusion. Also, I was wrong: The HVM param can indeed be set at any time without any troubles. >>> And there should be >>> code in libxl to handle situation when altp2m is not available. >> >> I am not so sure about that either: >> >> Currently, altp2m is supported on x86 HVM and on ARM. >> * on x86, altp2m depends on HW to support altp2m. Therefore, the >> cmd-line option "altp2m" is used do activate altp2m. All libxl >> interaction with the altp2m subsystem will be discarded at this point. >> * on ARM, altp2m is implemented purely in SW. That is, we do not have >> ARM architectures, that would not support altp2m -- so the idea. >> * All other architectures should not be able to activate altp2m, which >> is why I chose the #defines (__arm__, __x86_64__, ...) to guard the >> upper option. >> >>>> + 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); >>>> + } >>>> + > > > What I meant was: > > We don't care xl (the application) sets altp2m or not. It's entirely possible > that the xl on its own sets that field. The validation should be done > inside libxl. If a particular configuration is not supported by libxl, > it should reject that. Libxl shouldn't rely on xl (the application) to > pass in fully sanitised data, it should sanitise the input itself. > > Basically that means, in xl, you only need: > > + xlu_cfg_get_defbool(config, "altp2m", &b_info->altp2m, 0); > > And then in libxl:initiate_domain_create you check if the configuration > is valid. You can see a bunch of other checks are already there. > > Does this make sense to you? > > I think so. As you said, in my next patch, I will simply set b_info->u.hvm.altp2m and b_info->altp2m during parsing and check for sufficient support in initiate_domain_create. Thank you. >>> As always, if what I said above doesn't make sense to you, feel free to >>> ask. >> Best regards, ~Sergej
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 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)); + 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), ("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. + ("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"); 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 + + 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); + } + 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));
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(-)