From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
xen-devel@lists.xenproject.org, ross.lagerwall@citrix.com,
konrad@kernel.org, mpohlack@amazon.de, sasha.levin@oracle.com
Cc: Wei Liu <wei.liu2@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
Julien Grall <julien.grall@arm.com>,
Stefano Stabellini <stefano.stabellini@citrix.com>,
Jan Beulich <jbeulich@suse.com>, Keir Fraser <keir@xen.org>,
Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Date: Tue, 15 Mar 2016 18:29:12 +0000 [thread overview]
Message-ID: <56E85478.5070408@citrix.com> (raw)
In-Reply-To: <1458064616-23101-5-git-send-email-konrad.wilk@oracle.com>
On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote:
> @@ -388,6 +395,188 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> return -ENOSYS;
> }
>
> +static const char *capabilities_info(ssize_t *len)
> +{
> + static xen_capabilities_info_t cached_cap;
> + static unsigned int cached_cap_len;
> + static bool_t cached;
> +
> + if ( cached )
I am surprised that Coverity didn't complain about this being unused...
> + {
> + *len = cached_cap_len;
> + return cached_cap;
> + }
> + arch_get_xen_caps(&cached_cap);
> + cached_cap_len = strlen(cached_cap) + 1;
> +
> + *len = cached_cap_len;
> + return cached_cap;
You can turn the logic around as
if ( unliklely(!cached) )
{
arch_get_xen_caps(&cached_cap);
cached_cap_len = strlen(cached_cap) + 1;
cached = 1;
}
and have a single return path.
> +}
> +
> +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;
> +
> + case XEN_VERSION_OP_guest_handle:
> + *sz = ARRAY_SIZE(current->domain->handle);
> + break;
> +
> + case XEN_VERSION_OP_commandline:
> + *sz = ARRAY_SIZE(saved_cmdline);
> + break;
> +
> + default:
> + rc = -ENOSYS;
> + }
> +
> + return rc;
> +}
> +
> +/*
> + * Similar to HYPERVISOR_xen_version but with a sane interface
> + * (has a length, one can probe for the length) and with one less sub-ops:
> + * missing XENVER_compile_info.
> + */
> +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> + unsigned int len)
> +{
> + union {
> + xen_version_op_val_t n;
> + xen_feature_info_t fi;
> + } u;
= {}; and you can forgo the explicit memset() below.
> + 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;
> +
> + /* Some of the subops may have no data. */
> + if ( !sz )
> + return 0;
Really? I would have thought it would be reasonable to assert that
either sz != 0 after the rc != 0 return.
> + /*
> + * 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;
> +
> + memset(&u, 0, sizeof(u));
> + /*
> + * 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!
> + */
> +
> + rc = 0;
rc is guaranteed to be 0 at this point.
> + 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;
> + }
> +
> + if ( !rc )
> + {
> + ssize_t bytes;
> +
> + if ( sz > len )
> + bytes = len;
> + else
> + bytes = sz;
> +
> + if ( copy_to_guest(arg, ptr ? ptr : &u, bytes) )
Can be shortened to ptr ?: &u
> + rc = -EFAULT;
> + }
> + if ( !rc )
> + {
> + /*
> + * We return len (truncate) worth of data even if we fail.
> + */
> + if ( sz > len )
> + rc = -ENOBUFS;
This needs to be in the previous if() clause to avoid overriding -EFAULT
with -ENOBUFS.
> +
> +/*
> + * 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
> + * 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 unsigned char xen_version_op_buf_t[];
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);
Strictly speaking this should be a void* guest handle, as not all data
is returned via this mechanism is unsigned char.
> +
> +/* arg == version_op_val_t. Encoded as major:minor (31..16:15..0) */
> +#define XEN_VERSION_OP_version 0
> +
> +/* arg == version_op_buf. */
> +#define XEN_VERSION_OP_extraversion 1
> +
> +/* arg == version_op_buf */
> +#define XEN_VERSION_OP_capabilities 3
> +
> +/* arg == version_op_buf */
> +#define XEN_VERSION_OP_changeset 4
Might be worth stating that these return NUL terminated utf-8 strings?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-15 18:29 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 [this message]
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
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=56E85478.5070408@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=dgdegra@tycho.nsa.gov \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien.grall@arm.com \
--cc=keir@xen.org \
--cc=konrad.wilk@oracle.com \
--cc=konrad@kernel.org \
--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).