From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities Date: Mon, 21 Sep 2015 14:51:51 +0100 Message-ID: <56000B77.4080903@citrix.com> References: <1442490029-11583-1-git-send-email-andrew.cooper3@citrix.com> <56001C7302000078000A3FD4@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56001C7302000078000A3FD4@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Xen-devel , Shuai Ruan List-Id: xen-devel@lists.xenproject.org On 21/09/15 14:04, Jan Beulich wrote: >>>> On 17.09.15 at 13:40, wrote: >> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the >> uncertainty with how this information is exposed in libxl. This patch >> introduces no change with how the information is represented in userspace. > Mind explaining this "uncertainty"? I'd like to avoid extending the array > for no real reason... libxl exports "hw_caps" as uint32_t caps[8] in its API. I am uncertain what reusing word 2, or extending the length of the array means WRT to the API/ABI guarantees of libxl. For hw_caps itself, the data is essentially useless as there is no defined layout, Furthermore, some of the leaves are arbitrary/synthetic. One option might be to just drop it from libxl entirely, but this will need to be decided by the toolstack maintainers. For my cpuid levelling series, there is a new set exported which has an ABI guarantee of which words map to which cpuid feature leaves, and a strict rule of no synthetic values. > >> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp) >> >> /* Check extended XSAVE features. */ >> cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx); >> - if ( bsp ) >> - { >> - cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT); >> - cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC); >> - /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */ >> - /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */ >> - } >> - else >> - { >> - BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT)); >> - BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC)); >> - /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */ >> - /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */ >> - } >> + >> + /* Mask out features not currently understood by Xen. */ >> + eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) | >> + cpufeat_mask(X86_FEATURE_XSAVEC)); >> + >> + c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax; >> + >> + if ( !bsp ) >> + BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]); >> } > The !bsp conditional seems pretty pointless. And with the revised > model it looks like it could be relaxed (BUG only when bits the BSP > has set aren't set on the AP). I would be very wary about allowing a situation where certain amounts of heterogeneity would be permitted. Even moreso with the xsaves extensions, any non-homogeneity in the system will result in data corruption. I think it is better to keep this as strictly that the BSP must match all APs. As soon as we encounter a system where this is not the case, far more areas will also need modifying. ~Andrew