* [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts
@ 2016-08-08 23:56 Tamas K Lengyel
2016-08-08 23:56 ` [PATCH 2/2] x86/altp2m: allow specifying external-only use-case Tamas K Lengyel
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tamas K Lengyel @ 2016-08-08 23:56 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, George Dunlap, Tamas K Lengyel,
Jan Beulich, Andrew Cooper
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.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
xen/arch/x86/mm/p2m.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 812dbf6..fb1fda4 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 )
@@ -2563,8 +2563,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;
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] x86/altp2m: allow specifying external-only use-case
2016-08-08 23:56 [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Tamas K Lengyel
@ 2016-08-08 23:56 ` Tamas K Lengyel
2016-08-09 8:06 ` Jan Beulich
2016-08-09 6:16 ` [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Razvan Cojocaru
2016-08-09 7:55 ` Jan Beulich
2 siblings, 1 reply; 9+ messages in thread
From: Tamas K Lengyel @ 2016-08-08 23:56 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Tamas K Lengyel, Ian Jackson, Jan Beulich,
Andrew Cooper, Daniel De Graaf
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 HVMALTP2M_* modes. If external_only mode is
selected, the XSM check is shifted to use XSM_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>
---
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..f68996e 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 > HVMALTP2M_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..52883f9 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 HVMALTP2M_disabled 0
+#define HVMALTP2M_mixed 1
+#define HVMALTP2M_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..6df622a 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 HVMALTP2M_mixed:
+ return xsm_default_action(XSM_TARGET, current->domain, d);
+ case HVMALTP2M_external_only:
+ return xsm_default_action(XSM_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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts
2016-08-08 23:56 [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Tamas K Lengyel
2016-08-08 23:56 ` [PATCH 2/2] x86/altp2m: allow specifying external-only use-case Tamas K Lengyel
@ 2016-08-09 6:16 ` Razvan Cojocaru
2016-08-09 7:55 ` Jan Beulich
2 siblings, 0 replies; 9+ messages in thread
From: Razvan Cojocaru @ 2016-08-09 6:16 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: George Dunlap, Andrew Cooper, Tamas K Lengyel, Jan Beulich
On 08/09/16 02:56, Tamas K Lengyel 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.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
> xen/arch/x86/mm/p2m.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts
2016-08-08 23:56 [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Tamas K Lengyel
2016-08-08 23:56 ` [PATCH 2/2] x86/altp2m: allow specifying external-only use-case Tamas K Lengyel
2016-08-09 6:16 ` [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Razvan Cojocaru
@ 2016-08-09 7:55 ` Jan Beulich
2016-08-09 15:44 ` Tamas Lengyel
2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2016-08-09 7:55 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: George Dunlap, Andrew Cooper, Tamas K Lengyel, Razvan Cojocaru,
xen-devel
>>> On 09.08.16 at 01:56, <tamas.lengyel@zentific.com> wrote:
> 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.
You fail to mention why the resulting code is nevertheless correct:
> --- 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);
In this case the respective p2m lock is already being held.
> @@ -2563,8 +2563,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);
In this case, however, only ap2m's lock is being held, yet you query
hp2m, so it's not immediately obvious whether the change is correct,
and it would seem to me that you'd want to hold the gfn lock until
after the subsequent ap2m->set_entry() (to make sure the two don't
go out of sync). Since taking hp2m's lock can't nest inside any of its
ap2m-s' ones, that'd be a slightly more intrusive adjustment.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/altp2m: allow specifying external-only use-case
2016-08-08 23:56 ` [PATCH 2/2] x86/altp2m: allow specifying external-only use-case Tamas K Lengyel
@ 2016-08-09 8:06 ` Jan Beulich
2016-08-09 15:53 ` Tamas K Lengyel
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2016-08-09 8:06 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Andrew Cooper, Daniel De Graaf, Wei Liu, Ian Jackson, xen-devel
>>> On 09.08.16 at 01:56, <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])) )
As per the 3rd of the non-commit message notes in
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00865.html
(subject to a still outstanding response from Daniel or others)
I'd prefer if no new uses of XSM_OTHER got added.
> --- 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 HVMALTP2M_disabled 0
> +#define HVMALTP2M_mixed 1
> +#define HVMALTP2M_external_only 2
No new classes / groups of identifiers in the public interface please
which are in violation of the name spacing rules (read: should be
prefixed by XEN_).
Apart from that - would a guest-only mode make sense, i.e. would
making this individual flags rather than an enumeration like thing be
better?
> --- 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 HVMALTP2M_mixed:
> + return xsm_default_action(XSM_TARGET, current->domain, d);
> + case HVMALTP2M_external_only:
> + return xsm_default_action(XSM_PRIV, current->domain, d);
Does this really need to be PRIV instead of just DM_PRIV?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts
2016-08-09 7:55 ` Jan Beulich
@ 2016-08-09 15:44 ` Tamas Lengyel
0 siblings, 0 replies; 9+ messages in thread
From: Tamas Lengyel @ 2016-08-09 15:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Andrew Cooper, Razvan Cojocaru, Xen-devel
On Tue, Aug 9, 2016 at 1:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.08.16 at 01:56, <tamas.lengyel@zentific.com> wrote:
>> 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.
>
> You fail to mention why the resulting code is nevertheless correct:
>
>> --- 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);
>
> In this case the respective p2m lock is already being held.
>
>> @@ -2563,8 +2563,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);
>
> In this case, however, only ap2m's lock is being held, yet you query
> hp2m, so it's not immediately obvious whether the change is correct,
> and it would seem to me that you'd want to hold the gfn lock until
> after the subsequent ap2m->set_entry() (to make sure the two don't
> go out of sync). Since taking hp2m's lock can't nest inside any of its
> ap2m-s' ones, that'd be a slightly more intrusive adjustment.
>
True, we should lock the hp2m just before we lock the ap2m here,
similar to the mem_access path, as we can't do gfn_lock after the
altp2m is already locked.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/altp2m: allow specifying external-only use-case
2016-08-09 8:06 ` Jan Beulich
@ 2016-08-09 15:53 ` Tamas K Lengyel
2016-08-09 16:12 ` Tamas K Lengyel
0 siblings, 1 reply; 9+ messages in thread
From: Tamas K Lengyel @ 2016-08-09 15:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Daniel De Graaf, Wei Liu, Ian Jackson, xen-devel
On Tue, Aug 9, 2016 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.08.16 at 01:56, <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])) )
>
> As per the 3rd of the non-commit message notes in
> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00865.html
> (subject to a still outstanding response from Daniel or others)
> I'd prefer if no new uses of XSM_OTHER got added.
The problem is that the existing xsm function does an assert on the
type of action, in this case XSM_TARGET. In the external only case we
want to do something else so we can't get pass the assert. We could
introduce a new xsm op just for the external check where the assert is
for another type of action, but that is ugly when XSM is enabled as we
would have two ops that do the same thing. So if we don't want to use
XSM_OTHER then we would have to get rid of the xsm assert so the op
can be used with different actions.
>
>> --- 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 HVMALTP2M_disabled 0
>> +#define HVMALTP2M_mixed 1
>> +#define HVMALTP2M_external_only 2
>
> No new classes / groups of identifiers in the public interface please
> which are in violation of the name spacing rules (read: should be
> prefixed by XEN_).
Ack.
>
> Apart from that - would a guest-only mode make sense, i.e. would
> making this individual flags rather than an enumeration like thing be
> better?
I don't see a reason for a guest-only mode. Also, I'm not sure if it's
possibly to do that with XSM disabled.
>
>> --- 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 HVMALTP2M_mixed:
>> + return xsm_default_action(XSM_TARGET, current->domain, d);
>> + case HVMALTP2M_external_only:
>> + return xsm_default_action(XSM_PRIV, current->domain, d);
>
> Does this really need to be PRIV instead of just DM_PRIV?
It could be either, I'm OK with relaxing it to DM_PRIV.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/altp2m: allow specifying external-only use-case
2016-08-09 15:53 ` Tamas K Lengyel
@ 2016-08-09 16:12 ` Tamas K Lengyel
2016-08-09 16:17 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Tamas K Lengyel @ 2016-08-09 16:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Daniel De Graaf, Wei Liu, Ian Jackson, xen-devel
>>> --- 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 HVMALTP2M_disabled 0
>>> +#define HVMALTP2M_mixed 1
>>> +#define HVMALTP2M_external_only 2
>>
>> No new classes / groups of identifiers in the public interface please
>> which are in violation of the name spacing rules (read: should be
>> prefixed by XEN_).
>
> Ack.
Should it be XEN_ALTP2M_* or XEN_HVMALTP2M_*? I prefer the former as
it's a bit more readable.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/altp2m: allow specifying external-only use-case
2016-08-09 16:12 ` Tamas K Lengyel
@ 2016-08-09 16:17 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2016-08-09 16:17 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Andrew Cooper, Daniel De Graaf, Wei Liu, Ian Jackson, xen-devel
>>> On 09.08.16 at 18:12, <tamas.lengyel@zentific.com> wrote:
>>>> --- 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 HVMALTP2M_disabled 0
>>>> +#define HVMALTP2M_mixed 1
>>>> +#define HVMALTP2M_external_only 2
>>>
>>> No new classes / groups of identifiers in the public interface please
>>> which are in violation of the name spacing rules (read: should be
>>> prefixed by XEN_).
>>
>> Ack.
>
> Should it be XEN_ALTP2M_* or XEN_HVMALTP2M_*? I prefer the former as
> it's a bit more readable.
I'd be fine with the former, too.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-09 16:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-08 23:56 [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Tamas K Lengyel
2016-08-08 23:56 ` [PATCH 2/2] x86/altp2m: allow specifying external-only use-case Tamas K Lengyel
2016-08-09 8:06 ` Jan Beulich
2016-08-09 15:53 ` Tamas K Lengyel
2016-08-09 16:12 ` Tamas K Lengyel
2016-08-09 16:17 ` Jan Beulich
2016-08-09 6:16 ` [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts Razvan Cojocaru
2016-08-09 7:55 ` Jan Beulich
2016-08-09 15:44 ` Tamas Lengyel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.