From: "Jan Beulich" <JBeulich@suse.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
mpohlack@amazon.de, ross.lagerwall@citrix.com,
Julien Grall <julien.grall@arm.com>,
Stefano Stabellini <stefano.stabellini@citrix.com>,
xen-devel@lists.xenproject.org,
Daniel De Graaf <dgdegra@tycho.nsa.gov>,
Keir Fraser <keir@xen.org>,
sasha.levin@oracle.com
Subject: Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Date: Fri, 18 Mar 2016 06:36:35 -0600 [thread overview]
Message-ID: <56EC046302000078000DE54F@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20160317013852.GA22460@char.us.oracle.com>
> Subject: [PATCH v4 05/35] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane.
>
> This hypercall mirrors the XENVER_ in that it has similar functionality.
> However it is designed differently:
> - No compat layer. The data structures are the same size on 32
> as on 64-bit.
> - The hypercall accepts three arguments - the command, pointer to
> an buffer, and the length of the buffer.
> - Each sub-ops can be "probed" for size by returning the size of
> buffer that will be needed - if the buffer is NULL.
> - Subops can complete even if the buffer is too slow - truncated
> data will be filled and hypercall will return -ENOBUFS.
s/too slow/too small/ ?
> - VERSION_OP_commandline, VERSION_OP_changeset are privileged.
Aiui this is no difference to the old one anymore if we assume
patches get committed in the order they're being presented in
this series.
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -221,6 +221,47 @@ void __init do_initcalls(void)
>
> #endif
>
> +static int get_features(struct domain *d, xen_feature_info_t *fi)
> +{
> + switch ( fi->submap_idx )
> + {
> + case 0:
> + fi->submap = (1U << XENFEAT_memory_op_vnode_supported);
> + if ( VM_ASSIST(d, pae_extended_cr3) )
> + fi->submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
Since you already move this code, I think the two lines above
would better go into the x86-specific section below.
> @@ -302,50 +343,16 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> case XENVER_get_features:
> {
> xen_feature_info_t fi;
> - struct domain *d = current->domain;
>
> if ( copy_from_guest(&fi, arg, 1) )
> return -EFAULT;
>
> - switch ( fi.submap_idx )
> + if ( !deny )
> {
> - case 0:
> - if ( deny )
> - break;
> - fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> - if ( VM_ASSIST(d, pae_extended_cr3) )
> - fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
> - if ( paging_mode_translate(d) )
> - fi.submap |=
> - (1U << XENFEAT_writable_page_tables) |
> - (1U << XENFEAT_auto_translated_physmap);
> - if ( is_hardware_domain(d) )
> - fi.submap |= 1U << XENFEAT_dom0;
> -#ifdef CONFIG_X86
> - switch ( d->guest_type )
> - {
> - case guest_type_pv:
> - fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
> - (1U << XENFEAT_highmem_assist) |
> - (1U << XENFEAT_gnttab_map_avail_bits);
> - break;
> - case guest_type_pvh:
> - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> - (1U << XENFEAT_supervisor_mode_kernel) |
> - (1U << XENFEAT_hvm_callback_vector);
> - break;
> - case guest_type_hvm:
> - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> - (1U << XENFEAT_hvm_callback_vector) |
> - (1U << XENFEAT_hvm_pirqs);
> - break;
> - }
> -#endif
> - break;
> - default:
> - return -EINVAL;
> + int rc = get_features(current->domain, &fi);
> + if ( rc )
Blank line between declaration(s) and statement(s) please.
> @@ -388,6 +395,182 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> return -ENOSYS;
> }
>
> +static const char *capabilities_info(ssize_t *len)
Why ssize_t?
> +{
> + static xen_capabilities_info_t cached_cap;
__read_mostly?
> +static int size_of_subops_data(unsigned int cmd, ssize_t *sz)
> +{
> + int rc = 0;
> + /* Compute size. */
> + switch ( cmd )
> + {
> + case XEN_VERSION_OP_version:
> + *sz = sizeof(xen_version_op_val_t);
> + break;
> +
> + case XEN_VERSION_OP_extraversion:
> + *sz = strlen(xen_extra_version()) + 1;
> + break;
> +
> + case XEN_VERSION_OP_capabilities:
> + capabilities_info(sz);
> + break;
> +
> + case XEN_VERSION_OP_platform_parameters:
> + *sz = sizeof(xen_version_op_val_t);
> + break;
> +
> + case XEN_VERSION_OP_changeset:
> + *sz = strlen(xen_changeset()) + 1;
> + break;
> +
> + case XEN_VERSION_OP_get_features:
> + *sz = sizeof(xen_feature_info_t);
> + break;
> +
> + case XEN_VERSION_OP_pagesize:
> + *sz = sizeof(xen_version_op_val_t);
> + break;
Please combine all the cases producing this value.
> + case XEN_VERSION_OP_guest_handle:
> + *sz = ARRAY_SIZE(current->domain->handle);
> + break;
> +
> + case XEN_VERSION_OP_commandline:
> + *sz = ARRAY_SIZE(saved_cmdline);
strlen()?
> +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> + unsigned int len)
> +{
> + union {
> + xen_version_op_val_t n;
Would "v" or "val" be the more natural name here?
> + xen_feature_info_t fi;
> + } u = {};
> + ssize_t sz = 0;
> + const void *ptr = NULL;
> + int rc = xsm_version_op(XSM_OTHER, cmd);
> +
> + /* We can safely return -EPERM! */
> + if ( rc )
> + return rc;
> +
> + rc = size_of_subops_data(cmd, &sz);
> + if ( rc )
> + return rc;
> +
> + ASSERT(sz);
> + /*
> + * This hypercall also allows the client to probe. If it provides
> + * a NULL arg we will return the size of the space it has to
> + * allocate for the specific sub-op.
> + */
> + if ( guest_handle_is_null(arg) )
> + return sz;
> +
> + /*
> + * The HYPERVISOR_xen_version differs in that some return the value,
> + * and some copy it on back on argument. We follow the same rule for all
> + * sub-ops: return 0 on success, positive value of bytes returned, and
> + * always copy the result in arg. Yeey sanity!
> + */
> +
> + switch ( cmd )
> + {
> + case XEN_VERSION_OP_version:
> + u.n = (xen_major_version() << 16) | xen_minor_version();
> + break;
> +
> + case XEN_VERSION_OP_extraversion:
> + ptr = xen_extra_version();
> + break;
> +
> + case XEN_VERSION_OP_capabilities:
> + ptr = capabilities_info(&sz);
> + break;
> +
> + case XEN_VERSION_OP_platform_parameters:
> + u.n = HYPERVISOR_VIRT_START;
> + break;
> +
> + case XEN_VERSION_OP_changeset:
> + ptr = xen_changeset();
> + break;
> +
> + case XEN_VERSION_OP_get_features:
> + if ( copy_from_guest(&u.fi, arg, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + rc = get_features(current->domain, &u.fi);
> + break;
> +
> + case XEN_VERSION_OP_pagesize:
> + u.n = PAGE_SIZE;
> + break;
> +
> + case XEN_VERSION_OP_guest_handle:
> + ptr = current->domain->handle;
> + break;
> +
> + case XEN_VERSION_OP_commandline:
> + ptr = saved_cmdline;
> + break;
> +
> + default:
> + rc = -ENOSYS;
> + }
Seeing this long switch() a second time I wonder why
size_of_subops_data() doesn't just get folded here, with the null
handle being taken care of below instead of above.
> + if ( !rc )
> + {
> + ssize_t bytes;
> +
> + if ( sz > len )
> + bytes = len;
> + else
> + bytes = sz;
min() (for which sz and bytes being "unsigned int" would help)?
> + if ( copy_to_guest(arg, ptr ? : &u, bytes) )
> + rc = -EFAULT;
> + }
> + if ( !rc )
> + {
> + /*
> + * We return len (truncate) worth of data even if we fail.
> + */
> + if ( sz > len )
> + rc = -ENOBUFS;
Perhaps worth moving this up into the previous if(), such that
-EFAULT would also take precedence over -ENOBUFS?
> @@ -87,6 +95,68 @@ typedef struct xen_feature_info xen_feature_info_t;
> #define XENVER_commandline 9
> typedef char xen_commandline_t[1024];
>
> +
> +
> +/*
> + * The HYPERCALL_version_op has a set of sub-ops which mirror the
> + * sub-ops of HYPERCALL_xen_version. However this hypercall differs
> + * radically from the former:
> + * - It returns the amount of bytes returned.
> + * - It will return -XEN_EPERM if the guest is not permitted.
> + * - It will return the requested data in arg.
> + * - It requires an third argument (len) for the length of the
> + * arg. Naturally the arg has to fit the requested data otherwise
> + * -XEN_ENOBUFS is returned.
> + *
> + * It also offers an mechanism to probe for the amount of bytes an
> + * sub-op will require. Having the arg have an NULL pointer will
s/pointer/handle/
> + * return the number of bytes requested for the operation. Or an
> + * negative value if an error is encountered.
> + */
> +
> +typedef uint64_t xen_version_op_val_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t);
> +
> +typedef void xen_version_op_buf_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);
Are these actually useful for anything? And for the various strings,
wouldn't a "char" handle be more natural?
> +/* arg == version_op_val_t. Encoded as major:minor (31..16:15..0) */
Please make explicit that 63...32 are zero (or whatever they really
are).
> +#define XEN_VERSION_OP_version 0
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_extraversion 1
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_capabilities 3
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_changeset 4
> +
> +/*
> + * arg == xen_version_op_val_t. Contains the virtual address
> + * of the hypervisor encoded as [63..0].
I'd say the encoding info here is unnecessary and could - just like
you already do for pagesize below - be omitted.
> + */
> +#define XEN_VERSION_OP_platform_parameters 5
> +
> +/*
> + * arg = xen_feature_info_t - shares the same structure
> + * as the XENVER_get_features.
> + */
> +#define XEN_VERSION_OP_get_features 6
> +
> +/* arg == xen_version_op_val_t. */
> +#define XEN_VERSION_OP_pagesize 7
> +
> +/* arg == version_op_buf.
> + *
> + * The toolstack fills it out for guest consumption. It is intended to hold
> + * the UUID of the guest.
> + */
> +#define XEN_VERSION_OP_guest_handle 8
> +
> +/* arg = version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_commandline 9
> +
> #endif /* __XEN_PUBLIC_VERSION_H__ */
Would leaving out the _OP and _op everywhere result in any
name collisions with the old one?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-18 12:36 UTC|newest]
Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-15 17:56 [PATCH v4] xSplice v1 design and implementation Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 01/34] compat/x86: Remove unncessary #define Konrad Rzeszutek Wilk
2016-03-15 18:57 ` Andrew Cooper
2016-03-16 11:08 ` Jan Beulich
2016-03-17 0:44 ` Konrad Rzeszutek Wilk
2016-03-17 7:45 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 02/34] libxc: Remove dead code (XENVER_capabilities) Konrad Rzeszutek Wilk
2016-03-15 18:04 ` Andrew Cooper
2016-03-15 18:08 ` Konrad Rzeszutek Wilk
2016-03-16 18:11 ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 03/34] xsm/xen_version: Add XSM for the xen_version hypercall Konrad Rzeszutek Wilk
2016-03-18 11:55 ` Jan Beulich
2016-03-18 17:26 ` Konrad Rzeszutek Wilk
2016-03-21 11:22 ` Jan Beulich
2016-03-22 16:10 ` Konrad Rzeszutek Wilk
2016-03-22 17:54 ` Daniel De Graaf
2016-03-22 17:49 ` Daniel De Graaf
2016-03-24 15:34 ` anshul makkar
2016-03-24 19:19 ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane Konrad Rzeszutek Wilk
2016-03-15 18:29 ` Andrew Cooper
2016-03-15 20:19 ` Konrad Rzeszutek Wilk
2016-03-17 1:38 ` Konrad Rzeszutek Wilk
2016-03-17 14:28 ` Andrew Cooper
2016-03-18 12:36 ` Jan Beulich [this message]
2016-03-18 19:22 ` Konrad Rzeszutek Wilk
2016-03-21 12:45 ` Jan Beulich
2016-03-22 15:52 ` Konrad Rzeszutek Wilk
2016-03-22 16:06 ` Jan Beulich
2016-03-22 18:57 ` Konrad Rzeszutek Wilk
2016-03-22 19:28 ` Andrew Cooper
2016-03-22 20:39 ` Konrad Rzeszutek Wilk
2016-03-23 8:56 ` Jan Beulich
2016-03-24 2:37 ` Konrad Rzeszutek Wilk
2016-03-24 9:15 ` Jan Beulich
2016-03-24 11:39 ` Konrad Rzeszutek Wilk
2016-03-22 17:51 ` Daniel De Graaf
2016-03-15 17:56 ` [PATCH v4 05/34] libxc/libxl/python/xenstat: Use new XEN_VERSION_OP hypercall Konrad Rzeszutek Wilk
2016-03-15 18:45 ` Andrew Cooper
2016-03-16 12:31 ` George Dunlap
2016-03-16 18:11 ` Wei Liu
2016-03-17 1:08 ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 06/34] x86/arm: Add BUGFRAME_NR define and BUILD checks Konrad Rzeszutek Wilk
2016-03-15 18:54 ` Andrew Cooper
2016-03-16 11:49 ` Julien Grall
2016-03-18 12:40 ` Jan Beulich
2016-03-18 19:59 ` Konrad Rzeszutek Wilk
2016-03-21 12:49 ` Jan Beulich
2016-03-22 15:39 ` Konrad Rzeszutek Wilk
2016-03-22 15:58 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 07/34] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables Konrad Rzeszutek Wilk
2016-03-15 19:24 ` Andrew Cooper
2016-03-15 19:34 ` Konrad Rzeszutek Wilk
2016-03-15 19:51 ` Andrew Cooper
2016-03-15 20:02 ` Andrew Cooper
2016-03-16 10:33 ` Jan Beulich
2016-03-18 13:07 ` Jan Beulich
2016-03-22 20:18 ` Konrad Rzeszutek Wilk
2016-03-23 8:19 ` Jan Beulich
2016-03-23 11:17 ` Julien Grall
2016-03-23 11:21 ` Jan Beulich
2016-03-24 2:49 ` Konrad Rzeszutek Wilk
2016-03-24 9:20 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 08/34] vmap: Make the while loop less fishy Konrad Rzeszutek Wilk
2016-03-15 19:33 ` Andrew Cooper
2016-03-17 11:49 ` Jan Beulich
2016-03-17 14:37 ` Andrew Cooper
2016-03-17 15:30 ` Jan Beulich
2016-03-17 16:06 ` Ian Jackson
2016-03-17 11:48 ` Jan Beulich
2016-03-17 16:08 ` Ian Jackson
2016-03-21 12:04 ` George Dunlap
2016-03-21 13:26 ` Jan Beulich
2016-03-21 14:22 ` George Dunlap
2016-03-21 15:05 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 09/34] vmap: ASSERT on NULL Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 10/34] vmap: Add vmalloc_cb and vfree_cb Konrad Rzeszutek Wilk
2016-03-18 13:20 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 11/34] xsplice: Design document Konrad Rzeszutek Wilk
2016-03-23 11:18 ` Jan Beulich
2016-03-23 20:12 ` Konrad Rzeszutek Wilk
2016-03-23 20:21 ` Konrad Rzeszutek Wilk
2016-03-24 3:15 ` Konrad Rzeszutek Wilk
2016-03-24 9:32 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 12/34] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op Konrad Rzeszutek Wilk
2016-03-16 12:12 ` Julien Grall
2016-03-16 19:58 ` Konrad Rzeszutek Wilk
2016-03-23 13:51 ` Jan Beulich
2016-03-24 3:13 ` Konrad Rzeszutek Wilk
2016-03-24 9:29 ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 13/34] libxc: Implementation of XEN_XSPLICE_op in libxc Konrad Rzeszutek Wilk
2016-03-16 18:12 ` Wei Liu
2016-03-16 20:36 ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 14/34] xen-xsplice: Tool to manipulate xsplice payloads Konrad Rzeszutek Wilk
2016-03-16 18:12 ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 15/34] xsplice: Add helper elf routines Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 16/34] xsplice: Implement payload loading Konrad Rzeszutek Wilk
2016-03-22 17:25 ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 17/34] xsplice: Implement support for applying/reverting/replacing patches Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 18/34] x86/xen_hello_world.xsplice: Test payload for patching 'xen_extra_version' Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 19/34] xsplice, symbols: Implement symbol name resolution on address Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 20/34] x86, xsplice: Print payload's symbol name and payload name in backtraces Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 21/34] xsplice: Add .xsplice.hooks functions and test-case Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 22/34] xsplice: Add support for bug frames Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 23/34] xsplice: Add support for exception tables Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 24/34] xsplice: Add support for alternatives Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 25/34] build_id: Provide ld-embedded build-ids Konrad Rzeszutek Wilk
2016-03-16 18:34 ` Julien Grall
2016-03-16 21:02 ` Konrad Rzeszutek Wilk
2016-03-17 1:12 ` Konrad Rzeszutek Wilk
2016-03-17 11:08 ` Julien Grall
2016-03-17 13:39 ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 26/34] HYPERCALL_version_op: Add VERSION_OP_build_id to retrieve build-id Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 27/34] libxl: info: Display build_id of the hypervisor using XEN_VERSION_OP_build_id Konrad Rzeszutek Wilk
2016-03-16 18:12 ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 28/34] xsplice: Print build_id in keyhandler and on bootup Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 29/34] xsplice: Stacking build-id dependency checking Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 30/34] xsplice/xen_replace_world: Test-case for XSPLICE_ACTION_REPLACE Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 31/34] xsplice: Print dependency and payloads build_id in the keyhandler Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 32/34] xsplice: Prevent duplicate payloads from being loaded Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 33/34] xsplice: Add support for shadow variables Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 34/34] MAINTAINERS/xsplice: Add myself and Ross as the maintainers Konrad Rzeszutek Wilk
2016-03-16 11:10 ` Jan Beulich
2016-03-17 0:44 ` Konrad Rzeszutek Wilk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56EC046302000078000DE54F@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=dgdegra@tycho.nsa.gov \
--cc=ian.jackson@eu.citrix.com \
--cc=julien.grall@arm.com \
--cc=keir@xen.org \
--cc=konrad.wilk@oracle.com \
--cc=mpohlack@amazon.de \
--cc=ross.lagerwall@citrix.com \
--cc=sasha.levin@oracle.com \
--cc=stefano.stabellini@citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).