|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 23/25] arm/altp2m: Extend libxl to activate altp2m on ARM.
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@xxxxxxxxxxxxx>
>> ---
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |