[PATCH v2 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[PATCH v2 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[Tamas K Lengyel]

From: Tamas K Lengyel <tamas@tklengyel.com>

Use __get_gfn_type_access instead of get_gfn_type_access when checking
the hostp2m entries during altp2m mem_access setting and gfn remapping
to avoid a lock conflict which can make dom0 freeze. During mem_access
setting the hp2m is already locked. For gfn remapping we change the flow
to lock the hp2m before locking the ap2m.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2: Lock the hp2m during gfn remapping
---
 xen/arch/x86/mm/p2m.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 812dbf6..1eeb934 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     if ( !mfn_valid(mfn) )
     {
 
-        mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
+        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
+                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
 
         rc = -ESRCH;
         if ( !mfn_valid(mfn) || t != p2m_ram_rw )
@@ -2548,6 +2548,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     hp2m = p2m_get_hostp2m(d);
     ap2m = d->arch.altp2m_p2m[idx];
 
+    p2m_lock(hp2m);
     p2m_lock(ap2m);
 
     mfn = ap2m->get_entry(ap2m, gfn_x(old_gfn), &t, &a, 0, NULL, NULL);
@@ -2563,8 +2564,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
     {
-        mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
+        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
+                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
 
         if ( !mfn_valid(mfn) || t != p2m_ram_rw )
             goto out;
@@ -2606,6 +2607,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
 
  out:
     p2m_unlock(ap2m);
+    p2m_unlock(hp2m);
     return rc;
 }
 
-- 
2.8.1


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

[PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Tamas K Lengyel]

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@zentific.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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),
                                        ("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");
+
+            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;
+        }
+
+        if (!xlu_cfg_get_long(config, "altp2m", &l, 0)) {
+            if (l < LIBXL_ALTP2M_MODE_DISABLED ||
+                l > LIBXL_ALTP2M_MODE_EXTERNAL_ONLY) {
+                fprintf(stderr, "ERROR: invalid value %ld for \"altp2m\"\n", l);
+                exit (1);
+            }
+
+            b_info->u.hvm.altp2m = l;
+        } else if (!xlu_cfg_get_string(config, "altp2m", &buf, 0)) {
+            if (libxl_altp2m_mode_from_string(buf, &b_info->u.hvm.altp2m)) {
+                fprintf(stderr, "ERROR: invalid value \"%s\" for \"altp2m\"\n",
+                        buf);
+                exit (1);
+            }
+        }
 
         xlu_cfg_replace_string(config, "smbios_firmware",
                                &b_info->u.hvm.smbios_firmware, 0);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 893eff6..0d59940 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5042,7 +5042,7 @@ static int hvmop_set_param(
         rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d);
         if ( rc )
             break;
-        if ( a.value > 1 )
+        if ( a.value > XEN_ALTP2M_external_only )
             rc = -EINVAL;
         if ( a.value &&
              d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
@@ -5238,18 +5238,19 @@ static int do_altp2m_op(
         goto out;
     }
 
-    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
+    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
+    {
+        rc = -EINVAL;
+        goto out;
+    }
+
+    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
+                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
         goto out;
 
     switch ( a.cmd )
     {
     case HVMOP_altp2m_get_domain_state:
-        if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
-        {
-            rc = -EINVAL;
-            break;
-        }
-
         a.u.domain_state.state = altp2m_active(d);
         rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         break;
@@ -5259,8 +5260,7 @@ static int do_altp2m_op(
         struct vcpu *v;
         bool_t ostate;
 
-        if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] ||
-             nestedhvm_enabled(d) )
+        if ( nestedhvm_enabled(d) )
         {
             rc = -EINVAL;
             break;
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index f7338a3..3ffc33b 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -225,8 +225,16 @@
 /* Location of the VM Generation ID in guest physical address space. */
 #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
 
-/* Boolean: Enable altp2m */
+/*
+ * Set mode for altp2m:
+ *  disabled: don't activate altp2m (default)
+ *  mixed: allow access to altp2m for both in-guest and external tools
+ *  external_only: allow access to external privileged tools only
+ */
 #define HVM_PARAM_ALTP2M       35
+#define XEN_ALTP2M_disabled      0
+#define XEN_ALTP2M_mixed         1
+#define XEN_ALTP2M_external_only 2
 
 /*
  * Size of the x87 FPU FIP/FDP registers that the hypervisor needs to
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 406cd18..a0f6b70 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -554,10 +554,18 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
     return xsm_default_action(action, current->domain, d);
 }
 
-static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d)
+static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode)
 {
-    XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, current->domain, d);
+    XSM_ASSERT_ACTION(XSM_OTHER);
+    switch ( mode )
+    {
+    case XEN_ALTP2M_mixed:
+        return xsm_default_action(XSM_TARGET, current->domain, d);
+    case XEN_ALTP2M_external_only:
+        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
+    default:
+        return -EPERM;
+    };
 }
 
 static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 5dc59dd..4f53b27 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -140,7 +140,7 @@ struct xsm_operations {
     int (*hvm_control) (struct domain *d, unsigned long op);
     int (*hvm_param_nested) (struct domain *d);
     int (*hvm_param_altp2mhvm) (struct domain *d);
-    int (*hvm_altp2mhvm_op) (struct domain *d);
+    int (*hvm_altp2mhvm_op) (struct domain *d, int mode);
     int (*get_vnumainfo) (struct domain *d);
 
     int (*vm_event_control) (struct domain *d, int mode, int op);
@@ -583,9 +583,9 @@ static inline int xsm_hvm_param_altp2mhvm (xsm_default_t def, struct domain *d)
     return xsm_ops->hvm_param_altp2mhvm(d);
 }
 
-static inline int xsm_hvm_altp2mhvm_op (xsm_default_t def, struct domain *d)
+static inline int xsm_hvm_altp2mhvm_op (xsm_default_t def, struct domain *d, int mode)
 {
-    return xsm_ops->hvm_altp2mhvm_op(d);
+    return xsm_ops->hvm_altp2mhvm_op(d, mode);
 }
 
 static inline int xsm_get_vnumainfo (xsm_default_t def, struct domain *d)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index ec6f5b4..b97396e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1197,7 +1197,7 @@ static int flask_hvm_param_altp2mhvm(struct domain *d)
     return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM);
 }
 
-static int flask_hvm_altp2mhvm_op(struct domain *d)
+static int flask_hvm_altp2mhvm_op(struct domain *d, int mode)
 {
     return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM_OP);
 }
-- 
2.8.1


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

Re: [PATCH v2 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[Jan Beulich]

>>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
> From: Tamas K Lengyel <tamas@tklengyel.com>
> 
> Use __get_gfn_type_access instead of get_gfn_type_access when checking
> the hostp2m entries during altp2m mem_access setting and gfn remapping
> to avoid a lock conflict which can make dom0 freeze. During mem_access
> setting the hp2m is already locked. For gfn remapping we change the flow
> to lock the hp2m before locking the ap2m.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Jan Beulich]

>>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
> @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
>          goto out;
>      }
>  
> -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
> +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
> +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )

I'm sorry that this didn't occur to me on v1 already, but is there
really a need for passing this extra argument, when the callee
could - if it cared in the first place - read the value itself?

Jan


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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 904 bytes --]

On Aug 11, 2016 06:02, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
> > @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
> >          goto out;
> >      }
> >
> > -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
> > +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
> > +    {
> > +        rc = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
> > +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>
> I'm sorry that this didn't occur to me on v1 already, but is there
> really a need for passing this extra argument, when the callee
> could - if it cared in the first place - read the value itself?
>

I'm not sure if it's ok to have xsm poke around in arch specific parts like
this. We are adding this hvm param for ARM in another series but still..

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1348 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Jan Beulich]

>>> On 11.08.16 at 16:37, <tamas.lengyel@zentific.com> wrote:
> On Aug 11, 2016 06:02, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
>> > @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
>> >          goto out;
>> >      }
>> >
>> > -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
>> > +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
>> > +    {
>> > +        rc = -EINVAL;
>> > +        goto out;
>> > +    }
>> > +
>> > +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
>> > +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>>
>> I'm sorry that this didn't occur to me on v1 already, but is there
>> really a need for passing this extra argument, when the callee
>> could - if it cared in the first place - read the value itself?
> 
> I'm not sure if it's ok to have xsm poke around in arch specific parts like
> this. We are adding this hvm param for ARM in another series but still..

Daniel, what's your opinion?

Jan


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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Wei Liu]

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@zentific.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> 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.

Wei.

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Tamas K Lengyel]

On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu <wei.liu2@citrix.com> 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@zentific.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> 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.

Tamas

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Wei Liu]

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@citrix.com> 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@zentific.com>
> >> ---
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >>
> >> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Julien Grall]

Hello Tamas,

On 10/08/2016 17:00, Tamas K Lengyel wrote:
> 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),

Should we move altp2m directly outside of the structure "hvm" to avoid 
yet another change when altp2m will be supported on ARM? (see [1])

Regards,

[1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html

-- 
Julien Grall

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1742 bytes --]

On Aug 12, 2016 05:24, "Julien Grall" <julien.grall@arm.com> wrote:
>
> Hello Tamas,
>
>
> On 10/08/2016 17:00, Tamas K Lengyel wrote:
>>
>> 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),
>
>
> Should we move altp2m directly outside of the structure "hvm" to avoid
yet another change when altp2m will be supported on ARM? (see [1])
>
> Regards,
>
> [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html
>
> --
> Julien Grall

Hi Julien,
We can do that here if preferred by the tools maintainers though I'm on the
opinion that it should be adjusted in the ARM altp2m series once it
actually becomes available there.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2708 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Wei Liu]

On Fri, Aug 12, 2016 at 08:51:14AM -0600, Tamas K Lengyel wrote:
> On Aug 12, 2016 05:24, "Julien Grall" <julien.grall@arm.com> wrote:
> >
> > Hello Tamas,
> >
> >
> > On 10/08/2016 17:00, Tamas K Lengyel wrote:
> >>
> >> 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),
> >
> >
> > Should we move altp2m directly outside of the structure "hvm" to avoid
> yet another change when altp2m will be supported on ARM? (see [1])
> >
> > Regards,
> >
> > [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html
> >
> > --
> > Julien Grall
> 
> Hi Julien,
> We can do that here if preferred by the tools maintainers though I'm on the
> opinion that it should be adjusted in the ARM altp2m series once it
> actually becomes available there.
> 

I agree. We shouldn't mix things together.

Wei.

> Tamas

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

Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

[Daniel De Graaf]

On 08/11/2016 10:51 AM, Jan Beulich wrote:
>>>> On 11.08.16 at 16:37, <tamas.lengyel@zentific.com> wrote:
>> On Aug 11, 2016 06:02, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>>>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
>>>> @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
>>>>          goto out;
>>>>      }
>>>>
>>>> -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
>>>> +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
>>>> +    {
>>>> +        rc = -EINVAL;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
>>>> +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>>>
>>> I'm sorry that this didn't occur to me on v1 already, but is there
>>> really a need for passing this extra argument, when the callee
>>> could - if it cared in the first place - read the value itself?
>>
>> I'm not sure if it's ok to have xsm poke around in arch specific parts like
>> this. We are adding this hvm param for ARM in another series but still..
>
> Daniel, what's your opinion?
>
> Jan

XSM does have some required arch-specific knowledge already (x86 IO port
labeling, in particular), so it's really a style question.  I'd prefer the
form with the value passed in so that it's clearer what the XSM check is
inspecting to determine what to do, especially in this case where it changes
what permissions are actually being enforced (in the non-FLASK case).

-- 
Daniel De Graaf
National Security Agency

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

Re: [PATCH v2 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[Tamas K Lengyel]

On Thu, Aug 11, 2016 at 5:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.08.16 at 17:00, <tamas.lengyel@zentific.com> wrote:
>> From: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> Use __get_gfn_type_access instead of get_gfn_type_access when checking
>> the hostp2m entries during altp2m mem_access setting and gfn remapping
>> to avoid a lock conflict which can make dom0 freeze. During mem_access
>> setting the hp2m is already locked. For gfn remapping we change the flow
>> to lock the hp2m before locking the ap2m.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Patch ping, I think this just needs George's ack.

Tamas

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

Re: [PATCH v2 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[George Dunlap]

On Wed, Aug 10, 2016 at 4:00 PM, Tamas K Lengyel
<tamas.lengyel@zentific.com> wrote:
> From: Tamas K Lengyel <tamas@tklengyel.com>
>
> Use __get_gfn_type_access instead of get_gfn_type_access when checking
> the hostp2m entries during altp2m mem_access setting and gfn remapping
> to avoid a lock conflict which can make dom0 freeze. During mem_access
> setting the hp2m is already locked. For gfn remapping we change the flow
> to lock the hp2m before locking the ap2m.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> v2: Lock the hp2m during gfn remapping
> ---
>  xen/arch/x86/mm/p2m.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 812dbf6..1eeb934 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      if ( !mfn_valid(mfn) )
>      {
>
> -        mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
> +        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>
>          rc = -ESRCH;
>          if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> @@ -2548,6 +2548,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      hp2m = p2m_get_hostp2m(d);
>      ap2m = d->arch.altp2m_p2m[idx];
>
> +    p2m_lock(hp2m);
>      p2m_lock(ap2m);
>
>      mfn = ap2m->get_entry(ap2m, gfn_x(old_gfn), &t, &a, 0, NULL, NULL);
> @@ -2563,8 +2564,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
>      {
> -        mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
> +        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>
>          if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>              goto out;
> @@ -2606,6 +2607,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>
>   out:
>      p2m_unlock(ap2m);
> +    p2m_unlock(hp2m);
>      return rc;
>  }
>
> --
> 2.8.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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