| Message ID | 1451388711-18646-2-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <haozhong.zhang@intel.com> 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. 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? > --- > 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? > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 21470ec..58c83a5 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4598,6 +4598,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > /* Don't expose INVPCID to non-hap hvm. */ > if ( (count == 0) && !hap_enabled(d) ) > *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID); > + > + if ( (count == 0) && !cpu_has_clflushopt ) > + *ebx &= ~cpufeat_mask(X86_FEATURE_CLFLUSHOPT); > + > + if ( (count == 0) && !cpu_has_clwb ) > + *ebx &= ~cpufeat_mask(X86_FEATURE_CLWB); Please refactor this code along with if() in context above, to only check count once. ~Andrew
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 <haozhong.zhang@intel.com> > > 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. > >diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > >index 21470ec..58c83a5 100644 > >--- a/xen/arch/x86/hvm/hvm.c > >+++ b/xen/arch/x86/hvm/hvm.c > >@@ -4598,6 +4598,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > > /* Don't expose INVPCID to non-hap hvm. */ > > if ( (count == 0) && !hap_enabled(d) ) > > *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID); > >+ > >+ if ( (count == 0) && !cpu_has_clflushopt ) > >+ *ebx &= ~cpufeat_mask(X86_FEATURE_CLFLUSHOPT); > >+ > >+ if ( (count == 0) && !cpu_has_clwb ) > >+ *ebx &= ~cpufeat_mask(X86_FEATURE_CLWB); > > Please refactor this code along with if() in context above, to only check > count once. > Yes, I'll update in the next version. Thanks, Haozhong
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 <haozhong.zhang@intel.com> > > > > 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. Haozhong
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 <haozhong.zhang@intel.com> >>> 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
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; diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 21470ec..58c83a5 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4598,6 +4598,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, /* Don't expose INVPCID to non-hap hvm. */ if ( (count == 0) && !hap_enabled(d) ) *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID); + + if ( (count == 0) && !cpu_has_clflushopt ) + *ebx &= ~cpufeat_mask(X86_FEATURE_CLFLUSHOPT); + + if ( (count == 0) && !cpu_has_clwb ) + *ebx &= ~cpufeat_mask(X86_FEATURE_CLWB); + break; case 0xb: /* Fix the x2APIC identifier. */ diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index ef96514..5818228 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -162,6 +162,8 @@ #define X86_FEATURE_RDSEED (7*32+18) /* RDSEED instruction */ #define X86_FEATURE_ADX (7*32+19) /* ADCX, ADOX instructions */ #define X86_FEATURE_SMAP (7*32+20) /* Supervisor Mode Access Prevention */ +#define X86_FEATURE_CLFLUSHOPT (7*32+23) /* CLFLUSHOPT instruction */ +#define X86_FEATURE_CLWB (7*32+24) /* CLWB instruction */ /* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 8 */ #define X86_FEATURE_PKU (8*32+ 3) /* Protection Keys for Userspace */ @@ -234,6 +236,9 @@ #define cpu_has_xgetbv1 boot_cpu_has(X86_FEATURE_XGETBV1) #define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES) +#define cpu_has_clflushopt boot_cpu_has(X86_FEATURE_CLFLUSHOPT) +#define cpu_has_clwb boot_cpu_has(X86_FEATURE_CLWB) + enum _cache_type { CACHE_TYPE_NULL = 0, CACHE_TYPE_DATA = 1,
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 <haozhong.zhang@intel.com> --- 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(-)