Re: [Xen-devel] [PATCH 1/4] x86/hvm: allow guest to use clflushopt and clwb

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

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@xxxxxxxxx>
> 
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel