From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/4] x86/hvm: allow guest to use clflushopt and clwb Date: Wed, 30 Dec 2015 10:33:47 +0000 Message-ID: <5683B30B.5000703@citrix.com> References: <1451388711-18646-1-git-send-email-haozhong.zhang@intel.com> <1451388711-18646-2-git-send-email-haozhong.zhang@intel.com> <5682AAE6.4070908@citrix.com> <20151230013553.GB16809@hz-desktop.sh.intel.com> <20151230021648.GC16809@hz-desktop.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151230021648.GC16809@hz-desktop.sh.intel.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: xen-devel@lists.xen.org, Keir Fraser , Jan Beulich , Ian Jackson , Stefano Stabellini , Ian Campbell , Wei Liu , Jun Nakajima , Kevin Tian List-Id: xen-devel@lists.xenproject.org On 30/12/2015 02:16, Haozhong Zhang wrote: > On 12/30/15 09:35, Haozhong Zhang wrote: >> On 12/29/15 15:46, Andrew Cooper wrote: >>> On 29/12/2015 11:31, Haozhong Zhang wrote: >>>> Pass CPU features CLFLUSHOPT and CLWB into HVM domain so that those two >>>> instructions can be used by guest. >>>> >>>> The specification of above two instructions can be found in >>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf >>>> >>>> Signed-off-by: Haozhong Zhang >>> Please be aware that my cpuid rework series completely changes all of this >>> code. As this patch is small and self contained, it would be best to get >>> it accepted early and for me to rebase over the result. >>> >> I'll split this patch series into two parts and put these two >> instruction enabling patches in the first part. >> >>> As part of my cpuid work, I had come to the conclusion that CLFLUSHOPT, CLWB >>> and PCOMMIT were all safe for all guests to use, as they deemed safe for >>> cpl3 code to use. Is there any reason why these wouldn't be safe for PV >>> guests to use? >>> >> Not for safety concern. These three instructions are usually used with >> NVDIMM which are only implemented for HVM domains in this patch >> series, so I didn't enable them for PV. I think they can be enabled >> for PV later by another patch. >> >>>> --- >>>> tools/libxc/xc_cpufeature.h | 3 ++- >>>> tools/libxc/xc_cpuid_x86.c | 4 +++- >>>> xen/arch/x86/hvm/hvm.c | 7 +++++++ >>>> xen/include/asm-x86/cpufeature.h | 5 +++++ >>>> 4 files changed, 17 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/tools/libxc/xc_cpufeature.h b/tools/libxc/xc_cpufeature.h >>>> index c3ddc80..5288ac6 100644 >>>> --- a/tools/libxc/xc_cpufeature.h >>>> +++ b/tools/libxc/xc_cpufeature.h >>>> @@ -140,6 +140,7 @@ >>>> #define X86_FEATURE_RDSEED 18 /* RDSEED instruction */ >>>> #define X86_FEATURE_ADX 19 /* ADCX, ADOX instructions */ >>>> #define X86_FEATURE_SMAP 20 /* Supervisor Mode Access Protection */ >>>> - >>>> +#define X86_FEATURE_CLFLUSHOPT 23 /* CLFLUSHOPT instruction */ >>>> +#define X86_FEATURE_CLWB 24 /* CLWB instruction */ >>>> #endif /* __LIBXC_CPUFEATURE_H */ >>>> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c >>>> index 8882c01..fecfd6c 100644 >>>> --- a/tools/libxc/xc_cpuid_x86.c >>>> +++ b/tools/libxc/xc_cpuid_x86.c >>>> @@ -426,7 +426,9 @@ static void xc_cpuid_hvm_policy(xc_interface *xch, >>>> bitmaskof(X86_FEATURE_RDSEED) | >>>> bitmaskof(X86_FEATURE_ADX) | >>>> bitmaskof(X86_FEATURE_SMAP) | >>>> - bitmaskof(X86_FEATURE_FSGSBASE)); >>>> + bitmaskof(X86_FEATURE_FSGSBASE) | >>>> + bitmaskof(X86_FEATURE_CLWB) | >>>> + bitmaskof(X86_FEATURE_CLFLUSHOPT)); >>>> } else >>>> regs[1] = 0; >>>> regs[0] = regs[2] = regs[3] = 0; >>> The entry for CLFLUSHOPT in the ISA Extension manual (August 2015) talks >>> about CPUID.7(ECX=1).EBX[8:15] indicating the cache line size affected by >>> the instruction. However, I can't find any other reference to this >>> information, nor an extension of the CPUID instruction in the ISA manual. >>> Should the Xen cpuid handling code be updated not to clobber this? >>> >> Yes, I missed this part and will update in the next version. >> > I double-checked the manual and it says that > > "The aligned cache line size affected is also indicated with the > CPUID instruction (bits 8 through 15 of the EBX register when the > initial value in the EAX register is 1)" > > so I guess you really meant CPUID.1.EBX[8:15]. The 0x00000001 case > branch in xc_cpuid_hvm_policy() (and its callers) has already passed > the host CPUID.1.EBX[8:15] to HVM domains, so no more action is needed > in this patch. Oops sorry. Yes - I misread the paragraph in the manual. Apologies for the noise. ~Andrew