|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case
On Thu, Aug 11, 2016 at 11:50:44AM -0600, Tamas K Lengyel wrote:
> On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > On Wed, Aug 10, 2016 at 09:00:15AM -0600, Tamas K Lengyel wrote:
> >> Currently setting altp2mhvm=1 in the domain configuration allows access to
> >> the
> >> altp2m interface for both in-guest and external privileged tools. This
> >> poses
> >> a problem for use-cases where only external access should be allowed,
> >> requiring
> >> the user to compile Xen with XSM enabled to be able to appropriately
> >> restrict
> >> access.
> >>
> >> In this patch we deprecate the altp2mhvm domain configuration option and
> >> introduce the altp2m option, which allows specifying if by default the
> >> altp2m
> >> interface should be external-only. The information is stored in
> >> HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
> >> If external_only mode is selected, the XSM check is shifted to use
> >> XSM_DM_PRIV
> >> type check, thus restricting access to the interface by the guest itself.
> >> Note
> >> that we keep the default XSM policy untouched. Users of XSM who wish to
> >> enforce
> >> external_only mode for altp2m can do so by adjusting their XSM policy
> >> directly,
> >> as this domain config option does not override an active XSM policy.
> >>
> >> Also, as part of this patch we adjust the hvmop handler to require
> >> HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has
> >> been
> >> previously only required for get/set altp2m domain state, all other options
> >> were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
> >> altp2m domain state, this change introduces no new requirements to the
> >> other
> >> ops but makes it more clear that it is required for all ops.
> >>
> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> >> ---
> >> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >>
> >> v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
> >> Relax xsm check to XSM_DM_PRIV for external-only mode
> >> ---
> >> docs/man/xl.cfg.pod.5.in | 31 +++++++++++++++++++++++++++++++
> >> tools/libxl/libxl_create.c | 5 ++---
> >> tools/libxl/libxl_dom.c | 2 +-
> >> tools/libxl/libxl_types.idl | 9 ++++++++-
> >> tools/libxl/xl_cmdimpl.c | 34 +++++++++++++++++++++++++++++++++-
> >> xen/arch/x86/hvm/hvm.c | 20 ++++++++++----------
> >> xen/include/public/hvm/params.h | 10 +++++++++-
> >> xen/include/xsm/dummy.h | 14 +++++++++++---
> >> xen/include/xsm/xsm.h | 6 +++---
> >> xen/xsm/flask/hooks.c | 2 +-
> >> 10 files changed, 109 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> >> index 48c9c0d..0044b98 100644
> >> --- a/docs/man/xl.cfg.pod.5.in
> >> +++ b/docs/man/xl.cfg.pod.5.in
> >> @@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it.
> >> It may be necessary
> >> to disable the HPET in order to improve compatibility with guest
> >> Operating Systems (X86 only)
> >>
> >> +=item B<altp2m=MODE>
> >> +
> >> +Specifies access mode to the alternate-p2m capability for hvm guests.
> >> +Alternate-p2m allows a guest to manage multiple p2m guest physical
> >> +"memory views" (as opposed to a single p2m). You may want this option
> >> +if you want to access-control/isolate access to specific guest physical
> >> +memory pages accessed by the guest, e.g. for HVM domain memory
> >> +introspection or for isolation/access-control of memory between
> >> +components within a single guest hvm domain.
> >> +
> >> +The valid values are as follows:
> >> +
> >> +=over 4
> >> +
> >> +=item B<"disabled">
> >> +
> >> +Altp2m is disabled for the domain (default).
> >> +
> >> +=item B<"mixed">
> >> +
> >> +The mixed mode allows access to the altp2m interface for both in-guest
> >> +and external tools as well.
> >> +
> >> +=item B<"external_only">
> >> +
> >> +Enables access to the alternate-p2m capability for hvm guests only
> >> +by external privileged tools. Note: if XSM is enabled then the XSM policy
> >> +should be used to specify external-only access to the interface.
> >> +
> >> +=back
> >> +
> >> =item B<altp2mhvm=BOOLEAN>
> >>
> >> Enables or disables hvm guest access to alternate-p2m capability.
> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >> index 04f8ae9..c9076da 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);
> >>
> >> @@ -918,8 +917,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");
> >> + d_config->b_info.u.hvm.altp2m != LIBXL_ALTP2M_MODE_DISABLED)) {
> >> + 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 eef5045..0b8b2a8 100644
> >> --- a/tools/libxl/libxl_dom.c
> >> +++ b/tools/libxl/libxl_dom.c
> >> @@ -292,7 +292,7 @@ static void hvm_set_conf_params(xc_interface *handle,
> >> uint32_t domid,
> >> 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));
> >> + info->u.hvm.altp2m);
> >> }
> >>
> >> int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index ef614be..97948fd 100644
> >> --- a/tools/libxl/libxl_types.idl
> >> +++ b/tools/libxl/libxl_types.idl
> >> @@ -439,6 +439,13 @@ libxl_rdm_reserve = Struct("rdm_reserve", [
> >> ("policy", libxl_rdm_reserve_policy),
> >> ])
> >>
> >> +# Consistent with the values defined for HVM_PARAM_ALTP2M
> >> +libxl_altp2m_mode = Enumeration("altp2m_mode", [
> >> + (0, "disabled"),
> >> + (1, "mixed"),
> >> + (2, "external_only"),
> >> + ], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
> >> +
> >> libxl_domain_build_info = Struct("domain_build_info",[
> >> ("max_vcpus", integer),
> >> ("avail_vcpus", libxl_bitmap),
> >> @@ -512,7 +519,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >> ("mmio_hole_memkb", MemKB),
> >> ("timer_mode",
> >> libxl_timer_mode),
> >> ("nested_hvm",
> >> libxl_defbool),
> >> - ("altp2m",
> >> libxl_defbool),
> >> + ("altp2m",
> >> libxl_altp2m_mode),
> >
> > This is a breaking change.
> >
> > Let me think a bit how to make it backward compatible.
> >
> >> ("smbios_firmware", string),
> >> ("acpi_firmware", string),
> >> ("hdtype", libxl_hdtype),
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 7f961e3..a655296 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -1667,7 +1667,39 @@ 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_long(config, "altp2mhvm", &l, 0)) {
> >> + fprintf(stderr, "WARNING: Specifying \"altp2mhvm\" is
> >> deprecated. "
> >> + "Please use a \"altp2m\" instead.\n");
> >
> > You might want to delete this check, and ...
> >
> >> +
> >> + if (l < LIBXL_ALTP2M_MODE_DISABLED ||
> >> + l > LIBXL_ALTP2M_MODE_MIXED) {
> >> + fprintf(stderr, "ERROR: invalid value %ld for
> >> \"altp2mhvm\"\n", l);
> >> + exit (1);
> >> + }
> >> +
> >> + b_info->u.hvm.altp2m = l;
> >
> > b_info->u.hvm.altp2m = l ? LIBXL_ALTP2M_MIXED :
> > LIBXL_ALTP2M_DISABLED;
> >
> > Assuming that's the original semantics of this option, i.e. zero means
> > disabled, non-zero value means MIXED mode.
> >
>
> That's the idea but what if the user sets altp2mhvm=2 by accident
> instead of altp2m? If we just check l being non-zero it opens the door
> for some errors.
Oh, I see where you come from now. You're afraid of people accidentally
enabling MIXED mode.
I would normally argue that people should read manpage more carefully
and not type in the wrong thing. But I suspect most people only ever use
0 or 1 in setting so keeping your code won't affect most users and on
the other hand prevents error. Please keep your original code.
Wei.
>
> Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |