Re: [Xen-devel] [PATCH 01/11] public / x86: introduce hvmctl hypercall

[Andrew Cooper]

On 23/06/16 16:10, Jan Beulich wrote:
 On 23.06.16 at 16:55,  wrote:
>> On 20/06/16 13:52, Jan Beulich wrote:
>>> +/*
>>> + * Note that this value is effectively part of the ABI, even if we don't 
>> need
>>> + * to make it a formal part of it.  Hence this value may only be changed if
>>> + * accompanied by a suitable interface version increase.
>>> + */
>>> +#define HVMCTL_iter_shift 8
>>> +#define HVMCTL_iter_mask  ((1U << HVMCTL_iter_shift) - 1)
>>> +#define HVMCTL_iter_max   (1U << (16 + HVMCTL_iter_shift))
>> This (mis)use of the cmd parameter is surely no longer necessary, given
>> that there is space in xen_hvmctl_t to encode continuation information?
> There's no misuse of cmd anymore. This is just use to make the 16-bit
> continuation value (the opaque structure member) cover a more useful
> range, and at once avoid doing the preemption check on every
> iteration.

Ah ok, but it does leave the minimum iteration at 256, which could
easily be too large, depending on the underlying operation.

In this case, I think it would be far better to bump the cmd field to 32
bits, and opaque to 64bits, which affords us far more flexibility.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 01/11] public / x86: introduce hvmctl hypercall

[Jan Beulich]

>>> On 23.06.16 at 16:55,  wrote:
> On 20/06/16 13:52, Jan Beulich wrote:
>> +/*
>> + * Note that this value is effectively part of the ABI, even if we don't 
> need
>> + * to make it a formal part of it.  Hence this value may only be changed if
>> + * accompanied by a suitable interface version increase.
>> + */
>> +#define HVMCTL_iter_shift 8
>> +#define HVMCTL_iter_mask  ((1U << HVMCTL_iter_shift) - 1)
>> +#define HVMCTL_iter_max   (1U << (16 + HVMCTL_iter_shift))
> 
> This (mis)use of the cmd parameter is surely no longer necessary, given
> that there is space in xen_hvmctl_t to encode continuation information?

There's no misuse of cmd anymore. This is just use to make the 16-bit
continuation value (the opaque structure member) cover a more useful
range, and at once avoid doing the preemption check on every
iteration.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 01/11] public / x86: introduce hvmctl hypercall

[Andrew Cooper]

On 20/06/16 13:52, Jan Beulich wrote:
> +/*
> + * Note that this value is effectively part of the ABI, even if we don't need
> + * to make it a formal part of it.  Hence this value may only be changed if
> + * accompanied by a suitable interface version increase.
> + */
> +#define HVMCTL_iter_shift 8
> +#define HVMCTL_iter_mask  ((1U << HVMCTL_iter_shift) - 1)
> +#define HVMCTL_iter_max   (1U << (16 + HVMCTL_iter_shift))

This (mis)use of the cmd parameter is surely no longer necessary, given
that there is space in xen_hvmctl_t to encode continuation information?

~Andrew


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 01/11] public / x86: introduce hvmctl hypercall

[Wei Liu]

On Mon, Jun 20, 2016 at 06:52:41AM -0600, Jan Beulich wrote:
> ... as a means to replace all HVMOP_* which a domain can't issue on
> itself (i.e. intended for use by only the control domain or device
> model).
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 01/11] public / x86: introduce hvmctl hypercall

[Jan Beulich]

... as a means to replace all HVMOP_* which a domain can't issue on
itself (i.e. intended for use by only the control domain or device
model).

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -2,6 +2,7 @@ subdir-y += svm
 subdir-y += vmx
 
 obj-y += asid.o
+obj-y += control.o
 obj-y += emulate.o
 obj-y += event.o
 obj-y += hpet.o
