Re: [Xen-devel] [PATCH v4 13/26] x86/cpu: Sysctl and common infrastructure for levelling context switching

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/03/16 17:12, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 23, 2016 at 04:36:16PM +0000, Andrew Cooper wrote:
>> A toolstack needs to know how much control Xen has over the visible cpuid
>> values in PV guests.  Provide an explicit mechanism to query what Xen is
>> capable of.
>>
>> This interface will currently report no capabilities.  This change is
>> scaffolding for future patches, which will introduce detection and switching
>> logic, after which the interface will report hardware capabilities correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>
>> v2:
>>  * s/cpumasks/cpuidmasks/
>> v3:
>>  * Reintroduce XEN_SYSCTL_get_levelling_caps (requested by Joao for some
>>    development he has planned).
> s/some/libvirt/
>
>>  * Rename to XEN_SYSCTL_get_cpu_levelling_caps, and rename the constants to
>>    match the Xen command line options.
>> v4:
>>  * Move declarations from processor.h to cpuid.h
>>  * API corrections for XEN_SYSCTL_get_levelling_caps
>> ---
>>  xen/arch/x86/cpu/common.c        |  6 ++++++
>>  xen/arch/x86/sysctl.c            |  6 ++++++
>>  xen/include/asm-x86/cpufeature.h |  1 +
>>  xen/include/asm-x86/cpuid.h      | 32 ++++++++++++++++++++++++++++++++
>>  xen/include/public/sysctl.h      | 23 +++++++++++++++++++++++
>>  5 files changed, 68 insertions(+)
>>
>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>> index b5c023f..7ef75b0 100644
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -36,6 +36,12 @@ integer_param("cpuid_mask_ext_ecx", 
>> opt_cpuid_mask_ext_ecx);
>>  unsigned int opt_cpuid_mask_ext_edx = ~0u;
>>  integer_param("cpuid_mask_ext_edx", opt_cpuid_mask_ext_edx);
>>  
>> +unsigned int __initdata expected_levelling_cap;
>> +unsigned int __read_mostly levelling_caps;
>> +
>> +DEFINE_PER_CPU(struct cpuidmasks, cpuidmasks);
>> +struct cpuidmasks __read_mostly cpuidmask_defaults;
>> +
> Stray changes?

No.  Please refer to the commit message.

If it isn't sufficiently clear, please suggest how better to say "This
patch is deliberately like this to reduce the complexity of the two
following patches, both of which are already very complicated to review".

Compile-wise, you can either have this patch separate (and accept that I
introduce some variables before the code which sets them to a non-zero
value), or folded into the following patch.  I know which way I would
prefer to review them...

>
>>  const struct cpu_dev *__read_mostly cpu_devs[X86_VENDOR_NUM] = {};
>>  
>>  unsigned int paddr_bits __read_mostly = 36;
>> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
>> index 58cbd70..f68cbec 100644
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -190,6 +190,12 @@ long arch_do_sysctl(
>>          }
>>          break;
>>  
>> +    case XEN_SYSCTL_get_cpu_levelling_caps:
>> +        sysctl->u.cpu_levelling_caps.caps = levelling_caps;
>> +        if ( __copy_field_to_guest(u_sysctl, sysctl, 
>> u.cpu_levelling_caps.caps) )
>> +            ret = -EFAULT;
>> +        break;
>> +
>
> You are missing the XSM checks in flask_sysctl and the proper label in 
> attributes files
> (and also the default policy in xen.te).

So I have - will fix.

