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 15:09:51 +0100 Message-ID: <56000FAF.5020808@citrix.com> References: <1442490029-11583-1-git-send-email-andrew.cooper3@citrix.com> <56001C7302000078000A3FD4@prv-mh.provo.novell.com> <56000B77.4080903@citrix.com> <5600299302000078000A4090@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: <5600299302000078000A4090@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 15:00, Jan Beulich wrote: >>>> On 21.09.15 at 15:51, wrote: >> 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. > Even more so a reason to re-use word 2. This works for 0xd:1.eax, but the array has to be extended for 0x7:0.ebx and 0x7:0.ecx, both of which are also included in my levelling series. Currently I have just left the libxl question alone. > >>>> @@ -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. > I guess you misunderstood - I didn't mean for both lines to be > dropped; I meant the if() surrounding the BUG_ON() to go away. I don't mind dropping the if(), but I was querying your suggestion in brackets. ~Andrew