From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH v5 21/21] tools/libxc: Calculate xstate cpuid leaf from guest information
Date: Fri, 08 Apr 2016 15:00:52 -0600 [thread overview]
Message-ID: <57082A1402000078000E61F8@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1460030246-30153-22-git-send-email-andrew.cooper3@citrix.com>
>>> On 07.04.16 at 13:57, <andrew.cooper3@citrix.com> wrote:
> The existing logic is broken for heterogeneous migration. By always
> advertising the host maximum xstate, a migration to a less capable host always
> fails as Xen cannot accomodate the xcr0_accum in the migration stream.
I don't understand this - xcr0_accum is definitely part of the
migration stream.
> v5:
> * Reintroduce PKRU, (again, lost due to rebasing).
> * Rewrite the commit message and comments to try and better explain why I am
> deliberatly removing host-specific information from the xstate calculation.
> * Reintroduce 0xFFFFFFFF masks for EAX, to avoid Coverity complaining about
> truncation on assignment.
Urgh - I don't think ugliness like this can be justified by Coverity
complaining. That would be different if the compiler complained
(like compilers other than gcc do).
> static void xc_cpuid_config_xsave(xc_interface *xch,
> const struct cpuid_domain_info *info,
> const unsigned int *input, unsigned int *regs)
> {
> - if ( info->xfeature_mask == 0 )
> + uint64_t guest_xfeature_mask;
> +
> + if ( info->xfeature_mask == 0 ||
> + !test_bit(X86_FEATURE_XSAVE, info->featureset) )
> {
> regs[0] = regs[1] = regs[2] = regs[3] = 0;
> return;
> }
>
> + guest_xfeature_mask = X86_XCR0_SSE | X86_XCR0_X87;
> +
> + if ( test_bit(X86_FEATURE_AVX, info->featureset) )
> + guest_xfeature_mask |= X86_XCR0_AVX;
> +
> + if ( test_bit(X86_FEATURE_MPX, info->featureset) )
> + guest_xfeature_mask |= X86_XCR0_BNDREG | X86_XCR0_BNDCSR;
> +
> + if ( test_bit(X86_FEATURE_PKU, info->featureset) )
> + guest_xfeature_mask |= X86_XCR0_PKRU;
> +
> + if ( test_bit(X86_FEATURE_LWP, info->featureset) )
> + guest_xfeature_mask |= X86_XCR0_LWP;
I continue to be unhappy about that white listing, but well...
> case 1: /* leaf 1 */
> regs[0] = info->featureset[featureword_of(X86_FEATURE_XSAVEOPT)];
> - regs[2] &= info->xfeature_mask;
> - regs[3] = 0;
> + regs[1] = 0;
> +
> + if ( test_bit(X86_FEATURE_XSAVES, info->featureset) )
> + {
> + regs[2] = guest_xfeature_mask & X86_XSS_MASK & 0xFFFFFFFF;
> + regs[3] = (guest_xfeature_mask >> 32) & X86_XSS_MASK;
This is correct only because X86_XSS_MASK == 0(or else >> and &
need to be swapped). Yet if you assume this, the and-ing with
0xFFFFFFFF makes even less sense here than above.
> + case 2 ... 62: /* per-component sub-leaves */
> + if ( !(guest_xfeature_mask & (1ULL << input[1])) )
> {
> regs[0] = regs[1] = regs[2] = regs[3] = 0;
> break;
> }
> /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
> - regs[2] = regs[3] = 0;
> + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
I'm sorry for realizing this only now, but shouldn't we hide XSTATE_XSS
from PV guests? Or is this being taken care of elsewhere?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-04-08 21:00 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-07 11:57 [PATCH v5 00/21] x86: Improvements to cpuid handling for guests Andrew Cooper
2016-04-07 11:57 ` [PATCH v5 01/21] xen/x86: Annotate VM applicability in featureset Andrew Cooper
2016-04-07 23:01 ` Jan Beulich
2016-04-07 11:57 ` [PATCH v5 02/21] xen/x86: Calculate maximum host and guest featuresets Andrew Cooper
2016-04-07 23:04 ` Jan Beulich
2016-04-07 11:57 ` [PATCH v5 03/21] xen/x86: Generate deep dependencies of features Andrew Cooper
2016-04-07 23:18 ` Jan Beulich
2016-04-07 23:36 ` Andrew Cooper
2016-04-08 15:17 ` Jan Beulich
2016-04-08 15:18 ` Andrew Cooper
2016-04-07 11:57 ` [PATCH v5 04/21] xen/x86: Clear dependent features when clearing a cpu cap Andrew Cooper
2016-04-08 15:36 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 05/21] xen/x86: Improve disabling of features which have dependencies Andrew Cooper
2016-04-08 15:04 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 06/21] xen/x86: Improvements to in-hypervisor cpuid sanity checks Andrew Cooper
2016-04-08 16:10 ` Konrad Rzeszutek Wilk
2016-04-08 18:06 ` Jan Beulich
2016-04-07 11:57 ` [PATCH v5 07/21] x86/cpu: Move set_cpumask() calls into c_early_init() Andrew Cooper
2016-04-08 18:09 ` Jan Beulich
2016-04-07 11:57 ` [PATCH v5 08/21] x86/cpu: Sysctl and common infrastructure for levelling context switching Andrew Cooper
2016-04-07 16:54 ` Daniel De Graaf
2016-04-08 16:12 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 09/21] x86/cpu: Rework AMD masking MSR setup Andrew Cooper
2016-04-08 16:13 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 10/21] x86/cpu: Rework Intel masking/faulting setup Andrew Cooper
2016-04-08 16:14 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 11/21] x86/cpu: Context switch cpuid masks and faulting state in context_switch() Andrew Cooper
2016-04-08 16:15 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 12/21] x86/pv: Provide custom cpumasks for PV domains Andrew Cooper
2016-04-08 16:17 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 13/21] x86/domctl: Update PV domain cpumasks when setting cpuid policy Andrew Cooper
2016-04-08 16:26 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 14/21] xen+tools: Export maximum host and guest cpu featuresets via SYSCTL Andrew Cooper
2016-04-07 16:54 ` Daniel De Graaf
2016-04-08 16:32 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 15/21] tools/libxc: Modify bitmap operations to take void pointers Andrew Cooper
2016-04-07 13:00 ` Wei Liu
2016-04-08 16:34 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 16/21] tools/libxc: Use public/featureset.h for cpuid policy generation Andrew Cooper
2016-04-08 16:37 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 17/21] tools/libxc: Expose the automatically generated cpu featuremask information Andrew Cooper
2016-04-08 16:38 ` Konrad Rzeszutek Wilk
2016-04-07 11:57 ` [PATCH v5 18/21] tools: Utility for dealing with featuresets Andrew Cooper
2016-04-07 11:57 ` [PATCH v5 19/21] tools/libxc: Wire a featureset through to cpuid policy logic Andrew Cooper
2016-04-07 11:57 ` [PATCH v5 20/21] tools/libxc: Use featuresets rather than guesswork Andrew Cooper
2016-04-07 11:57 ` [PATCH v5 21/21] tools/libxc: Calculate xstate cpuid leaf from guest information Andrew Cooper
2016-04-07 12:58 ` Wei Liu
2016-04-08 21:00 ` Jan Beulich [this message]
2016-04-08 21:45 ` Andrew Cooper
2016-04-08 22:38 ` Jan Beulich
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=57082A1402000078000E61F8@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.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).