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

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

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
> 

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