--- /dev/null
+++ b/xen/arch/x86/hvm/control.c
@@ -0,0 +1,96 @@
+/*
+ * control.c: Hardware virtual machine control operations.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Note that this value is effectively part of the ABI, even if we don't need
+ * to make it a formal part of it.  Hence this value may only be changed if
+ * accompanied by a suitable interface version increase.
+ */
+#define HVMCTL_iter_shift 8
+#define HVMCTL_iter_mask  ((1U << HVMCTL_iter_shift) - 1)
+#define HVMCTL_iter_max   (1U << (16 + HVMCTL_iter_shift))
+
+long do_hvmctl(XEN_GUEST_HANDLE_PARAM(xen_hvmctl_t) u_hvmctl)
+{
+xen_hvmctl_t op;
+struct domain *d;
+unsigned int iter;
+int rc;
+
+BUILD_BUG_ON(sizeof(op.u) > sizeof(op.u.pad));
+
+if ( copy_from_guest(&op, u_hvmctl, 1) )
+return -EFAULT;
+
+if ( op.interface_version != XEN_HVMCTL_INTERFACE_VERSION )
+return -EACCES;
+
+rc = rcu_lock_remote_domain_by_id(op.domain, &d);
+if ( rc )
+return rc;
+
+if ( !has_hvm_container_domain(d) )
+{
+rcu_unlock_domain(d);
+return -EINVAL;
+}
+
+rc = xsm_hvm_control(XSM_DM_PRIV, d, op.cmd);
+if ( rc )
+{
+rcu_unlock_domain(d);
+return rc;
+}
+
+iter = op.opaque << HVMCTL_iter_shift;
+
+switch ( op.cmd )
+{
+default:
+rc = -EOPNOTSUPP;
+break;
+}
+
+rcu_unlock_domain(d);
+
+if ( rc == -ERESTART )
+{
+ASSERT(!(iter & HVMCTL_iter_mask));
+op.opaque = iter >> HVMCTL_iter_shift;
+if ( unlikely(copy_field_to_guest(u_hvmctl, &op, opaque)) )
+rc = -EFAULT;
+else
+rc = hypercall_create_continuation(__HYPERVISOR_hvmctl, "h",
+   u_hvmctl);
+}
+
+return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4113,6 +4113,7 @@ static const struct {
 COMPAT_CALL(platform_op),
 COMPAT_CALL(mmuext_op),
 HYPERCALL(xenpmu_op),
+HYPERCALL(hvmctl),
 HYPERCALL(arch_1)
 };
 
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -469,6 +469,7 @@ ENTRY(compat_hypercall_table)
 .quad do_tmem_op
 .quad do_ni_hypercall   /* reserved for XenClient */
 .quad do_xenpmu_op  /* 40 */
+.quad do_hvmctl
 .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
 .quad compat_ni_hypercall
 .endr
@@ -520,6 +521,7 @@ ENTRY(compat_hypercall_args_table)
 .byte 1 /* do_tmem_op   */
 .byte 0 /* reserved for XenClient   */
 .byte 2 /* do_xenpmu_op */  /* 40 */
+.byte 1 /* do_hvmctl*/
 .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
 .byte 0 /* compat_ni_hypercall  */
 .endr
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -791,6 +791,7 @@ ENTRY(hypercall_table)
 .quad do_tmem_op
 .quad do_ni_hypercall   /* reserved for XenClient */
 .quad do_xenpmu_op  /* 40 */
+.quad do_hvmctl
 .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
 .quad do_ni_hypercall
 .endr
@@ -842,6 +843,7 @@ ENTRY(hypercall_args_table)
 .byte 1 /* do_tmem_op   */
 .byte 0 /* reserved for XenClient */
 .byte 2 /* do_xenpmu_op */  /* 40 */
+.byte 1 /* do_hvmctl*/
 .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
 .byte 0 /* do_ni_hypercall  */
 .endr
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -93,7 +93,7 @@ all: headers.chk headers++.chk
 
 PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard 
public/*.h public/*/*.h) $(public-y))
 
-PUBLIC_ANSI_HEADERS := $(filter-ou