>
>>      default:
>>          ret = -ENOSYS;
>>          break;
>> diff --git a/xen/include/asm-x86/cpufeature.h 
>> b/xen/include/asm-x86/cpufeature.h
>> index e29b024..84d3220 100644
>> --- a/xen/include/asm-x86/cpufeature.h
>> +++ b/xen/include/asm-x86/cpufeature.h
>> @@ -81,6 +81,7 @@
>>  #define cpu_has_xsavec              boot_cpu_has(X86_FEATURE_XSAVEC)
>>  #define cpu_has_xgetbv1             boot_cpu_has(X86_FEATURE_XGETBV1)
>>  #define cpu_has_xsaves              boot_cpu_has(X86_FEATURE_XSAVES)
>> +#define cpu_has_hypervisor  boot_cpu_has(X86_FEATURE_HYPERVISOR)
>>  
>>  enum _cache_type {
>>      CACHE_TYPE_NULL = 0,
>> diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
>> index 4725672..9a21c25 100644
>> --- a/xen/include/asm-x86/cpuid.h
>> +++ b/xen/include/asm-x86/cpuid.h
>> @@ -3,6 +3,7 @@
>>  
>>  #include <asm/cpufeatureset.h>
>>  #include <asm/cpuid-autogen.h>
>> +#include <asm/percpu.h>
>>  
>>  #define FSCAPINTS FEATURESET_NR_ENTRIES
>>  
>> @@ -18,6 +19,7 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  #include <xen/types.h>
>> +#include <public/sysctl.h>
>>  
>>  extern const uint32_t known_features[FSCAPINTS];
>>  extern const uint32_t special_features[FSCAPINTS];
>> @@ -31,6 +33,36 @@ void calculate_featuresets(void);
>>  
>>  const uint32_t *lookup_deep_deps(uint32_t feature);
>>  
>> +/*
>> + * Expected levelling capabilities (given cpuid vendor/family information),
>> + * and levelling capabilities actually available (given MSR probing).
>> + */
>> +#define LCAP_faulting XEN_SYSCTL_CPU_LEVELCAP_faulting
>> +#define LCAP_1cd      (XEN_SYSCTL_CPU_LEVELCAP_ecx |        \
>> +                       XEN_SYSCTL_CPU_LEVELCAP_edx)
>> +#define LCAP_e1cd     (XEN_SYSCTL_CPU_LEVELCAP_extd_ecx |   \
>> +                       XEN_SYSCTL_CPU_LEVELCAP_extd_edx)
>> +#define LCAP_Da1      XEN_SYSCTL_CPU_LEVELCAP_xsave_eax
>> +#define LCAP_6c       XEN_SYSCTL_CPU_LEVELCAP_thermal_ecx
>> +#define LCAP_7ab0     (XEN_SYSCTL_CPU_LEVELCAP_l7s0_eax |   \
>> +                       XEN_SYSCTL_CPU_LEVELCAP_l7s0_ebx)
>> +extern unsigned int expected_levelling_cap, levelling_caps;
>> +
>> +struct cpuidmasks
>> +{
>> +    uint64_t _1cd;
>> +    uint64_t e1cd;
>> +    uint64_t Da1;
>> +    uint64_t _6c;
>> +    uint64_t _7ab0;
>> +};
>> +
>> +/* Per CPU shadows of masking MSR values, for lazy context switching. */
>> +DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
> Ditto? Should these be in another patchset?
>
> Why not make this patch only introduce the sub-ops and the very first
> patch that uses the cpuidmasks have these squashed in?

Because squashing this patch and the following patch will make something
far more difficult to review.

>
>> +
>> +/* Default masking MSR values, calculated at boot. */
>> +extern struct cpuidmasks cpuidmask_defaults;
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* !__X86_CPUID_H__ */
>>  
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 96680eb..1ab16db 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -766,6 +766,27 @@ struct xen_sysctl_tmem_op {
>>  typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t);
>>  
>> +/*
>> + * XEN_SYSCTL_get_cpu_levelling_caps (x86 specific)
>> + *
>> + * Return hardware capabilities concerning masking or faulting of the cpuid
>> + * instruction for PV guests.
> Just PV? Not HVM?

HVM doesn't have any cpuid problems; the cpuid instruction traps to Xen
under all circumstances.

>
>> + */
>> +struct xen_sysctl_cpu_levelling_caps {
> I presume these are the /*IN*/ args?

They are /*OUT*/, as only a get_cpu_levelling_caps hypercall is
defined.  I can't see a plausible use for a set variant of this
hypercall, but I have named the structure in a neutral way.

>
>
>> +#define XEN_SYSCTL_CPU_LEVELCAP_faulting    (1ul <<  0) /* CPUID faulting   
>>  */
> Perhaps also point to what they look like in the Xen header file?
>
> And mention that they differ in word placement from what Linux has
> (which in turns impacts how toolstack may mash this together).

Are you getting confused?  This has nothing to do with the featureset api.

This reports which cpuid leaves can be controlled by Xen, based on the
hardware availability of masking msrs or cpuid faulting.

~Andrew

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