* [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
@ 2020-01-30 13:07 Alexandru Stefan ISAILA
2020-02-07 8:54 ` Alexandru Stefan ISAILA
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2020-01-30 13:07 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima,
Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Ian Jackson, Alexandru Stefan ISAILA, Roger Pau Monné
At this moment a guest can call vmfunc to change the altp2m view. This
should be limited in order to avoid any unwanted view switch.
The new xc_altp2m_set_visibility() solves this by making views invisible
to vmfunc.
This is done by having a separate arch.altp2m_working_eptp that is
populated and made invalid in the same places as altp2m_eptp. This is
written to EPTP_LIST_ADDR.
The views are made in/visible by marking them with INVALID_MFN or
copying them back from altp2m_eptp.
To have consistency the visibility also applies to
p2m_switch_domain_altp2m_by_id().
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes since V1:
- Drop double view from title.
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_altp2m.c | 24 ++++++++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/arch/x86/mm/hap/hap.c | 15 +++++++++++++++
xen/arch/x86/mm/p2m-ept.c | 1 +
xen/arch/x86/mm/p2m.c | 5 ++++-
xen/include/asm-x86/domain.h | 1 +
xen/include/public/hvm/hvm_op.h | 10 ++++++++++
9 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index cc4eb1e3d3..dbea7861e7 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1943,6 +1943,8 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
xen_pfn_t new_gfn);
int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
uint32_t vcpuid, uint16_t *p2midx);
+int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, bool visible);
/**
* Mem paging operations.
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 46fb725806..6987c9541f 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -410,3 +410,27 @@ int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
xc_hypercall_buffer_free(handle, arg);
return rc;
}
+
+int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, bool visible)
+{
+ int rc;
+
+ DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+ arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+ if ( arg == NULL )
+ return -1;
+
+ arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+ arg->cmd = HVMOP_altp2m_set_visibility;
+ arg->domain = domid;
+ arg->u.set_visibility.altp2m_idx = view_id;
+ arg->u.set_visibility.visible = visible;
+
+ rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+ xc_hypercall_buffer_free(handle, arg);
+ return rc;
+}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0b93609a82..a41e9b6356 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4537,6 +4537,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_get_mem_access:
case HVMOP_altp2m_change_gfn:
case HVMOP_altp2m_get_p2m_idx:
+ case HVMOP_altp2m_set_visibility:
break;
default:
@@ -4814,6 +4815,30 @@ static int do_altp2m_op(
break;
}
+ case HVMOP_altp2m_set_visibility:
+ {
+ uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
+
+ if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
+ rc = -EINVAL;
+ else
+ {
+ if ( !altp2m_active(d) || !hap_enabled(d) )
+ {
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
+ if ( a.u.set_visibility.visible )
+ d->arch.altp2m_working_eptp[altp2m_idx] =
+ d->arch.altp2m_eptp[altp2m_idx];
+ else
+ d->arch.altp2m_working_eptp[altp2m_idx] =
+ mfn_x(INVALID_MFN);
+ }
+ break;
+ }
+
default:
ASSERT_UNREACHABLE();
}
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b262d38a7c..65fe75383f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2139,7 +2139,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
{
v->arch.hvm.vmx.secondary_exec_control |= mask;
__vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
- __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
+ __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_working_eptp));
if ( cpu_has_vmx_virt_exceptions )
{
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..5969ec8922 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
goto out;
}
+ if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
+ {
+ rv = -ENOMEM;
+ goto out;
+ }
+
for ( i = 0; i < MAX_EPTP; i++ )
+ {
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
+ }
for ( i = 0; i < MAX_ALTP2M; i++ )
{
@@ -523,6 +532,12 @@ void hap_final_teardown(struct domain *d)
d->arch.altp2m_eptp = NULL;
}
+ if ( d->arch.altp2m_working_eptp )
+ {
+ free_xenheap_page(d->arch.altp2m_working_eptp);
+ d->arch.altp2m_working_eptp = NULL;
+ }
+
for ( i = 0; i < MAX_ALTP2M; i++ )
p2m_teardown(d->arch.altp2m_p2m[i]);
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 05a5526e08..0e740ed58e 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1361,6 +1361,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
ept = &p2m->ept;
ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
+ d->arch.altp2m_working_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
}
unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 49cc138362..008357b761 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2531,6 +2531,7 @@ void p2m_flush_altp2m(struct domain *d)
{
p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
}
altp2m_list_unlock(d);
@@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
+ mfn_x(INVALID_MFN);
rc = 0;
}
}
@@ -2677,7 +2680,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
rc = -EINVAL;
altp2m_list_lock(d);
- if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
+ if ( d->arch.altp2m_working_eptp[idx] != mfn_x(INVALID_MFN) )
{
for_each_vcpu( d, v )
if ( idx != vcpu_altp2m(v).p2midx )
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index a3ae5d9a20..9d36f490e4 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -326,6 +326,7 @@ struct arch_domain
struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
mm_lock_t altp2m_list_lock;
uint64_t *altp2m_eptp;
+ uint64_t *altp2m_working_eptp;
#endif
/* NB. protected by d->event_lock and by irq_desc[irq].lock */
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 610e020a62..17a29615ed 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
uint16_t altp2m_idx;
};
+struct xen_hvm_altp2m_set_visibility {
+ uint16_t altp2m_idx;
+ uint8_t visible;
+ uint8_t pad;
+ uint32_t pad2;
+};
+
struct xen_hvm_altp2m_op {
uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */
uint32_t cmd;
@@ -349,6 +356,8 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_get_p2m_idx 14
/* Set the "Supress #VE" bit for a range of pages */
#define HVMOP_altp2m_set_suppress_ve_multi 15
+/* Set visibility for a given altp2m view */
+#define HVMOP_altp2m_set_visibility 16
domid_t domain;
uint16_t pad1;
uint32_t pad2;
@@ -366,6 +375,7 @@ struct xen_hvm_altp2m_op {
struct xen_hvm_altp2m_suppress_ve_multi suppress_ve_multi;
struct xen_hvm_altp2m_vcpu_disable_notify disable_notify;
struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx;
+ struct xen_hvm_altp2m_set_visibility set_visibility;
uint8_t pad[64];
} u;
};
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-01-30 13:07 [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility Alexandru Stefan ISAILA
@ 2020-02-07 8:54 ` Alexandru Stefan ISAILA
2020-02-17 11:52 ` [Xen-devel] Ping: " Alexandru Stefan ISAILA
2020-02-17 14:14 ` [Xen-devel] " Jan Beulich
2 siblings, 0 replies; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2020-02-07 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima,
Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Ian Jackson, Jan Beulich, Roger Pau Monné
Any thoughts on this are appreciated.
Thanks,
Alex
On 30.01.2020 15:07, Alexandru Stefan ISAILA wrote:
> At this moment a guest can call vmfunc to change the altp2m view. This
> should be limited in order to avoid any unwanted view switch.
>
> The new xc_altp2m_set_visibility() solves this by making views invisible
> to vmfunc.
> This is done by having a separate arch.altp2m_working_eptp that is
> populated and made invalid in the same places as altp2m_eptp. This is
> written to EPTP_LIST_ADDR.
> The views are made in/visible by marking them with INVALID_MFN or
> copying them back from altp2m_eptp.
> To have consistency the visibility also applies to
> p2m_switch_domain_altp2m_by_id().
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien@xen.org>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes since V1:
> - Drop double view from title.
> ---
> tools/libxc/include/xenctrl.h | 2 ++
> tools/libxc/xc_altp2m.c | 24 ++++++++++++++++++++++++
> xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++++++++++
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> xen/arch/x86/mm/hap/hap.c | 15 +++++++++++++++
> xen/arch/x86/mm/p2m-ept.c | 1 +
> xen/arch/x86/mm/p2m.c | 5 ++++-
> xen/include/asm-x86/domain.h | 1 +
> xen/include/public/hvm/hvm_op.h | 10 ++++++++++
> 9 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index cc4eb1e3d3..dbea7861e7 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1943,6 +1943,8 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
> xen_pfn_t new_gfn);
> int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> uint32_t vcpuid, uint16_t *p2midx);
> +int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, bool visible);
>
> /**
> * Mem paging operations.
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 46fb725806..6987c9541f 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -410,3 +410,27 @@ int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> xc_hypercall_buffer_free(handle, arg);
> return rc;
> }
> +
> +int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, bool visible)
> +{
> + int rc;
> +
> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> + if ( arg == NULL )
> + return -1;
> +
> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> + arg->cmd = HVMOP_altp2m_set_visibility;
> + arg->domain = domid;
> + arg->u.set_visibility.altp2m_idx = view_id;
> + arg->u.set_visibility.visible = visible;
> +
> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> + xc_hypercall_buffer_free(handle, arg);
> + return rc;
> +}
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0b93609a82..a41e9b6356 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4537,6 +4537,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_get_mem_access:
> case HVMOP_altp2m_change_gfn:
> case HVMOP_altp2m_get_p2m_idx:
> + case HVMOP_altp2m_set_visibility:
> break;
>
> default:
> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
> break;
> }
>
> + case HVMOP_altp2m_set_visibility:
> + {
> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
> +
> + if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
> + rc = -EINVAL;
> + else
> + {
> + if ( !altp2m_active(d) || !hap_enabled(d) )
> + {
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + if ( a.u.set_visibility.visible )
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + d->arch.altp2m_eptp[altp2m_idx];
> + else
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + mfn_x(INVALID_MFN);
> + }
> + break;
> + }
> +
> default:
> ASSERT_UNREACHABLE();
> }
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index b262d38a7c..65fe75383f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2139,7 +2139,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
> {
> v->arch.hvm.vmx.secondary_exec_control |= mask;
> __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
> - __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
> + __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_working_eptp));
>
> if ( cpu_has_vmx_virt_exceptions )
> {
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 3d93f3451c..5969ec8922 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
> goto out;
> }
>
> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
> + {
> + rv = -ENOMEM;
> + goto out;
> + }
> +
> for ( i = 0; i < MAX_EPTP; i++ )
> + {
> d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
> + }
>
> for ( i = 0; i < MAX_ALTP2M; i++ )
> {
> @@ -523,6 +532,12 @@ void hap_final_teardown(struct domain *d)
> d->arch.altp2m_eptp = NULL;
> }
>
> + if ( d->arch.altp2m_working_eptp )
> + {
> + free_xenheap_page(d->arch.altp2m_working_eptp);
> + d->arch.altp2m_working_eptp = NULL;
> + }
> +
> for ( i = 0; i < MAX_ALTP2M; i++ )
> p2m_teardown(d->arch.altp2m_p2m[i]);
> }
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 05a5526e08..0e740ed58e 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1361,6 +1361,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> ept = &p2m->ept;
> ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
> d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
> + d->arch.altp2m_working_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
> }
>
> unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 49cc138362..008357b761 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2531,6 +2531,7 @@ void p2m_flush_altp2m(struct domain *d)
> {
> p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
> d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
> }
>
> altp2m_list_unlock(d);
> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
> mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> + mfn_x(INVALID_MFN);
> rc = 0;
> }
> }
> @@ -2677,7 +2680,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
> rc = -EINVAL;
> altp2m_list_lock(d);
>
> - if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
> + if ( d->arch.altp2m_working_eptp[idx] != mfn_x(INVALID_MFN) )
> {
> for_each_vcpu( d, v )
> if ( idx != vcpu_altp2m(v).p2midx )
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index a3ae5d9a20..9d36f490e4 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -326,6 +326,7 @@ struct arch_domain
> struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> mm_lock_t altp2m_list_lock;
> uint64_t *altp2m_eptp;
> + uint64_t *altp2m_working_eptp;
> #endif
>
> /* NB. protected by d->event_lock and by irq_desc[irq].lock */
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 610e020a62..17a29615ed 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
> uint16_t altp2m_idx;
> };
>
> +struct xen_hvm_altp2m_set_visibility {
> + uint16_t altp2m_idx;
> + uint8_t visible;
> + uint8_t pad;
> + uint32_t pad2;
> +};
> +
> struct xen_hvm_altp2m_op {
> uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */
> uint32_t cmd;
> @@ -349,6 +356,8 @@ struct xen_hvm_altp2m_op {
> #define HVMOP_altp2m_get_p2m_idx 14
> /* Set the "Supress #VE" bit for a range of pages */
> #define HVMOP_altp2m_set_suppress_ve_multi 15
> +/* Set visibility for a given altp2m view */
> +#define HVMOP_altp2m_set_visibility 16
> domid_t domain;
> uint16_t pad1;
> uint32_t pad2;
> @@ -366,6 +375,7 @@ struct xen_hvm_altp2m_op {
> struct xen_hvm_altp2m_suppress_ve_multi suppress_ve_multi;
> struct xen_hvm_altp2m_vcpu_disable_notify disable_notify;
> struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx;
> + struct xen_hvm_altp2m_set_visibility set_visibility;
> uint8_t pad[64];
> } u;
> };
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Xen-devel] Ping: [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-01-30 13:07 [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility Alexandru Stefan ISAILA
2020-02-07 8:54 ` Alexandru Stefan ISAILA
@ 2020-02-17 11:52 ` Alexandru Stefan ISAILA
2020-02-17 14:14 ` [Xen-devel] " Jan Beulich
2 siblings, 0 replies; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2020-02-17 11:52 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima,
Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Ian Jackson, Jan Beulich, Roger Pau Monné
Hi all,
Any ideas on this patch appreciated.
Regards,
Alex
On 30.01.2020 15:07, Alexandru Stefan ISAILA wrote:
> At this moment a guest can call vmfunc to change the altp2m view. This
> should be limited in order to avoid any unwanted view switch.
>
> The new xc_altp2m_set_visibility() solves this by making views invisible
> to vmfunc.
> This is done by having a separate arch.altp2m_working_eptp that is
> populated and made invalid in the same places as altp2m_eptp. This is
> written to EPTP_LIST_ADDR.
> The views are made in/visible by marking them with INVALID_MFN or
> copying them back from altp2m_eptp.
> To have consistency the visibility also applies to
> p2m_switch_domain_altp2m_by_id().
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien@xen.org>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes since V1:
> - Drop double view from title.
> ---
> tools/libxc/include/xenctrl.h | 2 ++
> tools/libxc/xc_altp2m.c | 24 ++++++++++++++++++++++++
> xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++++++++++
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> xen/arch/x86/mm/hap/hap.c | 15 +++++++++++++++
> xen/arch/x86/mm/p2m-ept.c | 1 +
> xen/arch/x86/mm/p2m.c | 5 ++++-
> xen/include/asm-x86/domain.h | 1 +
> xen/include/public/hvm/hvm_op.h | 10 ++++++++++
> 9 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index cc4eb1e3d3..dbea7861e7 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1943,6 +1943,8 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
> xen_pfn_t new_gfn);
> int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> uint32_t vcpuid, uint16_t *p2midx);
> +int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, bool visible);
>
> /**
> * Mem paging operations.
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 46fb725806..6987c9541f 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -410,3 +410,27 @@ int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> xc_hypercall_buffer_free(handle, arg);
> return rc;
> }
> +
> +int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, bool visible)
> +{
> + int rc;
> +
> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> + if ( arg == NULL )
> + return -1;
> +
> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> + arg->cmd = HVMOP_altp2m_set_visibility;
> + arg->domain = domid;
> + arg->u.set_visibility.altp2m_idx = view_id;
> + arg->u.set_visibility.visible = visible;
> +
> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> + xc_hypercall_buffer_free(handle, arg);
> + return rc;
> +}
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0b93609a82..a41e9b6356 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4537,6 +4537,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_get_mem_access:
> case HVMOP_altp2m_change_gfn:
> case HVMOP_altp2m_get_p2m_idx:
> + case HVMOP_altp2m_set_visibility:
> break;
>
> default:
> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
> break;
> }
>
> + case HVMOP_altp2m_set_visibility:
> + {
> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
> +
> + if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
> + rc = -EINVAL;
> + else
> + {
> + if ( !altp2m_active(d) || !hap_enabled(d) )
> + {
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + if ( a.u.set_visibility.visible )
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + d->arch.altp2m_eptp[altp2m_idx];
> + else
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + mfn_x(INVALID_MFN);
> + }
> + break;
> + }
> +
> default:
> ASSERT_UNREACHABLE();
> }
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index b262d38a7c..65fe75383f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2139,7 +2139,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
> {
> v->arch.hvm.vmx.secondary_exec_control |= mask;
> __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
> - __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
> + __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_working_eptp));
>
> if ( cpu_has_vmx_virt_exceptions )
> {
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 3d93f3451c..5969ec8922 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
> goto out;
> }
>
> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
> + {
> + rv = -ENOMEM;
> + goto out;
> + }
> +
> for ( i = 0; i < MAX_EPTP; i++ )
> + {
> d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
> + }
>
> for ( i = 0; i < MAX_ALTP2M; i++ )
> {
> @@ -523,6 +532,12 @@ void hap_final_teardown(struct domain *d)
> d->arch.altp2m_eptp = NULL;
> }
>
> + if ( d->arch.altp2m_working_eptp )
> + {
> + free_xenheap_page(d->arch.altp2m_working_eptp);
> + d->arch.altp2m_working_eptp = NULL;
> + }
> +
> for ( i = 0; i < MAX_ALTP2M; i++ )
> p2m_teardown(d->arch.altp2m_p2m[i]);
> }
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 05a5526e08..0e740ed58e 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1361,6 +1361,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> ept = &p2m->ept;
> ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
> d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
> + d->arch.altp2m_working_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
> }
>
> unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 49cc138362..008357b761 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2531,6 +2531,7 @@ void p2m_flush_altp2m(struct domain *d)
> {
> p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
> d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
> }
>
> altp2m_list_unlock(d);
> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
> mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> + mfn_x(INVALID_MFN);
> rc = 0;
> }
> }
> @@ -2677,7 +2680,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
> rc = -EINVAL;
> altp2m_list_lock(d);
>
> - if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
> + if ( d->arch.altp2m_working_eptp[idx] != mfn_x(INVALID_MFN) )
> {
> for_each_vcpu( d, v )
> if ( idx != vcpu_altp2m(v).p2midx )
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index a3ae5d9a20..9d36f490e4 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -326,6 +326,7 @@ struct arch_domain
> struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> mm_lock_t altp2m_list_lock;
> uint64_t *altp2m_eptp;
> + uint64_t *altp2m_working_eptp;
> #endif
>
> /* NB. protected by d->event_lock and by irq_desc[irq].lock */
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 610e020a62..17a29615ed 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
> uint16_t altp2m_idx;
> };
>
> +struct xen_hvm_altp2m_set_visibility {
> + uint16_t altp2m_idx;
> + uint8_t visible;
> + uint8_t pad;
> + uint32_t pad2;
> +};
> +
> struct xen_hvm_altp2m_op {
> uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */
> uint32_t cmd;
> @@ -349,6 +356,8 @@ struct xen_hvm_altp2m_op {
> #define HVMOP_altp2m_get_p2m_idx 14
> /* Set the "Supress #VE" bit for a range of pages */
> #define HVMOP_altp2m_set_suppress_ve_multi 15
> +/* Set visibility for a given altp2m view */
> +#define HVMOP_altp2m_set_visibility 16
> domid_t domain;
> uint16_t pad1;
> uint32_t pad2;
> @@ -366,6 +375,7 @@ struct xen_hvm_altp2m_op {
> struct xen_hvm_altp2m_suppress_ve_multi suppress_ve_multi;
> struct xen_hvm_altp2m_vcpu_disable_notify disable_notify;
> struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx;
> + struct xen_hvm_altp2m_set_visibility set_visibility;
> uint8_t pad[64];
> } u;
> };
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-01-30 13:07 [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility Alexandru Stefan ISAILA
2020-02-07 8:54 ` Alexandru Stefan ISAILA
2020-02-17 11:52 ` [Xen-devel] Ping: " Alexandru Stefan ISAILA
@ 2020-02-17 14:14 ` Jan Beulich
2020-02-18 8:13 ` Alexandru Stefan ISAILA
2 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2020-02-17 14:14 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Jun Nakajima, xen-devel, Roger Pau Monné
On 30.01.2020 14:07, Alexandru Stefan ISAILA wrote:
> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
> break;
> }
>
> + case HVMOP_altp2m_set_visibility:
> + {
> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
> +
> + if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
> + rc = -EINVAL;
> + else
> + {
> + if ( !altp2m_active(d) || !hap_enabled(d) )
Doesn't altp2m_active() imply hap_enabled()? At the very least
there's no other use of hap_enabled() in do_altp2m_op().
> + {
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + if ( a.u.set_visibility.visible )
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + d->arch.altp2m_eptp[altp2m_idx];
> + else
> + d->arch.altp2m_working_eptp[altp2m_idx] =
> + mfn_x(INVALID_MFN);
> + }
> + break;
Also the code here lends itself to reduction of indentation
depth:
case HVMOP_altp2m_set_visibility:
{
uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
rc = -EINVAL;
else if ( !altp2m_active(d) || !hap_enabled(d) )
rc = -EOPNOTSUPP;
else if ( a.u.set_visibility.visible )
d->arch.altp2m_working_eptp[altp2m_idx] =
d->arch.altp2m_eptp[altp2m_idx];
else
d->arch.altp2m_working_eptp[altp2m_idx] =
mfn_x(INVALID_MFN);
break;
}
Also note the altered indentation of the assignments.
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
> goto out;
> }
>
> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
> + {
> + rv = -ENOMEM;
> + goto out;
> + }
Isn't there a pre-existing error handling issue here which you
widen, in that later encountered errors don't cause clean up
of what had already succeeded before?
> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
> mfn_x(INVALID_MFN);
> + d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> + mfn_x(INVALID_MFN);
Like above, and irrespective of you cloning existing code -
indentation of the 2nd line is wrong here.
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
> uint16_t altp2m_idx;
> };
>
> +struct xen_hvm_altp2m_set_visibility {
> + uint16_t altp2m_idx;
> + uint8_t visible;
> + uint8_t pad;
> + uint32_t pad2;
> +};
What is pad2 good/intended for? 32-bit padding fields in some
other structures are needed because one or more uint64_t
fields follow, but this isn't the case here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-02-17 14:14 ` [Xen-devel] " Jan Beulich
@ 2020-02-18 8:13 ` Alexandru Stefan ISAILA
2020-02-18 14:42 ` Alexandru Stefan ISAILA
0 siblings, 1 reply; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2020-02-18 8:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Jun Nakajima, xen-devel, Roger Pau Monné
On 17.02.2020 16:14, Jan Beulich wrote:
> On 30.01.2020 14:07, Alexandru Stefan ISAILA wrote:
>> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
>> break;
>> }
>>
>> + case HVMOP_altp2m_set_visibility:
>> + {
>> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>> +
>> + if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>> + rc = -EINVAL;
>> + else
>> + {
>> + if ( !altp2m_active(d) || !hap_enabled(d) )
>
> Doesn't altp2m_active() imply hap_enabled()? At the very least
> there's no other use of hap_enabled() in do_altp2m_op().
Yes, the hap_enabled can be dropped.
>
>> + {
>> + rc = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> + if ( a.u.set_visibility.visible )
>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>> + d->arch.altp2m_eptp[altp2m_idx];
>> + else
>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>> + mfn_x(INVALID_MFN);
>> + }
>> + break;
>
> Also the code here lends itself to reduction of indentation
> depth:
>
> case HVMOP_altp2m_set_visibility:
> {
> uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>
> if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
> rc = -EINVAL;
> else if ( !altp2m_active(d) || !hap_enabled(d) )
> rc = -EOPNOTSUPP;
> else if ( a.u.set_visibility.visible )
> d->arch.altp2m_working_eptp[altp2m_idx] =
> d->arch.altp2m_eptp[altp2m_idx];
> else
> d->arch.altp2m_working_eptp[altp2m_idx] =
> mfn_x(INVALID_MFN);
>
> break;
> }
>
>
> Also note the altered indentation of the assignments.
I will fix the else if alignment as well as the indentation for the
assignments.
>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>> goto out;
>> }
>>
>> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>> + {
>> + rv = -ENOMEM;
>> + goto out;
>> + }
>
> Isn't there a pre-existing error handling issue here which you
> widen, in that later encountered errors don't cause clean up
> of what had already succeeded before?
It seems non of the errors perform a cleanup. It might be better to have
a general cleanup done at "out:" if ( !rv ) and then check what should
be "p2m_teardown()" or "free_xenheap_page()".
>
>> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
>> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> mfn_x(INVALID_MFN);
>> + d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> + mfn_x(INVALID_MFN);
>
> Like above, and irrespective of you cloning existing code -
> indentation of the 2nd line is wrong here.
>
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
>> uint16_t altp2m_idx;
>> };
>>
>> +struct xen_hvm_altp2m_set_visibility {
>> + uint16_t altp2m_idx;
>> + uint8_t visible;
>> + uint8_t pad;
>> + uint32_t pad2;
>> +};
>
> What is pad2 good/intended for? 32-bit padding fields in some
> other structures are needed because one or more uint64_t
> fields follow, but this isn't the case here.
Right, pad2 can be dropped.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-02-18 8:13 ` Alexandru Stefan ISAILA
@ 2020-02-18 14:42 ` Alexandru Stefan ISAILA
2020-02-18 14:47 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2020-02-18 14:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Jun Nakajima, xen-devel, Roger Pau Monné
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>>> goto out;
>>> }
>>>
>>> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>>> + {
>>> + rv = -ENOMEM;
>>> + goto out;
>>> + }
>>
>> Isn't there a pre-existing error handling issue here which you
>> widen, in that later encountered errors don't cause clean up
>> of what had already succeeded before?
>
> It seems non of the errors perform a cleanup. It might be better to have
> a general cleanup done at "out:" if ( !rv ) and then check what should
> be "p2m_teardown()" or "free_xenheap_page()".
>
I've looked around for this and it is handled in arch_domain_create().
If hvm_domain_initialise() fails then paging_final_teardown() is called
and in the end hap_final_teardown() takes care of cleaning up.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility
2020-02-18 14:42 ` Alexandru Stefan ISAILA
@ 2020-02-18 14:47 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2020-02-18 14:47 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Jun Nakajima, xen-devel, Roger Pau Monné
On 18.02.2020 15:42, Alexandru Stefan ISAILA wrote:
>
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>>>> goto out;
>>>> }
>>>>
>>>> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>>>> + {
>>>> + rv = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>
>>> Isn't there a pre-existing error handling issue here which you
>>> widen, in that later encountered errors don't cause clean up
>>> of what had already succeeded before?
>>
>> It seems non of the errors perform a cleanup. It might be better to have
>> a general cleanup done at "out:" if ( !rv ) and then check what should
>> be "p2m_teardown()" or "free_xenheap_page()".
>>
>
> I've looked around for this and it is handled in arch_domain_create().
> If hvm_domain_initialise() fails then paging_final_teardown() is called
> and in the end hap_final_teardown() takes care of cleaning up.
Ah, good - thanks for checking. This code could be more obviously
correct, though.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-18 14:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-30 13:07 [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility Alexandru Stefan ISAILA
2020-02-07 8:54 ` Alexandru Stefan ISAILA
2020-02-17 11:52 ` [Xen-devel] Ping: " Alexandru Stefan ISAILA
2020-02-17 14:14 ` [Xen-devel] " Jan Beulich
2020-02-18 8:13 ` Alexandru Stefan ISAILA
2020-02-18 14:42 ` Alexandru Stefan ISAILA
2020-02-18 14:47 ` Jan Beulich
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.