* [PATCH v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
@ 2017-10-05 15:42 Petre Pircalabu
2017-10-06 15:34 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Petre Pircalabu @ 2017-10-05 15:42 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, sstabellini, wei.liu2, Razvan Cojocaru,
konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim,
jbeulich
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
For the default EPT view we have xc_set_mem_access_multi(), which
is able to set an array of pages to an array of access rights with
a single hypercall. However, this functionality was lacking for the
altp2m subsystem, which could only set page restrictions for one
page at a time. This patch addresses the gap.
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
Changed since v2:
* Added support for compat arguments translation
---
tools/libxc/include/xenctrl.h | 3 ++
tools/libxc/xc_altp2m.c | 41 ++++++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 76 +++++++++++++++++++++++++++++++++++++++-
xen/include/Makefile | 1 +
xen/include/public/hvm/hvm_op.h | 30 +++++++++++++---
xen/include/xlat.lst | 1 +
| 1 +
7 files changed, 147 insertions(+), 6 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bcab3c..4e2ce64 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1971,6 +1971,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t gfn,
xenmem_access_t access);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
+ uint16_t view_id, uint8_t *access,
+ uint64_t *pages, uint32_t nr);
int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t old_gfn,
xen_pfn_t new_gfn);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632..f202ca1 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
return rc;
}
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
+ uint16_t view_id, uint8_t *access,
+ uint64_t *pages, uint32_t nr)
+{
+ int rc;
+
+ DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+ DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+ DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
+ if ( arg == NULL )
+ return -1;
+
+ arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+ arg->cmd = HVMOP_altp2m_set_mem_access_multi;
+ arg->domain = domid;
+ arg->u.set_mem_access_multi.view = view_id;
+ arg->u.set_mem_access_multi.nr = nr;
+
+ if ( xc_hypercall_bounce_pre(xch, pages) ||
+ xc_hypercall_bounce_pre(xch, access) )
+ {
+ PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
+ return -1;
+ }
+
+ set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
+ set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
+
+ rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+ xc_hypercall_buffer_free(xch, arg);
+ xc_hypercall_bounce_post(xch, access);
+ xc_hypercall_bounce_post(xch, pages);
+
+ return rc;
+}
+
int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t old_gfn,
xen_pfn_t new_gfn)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb..1f4358c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -73,6 +73,8 @@
#include <public/arch-x86/cpuid.h>
#include <asm/cpuid.h>
+#include <compat/hvm/hvm_op.h>
+
bool_t __read_mostly hvm_enabled;
#ifdef DBG_LEVEL_0
@@ -4451,6 +4453,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_destroy_p2m:
case HVMOP_altp2m_switch_p2m:
case HVMOP_altp2m_set_mem_access:
+ case HVMOP_altp2m_set_mem_access_multi:
case HVMOP_altp2m_change_gfn:
break;
default:
@@ -4568,6 +4571,30 @@ static int do_altp2m_op(
a.u.set_mem_access.view);
break;
+ case HVMOP_altp2m_set_mem_access_multi:
+ if ( a.u.set_mem_access_multi.pad ||
+ a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
+ a.u.set_mem_access_multi.access_list,
+ a.u.set_mem_access_multi.nr,
+ a.u.set_mem_access_multi.opaque,
+ MEMOP_CMD_MASK,
+ a.u.set_mem_access_multi.view);
+ if ( rc > 0 )
+ {
+ a.u.set_mem_access_multi.opaque = rc;
+ if ( __copy_to_guest(arg, &a, 1) )
+ rc = -EFAULT;
+ else
+ rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+ HVMOP_altp2m, arg);
+ }
+ break;
+
case HVMOP_altp2m_change_gfn:
if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
rc = -EINVAL;
@@ -4586,6 +4613,53 @@ static int do_altp2m_op(
return rc;
}
+static int compat_altp2m_op(
+ XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+ struct compat_hvm_altp2m_op a;
+ union
+ {
+ XEN_GUEST_HANDLE_PARAM(void) hnd;
+ struct xen_hvm_altp2m_op *altp2m_op;
+ } nat;
+
+ if ( !hvm_altp2m_supported() )
+ return -EOPNOTSUPP;
+
+ if ( copy_from_guest(&a, arg, 1) )
+ return -EFAULT;
+
+ if ( a.pad1 || a.pad2 ||
+ (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
+ return -EINVAL;
+
+ set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
+
+ switch ( a.cmd )
+ {
+ case HVMOP_altp2m_set_mem_access_multi:
+#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
+ guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
+#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
+ guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
+ XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
+ &a.u.set_mem_access_multi);
+#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
+#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
+ break;
+ default:
+ return do_altp2m_op(arg);
+ }
+
+ nat.altp2m_op->version = a.version;
+ nat.altp2m_op->cmd = a.cmd;
+ nat.altp2m_op->domain = a.domain;
+ nat.altp2m_op->pad1 = a.pad1;
+ nat.altp2m_op->pad2 = a.pad2;
+
+ return do_altp2m_op(nat.hnd);
+}
+
static int hvmop_get_mem_type(
XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
{
@@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
case HVMOP_altp2m:
- rc = do_altp2m_op(arg);
+ rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : do_altp2m_op(arg);
break;
default:
diff --git a/xen/include/Makefile b/xen/include/Makefile
index c90fdee..814b0a8 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -28,6 +28,7 @@ headers-$(CONFIG_X86) += compat/arch-x86/xen.h
headers-$(CONFIG_X86) += compat/arch-x86/xen-$(compat-arch-y).h
headers-$(CONFIG_X86) += compat/hvm/hvm_vcpu.h
headers-$(CONFIG_X86) += compat/hvm/dm_op.h
+headers-$(CONFIG_X86) += compat/hvm/hvm_op.h
headers-y += compat/arch-$(compat-arch-y).h compat/pmu.h compat/xlat.h
headers-$(CONFIG_FLASK) += compat/xsm/flask_op.h
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 0bdafdf..bedce71 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -237,6 +237,23 @@ struct xen_hvm_altp2m_set_mem_access {
typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
+struct xen_hvm_altp2m_set_mem_access_multi {
+ /* view */
+ uint16_t view;
+ uint16_t pad;
+ /* Number of pages */
+ uint32_t nr;
+ /* Used for continuation purposes */
+ uint64_t opaque;
+ /* List of pfns to set access for */
+ XEN_GUEST_HANDLE(const_uint64) pfn_list;
+ /* Corresponding list of access settings for pfn_list */
+ XEN_GUEST_HANDLE(const_uint8) access_list;
+};
+typedef struct xen_hvm_altp2m_set_mem_access_multi
+ xen_hvm_altp2m_set_mem_access_multi_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
+
struct xen_hvm_altp2m_change_gfn {
/* view */
uint16_t view;
@@ -268,15 +285,18 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_set_mem_access 7
/* Change a p2m entry to have a different gfn->mfn mapping */
#define HVMOP_altp2m_change_gfn 8
+/* Set access for an array of pages */
+#define HVMOP_altp2m_set_mem_access_multi 9
domid_t domain;
uint16_t pad1;
uint32_t pad2;
union {
- struct xen_hvm_altp2m_domain_state domain_state;
- struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
- struct xen_hvm_altp2m_view view;
- struct xen_hvm_altp2m_set_mem_access set_mem_access;
- struct xen_hvm_altp2m_change_gfn change_gfn;
+ struct xen_hvm_altp2m_domain_state domain_state;
+ struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
+ struct xen_hvm_altp2m_view view;
+ struct xen_hvm_altp2m_set_mem_access set_mem_access;
+ struct xen_hvm_altp2m_change_gfn change_gfn;
+ struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
uint8_t pad[64];
} u;
};
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 0f17000..750d199 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -73,6 +73,7 @@
? vcpu_hvm_context hvm/hvm_vcpu.h
? vcpu_hvm_x86_32 hvm/hvm_vcpu.h
? vcpu_hvm_x86_64 hvm/hvm_vcpu.h
+! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h
? kexec_exec kexec.h
! kexec_image kexec.h
! kexec_range kexec.h
--git a/xen/tools/compat-build-header.py b/xen/tools/compat-build-header.py
index 32421b6..12b7a6c 100755
--- a/xen/tools/compat-build-header.py
+++ b/xen/tools/compat-build-header.py
@@ -16,6 +16,7 @@ pats = [
[ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
[ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ],
[ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
+ [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
[ r"(^|[^\w])Xen_?", r"\1Compat_" ],
[ r"(^|[^\w])long([^\w]|$$)", r"\1int\2" ]
];
--
2.7.4
_______________________________________________
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-05 15:42 [PATCH v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi() Petre Pircalabu
@ 2017-10-06 15:34 ` Jan Beulich
2017-10-06 16:07 ` Razvan Cojocaru
2017-10-09 10:10 ` Petre Ovidiu PIRCALABU
0 siblings, 2 replies; 9+ messages in thread
From: Jan Beulich @ 2017-10-06 15:34 UTC (permalink / raw)
To: Petre Pircalabu
Cc: tim, sstabellini, wei.liu2, Razvan Cojocaru, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, xen-devel
>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
> @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_destroy_p2m:
> case HVMOP_altp2m_switch_p2m:
> case HVMOP_altp2m_set_mem_access:
> + case HVMOP_altp2m_set_mem_access_multi:
Was it agreed that this, just like others (many wrongly, I think) is
supposed to be invokable by the affected domain itself? Its non-
altp2m counterpart is a DM_PRIV operation. If the one here is
meant to be different, I think the commit message should say why.
> @@ -4568,6 +4571,30 @@ static int do_altp2m_op(
> a.u.set_mem_access.view);
> break;
>
> + case HVMOP_altp2m_set_mem_access_multi:
> + if ( a.u.set_mem_access_multi.pad ||
> + a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> + a.u.set_mem_access_multi.access_list,
> + a.u.set_mem_access_multi.nr,
> + a.u.set_mem_access_multi.opaque,
> + MEMOP_CMD_MASK,
> + a.u.set_mem_access_multi.view);
> + if ( rc > 0 )
> + {
> + a.u.set_mem_access_multi.opaque = rc;
> + if ( __copy_to_guest(arg, &a, 1) )
__copy_field_to_guest() would suffice here afaict.
> @@ -4586,6 +4613,53 @@ static int do_altp2m_op(
> return rc;
> }
>
> +static int compat_altp2m_op(
> + XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> + struct compat_hvm_altp2m_op a;
> + union
> + {
> + XEN_GUEST_HANDLE_PARAM(void) hnd;
> + struct xen_hvm_altp2m_op *altp2m_op;
> + } nat;
> +
> + if ( !hvm_altp2m_supported() )
> + return -EOPNOTSUPP;
> +
> + if ( copy_from_guest(&a, arg, 1) )
> + return -EFAULT;
> +
> + if ( a.pad1 || a.pad2 ||
> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> + return -EINVAL;
Why doesn't suffice what do_altp2m_op() does?
> + set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> + switch ( a.cmd )
> + {
> + case HVMOP_altp2m_set_mem_access_multi:
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> + guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> + guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
> + &a.u.set_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> + break;
> + default:
> + return do_altp2m_op(arg);
> + }
> +
> + nat.altp2m_op->version = a.version;
> + nat.altp2m_op->cmd = a.cmd;
> + nat.altp2m_op->domain = a.domain;
> + nat.altp2m_op->pad1 = a.pad1;
> + nat.altp2m_op->pad2 = a.pad2;
Why do you do this by hand, rather than using XLAT_*() macros
which at the same time check that the field sizes match?
> @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> break;
>
> case HVMOP_altp2m:
> - rc = do_altp2m_op(arg);
> + rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : do_altp2m_op(arg);
Pointless parentheses and spaces. Plus, to be honest, I'm not really
happy about this ad hoc compat handling, but at the same time I
can't suggest a truly better alternative.
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -73,6 +73,7 @@
> ? vcpu_hvm_context hvm/hvm_vcpu.h
> ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h
> ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h
> +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h
Please insert alphabetically, sorted by filename (and then structure
name).
> --- a/xen/tools/compat-build-header.py
> +++ b/xen/tools/compat-build-header.py
> @@ -16,6 +16,7 @@ pats = [
> [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
> [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ],
> [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
> + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
What is this needed for? I can't find any instance of HVMMEM_*
elsewhere in the patch. As you can see, so far there are only
pretty generic tokens being replaced here.
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-06 15:34 ` Jan Beulich
@ 2017-10-06 16:07 ` Razvan Cojocaru
2017-10-09 7:23 ` Jan Beulich
2017-10-09 10:10 ` Petre Ovidiu PIRCALABU
1 sibling, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2017-10-06 16:07 UTC (permalink / raw)
To: Jan Beulich, Petre Pircalabu
Cc: tim, sstabellini, wei.liu2, konrad.wilk, George.Dunlap,
andrew.cooper3, ian.jackson, xen-devel
On 10/06/2017 06:34 PM, Jan Beulich wrote:
>>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
>> @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
>> case HVMOP_altp2m_destroy_p2m:
>> case HVMOP_altp2m_switch_p2m:
>> case HVMOP_altp2m_set_mem_access:
>> + case HVMOP_altp2m_set_mem_access_multi:
>
> Was it agreed that this, just like others (many wrongly, I think) is
> supposed to be invokable by the affected domain itself? Its non-
> altp2m counterpart is a DM_PRIV operation. If the one here is
> meant to be different, I think the commit message should say why.
In the absence of an answer from the designers of altp2m, we've chosen
to remain consistent with HVMOP_altp2m_set_mem_access - since that is
allowed to be invoked by the domain itself, this operation is also
allowed to do that.
Back in March, I've sent a DOMCTL version:
https://patchwork.kernel.org/patch/9633615/
and a HVMOP version (minus the compat part):
https://patchwork.kernel.org/patch/9612799/
It has been discussed, and an authoritative answer on the design of this
was sought out, but despite several kind reminders during this time, it
never came. At this point, the least modification to the initial design
appears to be to keep the new operation as a HVMOP. This is an important
optimization, and the waiting period for objections has surely been
reasonable.
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-06 16:07 ` Razvan Cojocaru
@ 2017-10-09 7:23 ` Jan Beulich
2017-10-09 7:25 ` Razvan Cojocaru
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2017-10-09 7:23 UTC (permalink / raw)
To: Petre Pircalabu, Razvan Cojocaru
Cc: tim, sstabellini, wei.liu2, konrad.wilk, George.Dunlap,
andrew.cooper3, ian.jackson, xen-devel
>>> On 06.10.17 at 18:07, <rcojocaru@bitdefender.com> wrote:
> On 10/06/2017 06:34 PM, Jan Beulich wrote:
>>>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
>>> @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
>>> case HVMOP_altp2m_destroy_p2m:
>>> case HVMOP_altp2m_switch_p2m:
>>> case HVMOP_altp2m_set_mem_access:
>>> + case HVMOP_altp2m_set_mem_access_multi:
>>
>> Was it agreed that this, just like others (many wrongly, I think) is
>> supposed to be invokable by the affected domain itself? Its non-
>> altp2m counterpart is a DM_PRIV operation. If the one here is
>> meant to be different, I think the commit message should say why.
>
> In the absence of an answer from the designers of altp2m, we've chosen
> to remain consistent with HVMOP_altp2m_set_mem_access - since that is
> allowed to be invoked by the domain itself, this operation is also
> allowed to do that.
>
> Back in March, I've sent a DOMCTL version:
>
> https://patchwork.kernel.org/patch/9633615/
>
> and a HVMOP version (minus the compat part):
>
> https://patchwork.kernel.org/patch/9612799/
>
> It has been discussed, and an authoritative answer on the design of this
> was sought out, but despite several kind reminders during this time, it
> never came. At this point, the least modification to the initial design
> appears to be to keep the new operation as a HVMOP. This is an important
> optimization, and the waiting period for objections has surely been
> reasonable.
Okay, this is (sort of) fine, but especially when there was no
feedback the decision taken (and its reason) should be recorded
in the commit message. As stated above as well as earlier, I
strongly think that the altp2m permissions are too lax right now
(and hence the patch here widens the problem, but I can agree
that making it match set_mem_access is not unreasonable).
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-09 7:23 ` Jan Beulich
@ 2017-10-09 7:25 ` Razvan Cojocaru
0 siblings, 0 replies; 9+ messages in thread
From: Razvan Cojocaru @ 2017-10-09 7:25 UTC (permalink / raw)
To: Jan Beulich, Petre Pircalabu
Cc: tim, sstabellini, wei.liu2, konrad.wilk, George.Dunlap,
andrew.cooper3, ian.jackson, xen-devel
On 09.10.2017 10:23, Jan Beulich wrote:
>>>> On 06.10.17 at 18:07, <rcojocaru@bitdefender.com> wrote:
>> On 10/06/2017 06:34 PM, Jan Beulich wrote:
>>>>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
>>>> @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
>>>> case HVMOP_altp2m_destroy_p2m:
>>>> case HVMOP_altp2m_switch_p2m:
>>>> case HVMOP_altp2m_set_mem_access:
>>>> + case HVMOP_altp2m_set_mem_access_multi:
>>>
>>> Was it agreed that this, just like others (many wrongly, I think) is
>>> supposed to be invokable by the affected domain itself? Its non-
>>> altp2m counterpart is a DM_PRIV operation. If the one here is
>>> meant to be different, I think the commit message should say why.
>>
>> In the absence of an answer from the designers of altp2m, we've chosen
>> to remain consistent with HVMOP_altp2m_set_mem_access - since that is
>> allowed to be invoked by the domain itself, this operation is also
>> allowed to do that.
>>
>> Back in March, I've sent a DOMCTL version:
>>
>> https://patchwork.kernel.org/patch/9633615/
>>
>> and a HVMOP version (minus the compat part):
>>
>> https://patchwork.kernel.org/patch/9612799/
>>
>> It has been discussed, and an authoritative answer on the design of this
>> was sought out, but despite several kind reminders during this time, it
>> never came. At this point, the least modification to the initial design
>> appears to be to keep the new operation as a HVMOP. This is an important
>> optimization, and the waiting period for objections has surely been
>> reasonable.
>
> Okay, this is (sort of) fine, but especially when there was no
> feedback the decision taken (and its reason) should be recorded
> in the commit message. As stated above as well as earlier, I
> strongly think that the altp2m permissions are too lax right now
> (and hence the patch here widens the problem, but I can agree
> that making it match set_mem_access is not unreasonable).
Of course. We'll update the commit message. Thanks for the review!
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-06 15:34 ` Jan Beulich
2017-10-06 16:07 ` Razvan Cojocaru
@ 2017-10-09 10:10 ` Petre Ovidiu PIRCALABU
2017-10-09 10:36 ` Jan Beulich
1 sibling, 1 reply; 9+ messages in thread
From: Petre Ovidiu PIRCALABU @ 2017-10-09 10:10 UTC (permalink / raw)
To: JBeulich
Cc: tim, sstabellini, wei.liu2, rcojocaru, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, xen-devel
On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
> > @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
> > case HVMOP_altp2m_destroy_p2m:
> > case HVMOP_altp2m_switch_p2m:
> > case HVMOP_altp2m_set_mem_access:
> > + case HVMOP_altp2m_set_mem_access_multi:
> Was it agreed that this, just like others (many wrongly, I think) is
> supposed to be invokable by the affected domain itself? Its non-
> altp2m counterpart is a DM_PRIV operation. If the one here is
> meant to be different, I think the commit message should say why.
Will be corrected in the new patch iteration.
>
> >
> > @@ -4568,6 +4571,30 @@ static int do_altp2m_op(
> > a.u.set_mem_access.view);
> > break;
> >
> > + case HVMOP_altp2m_set_mem_access_multi:
> > + if ( a.u.set_mem_access_multi.pad ||
> > + a.u.set_mem_access_multi.opaque >=
> > a.u.set_mem_access_multi.nr )
> > + {
> > + rc = -EINVAL;
> > + break;
> > + }
> > + rc = p2m_set_mem_access_multi(d,
> > a.u.set_mem_access_multi.pfn_list,
> > + a.u.set_mem_access_multi.acc
> > ess_list,
> > + a.u.set_mem_access_multi.nr,
> > + a.u.set_mem_access_multi.opa
> > que,
> > + MEMOP_CMD_MASK,
> > + a.u.set_mem_access_multi.vie
> > w);
> > + if ( rc > 0 )
> > + {
> > + a.u.set_mem_access_multi.opaque = rc;
> > + if ( __copy_to_guest(arg, &a, 1) )
> __copy_field_to_guest() would suffice here afaict.
Will be corrected in the new patch iteration.
>
> >
> > @@ -4586,6 +4613,53 @@ static int do_altp2m_op(
> > return rc;
> > }
> >
> > +static int compat_altp2m_op(
> > + XEN_GUEST_HANDLE_PARAM(void) arg)
> > +{
> > + struct compat_hvm_altp2m_op a;
> > + union
> > + {
> > + XEN_GUEST_HANDLE_PARAM(void) hnd;
> > + struct xen_hvm_altp2m_op *altp2m_op;
> > + } nat;
> > +
> > + if ( !hvm_altp2m_supported() )
> > + return -EOPNOTSUPP;
> > +
> > + if ( copy_from_guest(&a, arg, 1) )
> > + return -EFAULT;
> > +
> > + if ( a.pad1 || a.pad2 ||
> > + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> > + return -EINVAL;
> Why doesn't suffice what do_altp2m_op() does?
The sanity checks are the same as for do_altp2m_op but it wanted to
check as early as possible for invalid arguments.
>
> >
> > + set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> > +
> > + switch ( a.cmd )
> > + {
> > + case HVMOP_altp2m_set_mem_access_multi:
> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_,
> > _s_); \
> > + guest_from_compat_handle((_d_)->pfn_list, (_s_)-
> > >pfn_list)
> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_,
> > _s_); \
> > + guest_from_compat_handle((_d_)->access_list, (_s_)-
> > >access_list)
> > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op-
> > >u.set_mem_access_multi,
> > + &a.u.set_mem_access_multi);
> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> > + break;
> > + default:
> > + return do_altp2m_op(arg);
> > + }
> > +
> > + nat.altp2m_op->version = a.version;
> > + nat.altp2m_op->cmd = a.cmd;
> > + nat.altp2m_op->domain = a.domain;
> > + nat.altp2m_op->pad1 = a.pad1;
> > + nat.altp2m_op->pad2 = a.pad2;
> Why do you do this by hand, rather than using XLAT_*() macros
> which at the same time check that the field sizes match?
Actually, there is a problem with the XLAT_hvm_altp2m_op macro
generation.
The current definition of struct xen_hvm_altp2m_op uses "structs" as
member of the union. In this case the enum values for switch(u) are not
generated.
#define XLAT_hvm_altp2m_op(_d_, _s_) do { \
(_d_)->version = (_s_)->version; \
(_d_)->cmd = (_s_)->cmd; \
(_d_)->domain = (_s_)->dWill be corrected in the new patch
iteration.omain; \
(_d_)->pad1 = (_s_)->pad1; \
(_d_)->pad2 = (_s_)->pad2; \
switch (u) { \
case XLAT_hvm_altp2m_op_u_domain_state: \
XLAT_hvm_altp2m_domain_state(&(_d_)->u.domain_state, &(_s_)-
>u.domain_state); \
break; \
case
XLAT_hvm_alxen_hvm_altp2m_set_mem_access_multi_ttp2m_op_u_enable_notify
: \
XLAT_hvm_altp2m_vcpu_enable_notify(&(_d_)->u.enable_notify,
&(_s_)->u.enable_notifyxen_hvm_altp2m_set_mem_access_multi_t); \
break; \
case XLAT_hvm_altp2m_op_u_view: \
XLAT_hvm_altp2m_view(&(_d_)->u.view, &(_s_)->u.view); \
break; \Will be corrected in the new patch iteration.
case XLAT_hvm_altp2m_op_u_set_mem_access: \
XLAT_hvm_altp2m_set_mem_access(&(_d_)->u.set_mem_access,
&(_s_)->u.set_mem_access); \
break; \
case XLAT_hvm_altp2m_op_u_change_gfn: \
XLAT_hvm_altp2m_change_gfn(&(_d_)->u.change_gfn, &(_s_)-
>u.change_gfn); \
break; \
case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \
XLAT_hvm_altp2m_set_mem_access_multi(&(_d_)-
>u.set_mem_access_multi, &(_s_)->u.set_mem_access_multi); \
break; \
case XLAT_hvm_altp2m_op_u_pad: \
(_d_)->u.pad = (_s_)->u.pad; \
break; \
} \
} while (0)
If the "structs" are replaced with the corresponding typedefs in the
definition of xen_hvm_altp2m_op
(e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct
xen_hvm_altp2m_domain_state), the enum values are generated correctly
but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with a
simple assignment, thus breaking the build (
compat_hvm_altp2m_set_mem_access_multi_t
to xen_hvm_altp2m_set_mem_access_multi_t assignment)
enum XLAT_hvm_altp2m_op_u {
XLAT_hvm_altp2m_op_u_domain_state,
XLAT_hvm_altp2m_op_u_enable_notify,
XLAT_hvm_altp2m_op_u_view,
XLAT_hvm_altp2m_op_u_set_mem_access,
XLAT_hvm_altp2m_op_u_change_gfn,
XLAT_hvm_altp2m_op_u_set_mem_access_multi,
XLAT_hvm_altp2m_op_u_pad,
};
#define XLAT_hvm_altp2m_op(_d_, _s_) do { \
(_d_)->version = (_s_)->version; \
(_d_)->cmd = (_s_)->cmd; \
(_d_)->domain = (_s_)->domain; \
(_d_)->pad1 = (_s_)->pad1; \
(_d_)->pad2 = (_s_)->pad2; \
switch (u) { \
case XLAT_hvm_altp2m_op_u_domain_state: \
(_d_)->u.domain_state = (_s_)->u.domain_state; \
break; \
case XLAT_hvm_altp2m_op_u_enable_notify: \
(_d_)->u.enable_notify = (_s_)->u.enable_notify; \
break; \
case XLAT_hvm_altp2m_op_u_view: \
(_d_)->u.view = (_s_)->u.view; \
break; \
case XLAT_hvm_altp2m_op_u_set_mem_access: \
(_d_)->u.set_mem_access = (_s_)->u.set_mem_access; \
break; \
case XLAT_hvm_altp2m_op_u_change_gfn: \
(_d_)->u.change_gfn = (_s_)->u.change_gfn; \
break; \
case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \
(_d_)->u.set_mem_access_multi = (_s_)->u.set_mem_access_multi;
\
break; \
case XLAT_hvm_altp2m_op_u_pad: \
(_d_)->u.pad = (_s_)->u.pad; \
break; \
} \
} while (0)
At this stage the easiest approach was to set the values by hand.
>
> >
> > @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> > break;
> >
> > case HVMOP_altp2m:
> > - rc = do_altp2m_op(arg);
> > + rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) :
> > do_altp2m_op(arg);
> Pointless parentheses and spaces. Plus, to be honest, I'm not really
> happy about this ad hoc compat handling, but at the same time I
> can't suggest a truly better alternative.
>
Removed the spaces. The alternative (e.g. changing hvm_hypercall_table
to use COMPAT_CALL(hvm_op) and defining a compat_hvm_op function in ch
only altp2m is handled differently) is uglier in my opinion because
only HVMOP_altp2m_set_mem_access_multi requires COMPAT argument
translation.
> >
> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -73,6 +73,7 @@
> > ? vcpu_hvm_context hvm/hvm_vcpu.h
> > ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h
> > ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h
> > +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h
> Please insert alphabetically, sorted by filename (and then structure
> name).
Will be corrected in the new patch iteration.
>
> >
> > --- a/xen/tools/compat-build-header.py
> > +++ b/xen/tools/compat-build-header.py
> > @@ -16,6 +16,7 @@ pats = [
> > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
> > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3"
> > ],
> > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
> > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
> What is this needed for? I can't find any instance of HVMMEM_*
> elsewhere in the patch. As you can see, so far there are only
> pretty generic tokens being replaced here.
>
Without this, both hvmmem_type_t and hvmmem_type_compat_t enums will
define the same values (e.g. HVMMEM_ram_rw,) resulting in a compile
error. By adding this translation we will have a COMPAT_HVMMEM value
for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g.
HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw)
Many thanks for your support,
Petre
> Jan
>
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-09 10:10 ` Petre Ovidiu PIRCALABU
@ 2017-10-09 10:36 ` Jan Beulich
2017-10-09 11:19 ` Petre Ovidiu PIRCALABU
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2017-10-09 10:36 UTC (permalink / raw)
To: Petre Ovidiu PIRCALABU
Cc: tim, sstabellini, wei.liu2, rcojocaru, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, xen-devel
>>> On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote:
> On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
>> > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
>> > + switch ( a.cmd )
>> > + {
>> > + case HVMOP_altp2m_set_mem_access_multi:
>> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_,
>> > _s_); \
>> > + guest_from_compat_handle((_d_)->pfn_list, (_s_)-
>> > >pfn_list)
>> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_,
>> > _s_); \
>> > + guest_from_compat_handle((_d_)->access_list, (_s_)-
>> > >access_list)
>> > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op-
>> > >u.set_mem_access_multi,
>> > + &a.u.set_mem_access_multi);
>> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
>> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
>> > + break;
>> > + default:
>> > + return do_altp2m_op(arg);
>> > + }
>> > +
>> > + nat.altp2m_op->version = a.version;
>> > + nat.altp2m_op->cmd = a.cmd;
>> > + nat.altp2m_op->domain = a.domain;
>> > + nat.altp2m_op->pad1 = a.pad1;
>> > + nat.altp2m_op->pad2 = a.pad2;
>> Why do you do this by hand, rather than using XLAT_*() macros
>> which at the same time check that the field sizes match?
> Actually, there is a problem with the XLAT_hvm_altp2m_op macro
> generation.
> The current definition of struct xen_hvm_altp2m_op uses "structs" as
> member of the union. In this case the enum values for switch(u) are not
> generated.
>[...]
> If the "structs" are replaced with the corresponding typedefs in the
> definition of xen_hvm_altp2m_op
> (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct
> xen_hvm_altp2m_domain_state), the enum values are generated correctly
> but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with a
> simple assignment, thus breaking the build (
> compat_hvm_altp2m_set_mem_access_multi_t
> to xen_hvm_altp2m_set_mem_access_multi_t assignment)
>[...]
> At this stage the easiest approach was to set the values by hand.
Okay, but then please add a comment to say so (after all it should
really be the script that gets adjusted in order to correctly deal
with the cases) and add the missing size checks (and whatever
else verification the macros may do).
>> > --- a/xen/tools/compat-build-header.py
>> > +++ b/xen/tools/compat-build-header.py
>> > @@ -16,6 +16,7 @@ pats = [
>> > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
>> > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3"
>> > ],
>> > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
>> > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
>> What is this needed for? I can't find any instance of HVMMEM_*
>> elsewhere in the patch. As you can see, so far there are only
>> pretty generic tokens being replaced here.
>>
> Without this, both hvmmem_type_t and hvmmem_type_compat_t enums will
> define the same values (e.g. HVMMEM_ram_rw,) resulting in a compile
> error. By adding this translation we will have a COMPAT_HVMMEM value
> for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g.
> HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw)
That's ugly. I realize that we shouldn't even attempt to translate
enumerations (or to be fully precise, there shouldn't be any
enumerations in the public interface in the first place), as the
enumerator values ought to remain consistent between native
and compat uses. Hence we could either convert the enum to a
set of #define-s, or we would need a mechanism to exclude
parts of a header from the compat conversion.
In the end the problem here is because of the enumerators,
other than xenmem_access_t's, aren't properly prefixed with
XEN or XEN_ (or else the script would already handle them fine
afaict). So another variant of addressing this would be to
deprecate (but not remove) the current names, introducing
properly named ones for __XEN_INTERFACE_VERSION__ >=
0x00040a00.
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-09 10:36 ` Jan Beulich
@ 2017-10-09 11:19 ` Petre Ovidiu PIRCALABU
2017-10-09 11:56 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Petre Ovidiu PIRCALABU @ 2017-10-09 11:19 UTC (permalink / raw)
To: JBeulich
Cc: tim, sstabellini, wei.liu2, rcojocaru, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, xen-devel
On Lu, 2017-10-09 at 04:36 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote:
> > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
> > > > + switch ( a.cmd )
> > > > + {
> > > > + case HVMOP_altp2m_set_mem_access_multi:
> > > > +#define
> > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_,
> > > > _s_); \
> > > > + guest_from_compat_handle((_d_)->pfn_list, (_s_)-
> > > > >
> > > > > pfn_list)
> > > > +#define
> > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_,
> > > > _s_); \
> > > > + guest_from_compat_handle((_d_)->access_list,
> > > > (_s_)-
> > > > >
> > > > > access_list)
> > > > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_o
> > > > p-
> > > > >
> > > > > u.set_mem_access_multi,
> > > > + &a.u.set_mem_access_multi);
> > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> > > > + break;
> > > > + default:
> > > > + return do_altp2m_op(arg);
> > > > + }
> > > > +
> > > > + nat.altp2m_op->version = a.version;
> > > > + nat.altp2m_op->cmd = a.cmd;
> > > > + nat.altp2m_op->domain = a.domain;
> > > > + nat.altp2m_op->pad1 = a.pad1;
> > > > + nat.altp2m_op->pad2 = a.pad2;
> > > Why do you do this by hand, rather than using XLAT_*() macros
> > > which at the same time check that the field sizes match?
> > Actually, there is a problem with the XLAT_hvm_altp2m_op macro
> > generation.
> > The current definition of struct xen_hvm_altp2m_op uses "structs"
> > as
> > member of the union. In this case the enum values for switch(u) are
> > not
> > generated.
> > [...]
> > If the "structs" are replaced with the corresponding typedefs in
> > the
> > definition of xen_hvm_altp2m_op
> > (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct
> > xen_hvm_altp2m_domain_state), the enum values are generated
> > correctly
> > but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with
> > a
> > simple assignment, thus breaking the build (
> > compat_hvm_altp2m_set_mem_access_multi_t
> > to xen_hvm_altp2m_set_mem_access_multi_t assignment)
> > [...]
> > At this stage the easiest approach was to set the values by hand.
> Okay, but then please add a comment to say so (after all it should
> really be the script that gets adjusted in order to correctly deal
> with the cases) and add the missing size checks (and whatever
> else verification the macros may do).
Will fix in in the new patch iteration.
>
> >
> > >
> > > >
> > > > --- a/xen/tools/compat-build-header.py
> > > > +++ b/xen/tools/compat-build-header.py
> > > > @@ -16,6 +16,7 @@ pats = [
> > > > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
> > > > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)",
> > > > r"\1compat_\2_t\3"
> > > > ],
> > > > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
> > > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
> > > What is this needed for? I can't find any instance of HVMMEM_*
> > > elsewhere in the patch. As you can see, so far there are only
> > > pretty generic tokens being replaced here.
> > >
> > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums
> > will
> > define the same values (e.g. HVMMEM_ram_rw,) resulting in a
> > compile
> > error. By adding this translation we will have a COMPAT_HVMMEM
> > value
> > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g.
> > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw)
> That's ugly. I realize that we shouldn't even attempt to translate
> enumerations (or to be fully precise, there shouldn't be any
> enumerations in the public interface in the first place), as the
> enumerator values ought to remain consistent between native
> and compat uses. Hence we could either convert the enum to a
> set of #define-s, or we would need a mechanism to exclude
> parts of a header from the compat conversion.
>
> In the end the problem here is because of the enumerators,
> other than xenmem_access_t's, aren't properly prefixed with
> XEN or XEN_ (or else the script would already handle them fine
> afaict). So another variant of addressing this would be to
> deprecate (but not remove) the current names, introducing
> properly named ones for __XEN_INTERFACE_VERSION__ >=
> 0x00040a00.
>
Unfortunately the enum is referenced also in other functions
(e.g. xendevicemodel_set_mem_type) so replacing it with #defines would
be more difficult.
When generating the compat headers,-DXEN_GENERATING_COMPAT_HEADERS is
defined (xen/include/Makefile). I can guard hvmmem_type_t with an
#ifndef XEN_GENERATING_COMPAT_HEADERS so the enum is not processed by
the compat-build-header.py script. (in my opinion this is the minimum-
impact approach)
Do you agree with this?
Many thanks,
Petre
> Jan
>
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
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 v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-10-09 11:19 ` Petre Ovidiu PIRCALABU
@ 2017-10-09 11:56 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2017-10-09 11:56 UTC (permalink / raw)
To: Petre Ovidiu PIRCALABU
Cc: tim, sstabellini, wei.liu2, rcojocaru, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, xen-devel
>>> On 09.10.17 at 13:19, <ppircalabu@bitdefender.com> wrote:
> On Lu, 2017-10-09 at 04:36 -0600, Jan Beulich wrote:
>> > > > On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote:
>> > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
>> > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
>> > > > --- a/xen/tools/compat-build-header.py
>> > > > +++ b/xen/tools/compat-build-header.py
>> > > > @@ -16,6 +16,7 @@ pats = [
>> > > > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
>> > > > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)",
>> > > > r"\1compat_\2_t\3"
>> > > > ],
>> > > > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
>> > > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
>> > > What is this needed for? I can't find any instance of HVMMEM_*
>> > > elsewhere in the patch. As you can see, so far there are only
>> > > pretty generic tokens being replaced here.
>> > >
>> > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums
>> > will
>> > define the same values (e.g. HVMMEM_ram_rw,) resulting in a
>> > compile
>> > error. By adding this translation we will have a COMPAT_HVMMEM
>> > value
>> > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g.
>> > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw)
>> That's ugly. I realize that we shouldn't even attempt to translate
>> enumerations (or to be fully precise, there shouldn't be any
>> enumerations in the public interface in the first place), as the
>> enumerator values ought to remain consistent between native
>> and compat uses. Hence we could either convert the enum to a
>> set of #define-s, or we would need a mechanism to exclude
>> parts of a header from the compat conversion.
>>
>> In the end the problem here is because of the enumerators,
>> other than xenmem_access_t's, aren't properly prefixed with
>> XEN or XEN_ (or else the script would already handle them fine
>> afaict). So another variant of addressing this would be to
>> deprecate (but not remove) the current names, introducing
>> properly named ones for __XEN_INTERFACE_VERSION__ >=
>> 0x00040a00.
>>
> Unfortunately the enum is referenced also in other functions
> (e.g. xendevicemodel_set_mem_type) so replacing it with #defines would
> be more difficult.
> When generating the compat headers,-DXEN_GENERATING_COMPAT_HEADERS is
> defined (xen/include/Makefile). I can guard hvmmem_type_t with an
> #ifndef XEN_GENERATING_COMPAT_HEADERS so the enum is not processed by
> the compat-build-header.py script. (in my opinion this is the minimum-
> impact approach)
> Do you agree with this?
I'm not entirely happy about that approach, but it'll do for the
moment.
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:[~2017-10-09 11:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-05 15:42 [PATCH v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi() Petre Pircalabu
2017-10-06 15:34 ` Jan Beulich
2017-10-06 16:07 ` Razvan Cojocaru
2017-10-09 7:23 ` Jan Beulich
2017-10-09 7:25 ` Razvan Cojocaru
2017-10-09 10:10 ` Petre Ovidiu PIRCALABU
2017-10-09 10:36 ` Jan Beulich
2017-10-09 11:19 ` Petre Ovidiu PIRCALABU
2017-10-09 11:56 ` 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.