Re: [PATCH v3 03/28] xen/public: Export cpu featureset information in the public API

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.03.16 at 16:34, <andrew.cooper3@citrix.com> wrote:
> -/* Other features, Linux-defined mapping, word 3 */
> +/* Other features, Linux-defined mapping, FSMAX+1 */
>  /* This range is used for feature bits which conflict or are synthesized */

I think this should become "Xen-defined" and ...

> +#define X86_FEATURE_CONSTANT_TSC	((FSCAPINTS+0)*32+ 8) /* TSC ticks at a constant rate */
> +#define X86_FEATURE_NONSTOP_TSC		((FSCAPINTS+0)*32+ 9) /* TSC does not stop in C states */
> +#define X86_FEATURE_ARAT		((FSCAPINTS+0)*32+10) /* Always running APIC timer */
> +#define X86_FEATURE_ARCH_PERFMON	((FSCAPINTS+0)*32+11) /* Intel Architectural PerfMon */
> +#define X86_FEATURE_TSC_RELIABLE	((FSCAPINTS+0)*32+12) /* TSC is known to be reliable */
> +#define X86_FEATURE_XTOPOLOGY		((FSCAPINTS+0)*32+13) /* cpu topology enum extensions */
> +#define X86_FEATURE_CPUID_FAULTING	((FSCAPINTS+0)*32+14) /* cpuid faulting */
> +#define X86_FEATURE_CLFLUSH_MONITOR	((FSCAPINTS+0)*32+15) /* clflush reqd with monitor */
> +#define X86_FEATURE_APERFMPERF		((FSCAPINTS+0)*32+16) /* APERFMPERF */

... these could get re-adjusted to start at bit zero.

> --- /dev/null
> +++ b/xen/include/asm-x86/cpufeatureset.h
> @@ -0,0 +1,22 @@
> +#ifndef __XEN_X86_CPUFEATURESET_H__
> +#define __XEN_X86_CPUFEATURESET_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +#define XEN_CPUFEATURE(name, value) X86_FEATURE_##name = value,
> +enum {
> +#include <public/arch-x86/cpufeatureset.h>
> +#undef XEN_CPUFEATURE
> +};
> +
> +#define XEN_CPUFEATURE(name, value) asm (".equ X86_FEATURE_" #name ", " 
> #value);
> +#include <public/arch-x86/cpufeatureset.h>
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define XEN_CPUFEATURE(name, value) .equ X86_FEATURE_##name, value
> +#include <public/arch-x86/cpufeatureset.h>
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* !__XEN_X86_CPUFEATURESET_H__ */

It seems wrong for XEN_CPUFEATURE() to remain defined. See
how it would have been easier if you did this just like
public/errno.h does (to your disliking)?

> +#ifndef XEN_CPUFEATURE
> +
> +/*
> + * Includer has not provided a custom XEN_CPUFEATURE().  Arrange for normal
> + * header guards, an automatic enum (for C code) and constants in the
> + * XEN_X86_FEATURE_xxx namespace.
> + */
> +#ifndef __XEN_PUBLIC_ARCH_X86_CPUFEATURESET_H__
> +#define __XEN_PUBLIC_ARCH_X86_CPUFEATURESET_H__
> +
> +#define XEN_CPUFEATURESET_DEFAULT_INCLUDE
> +
> +#ifndef __ASSEMBLY__
> +
> +#define XEN_CPUFEATURE(name, value) XEN_X86_FEATURE_##name = value,
> +enum {
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define XEN_CPUFEATURE(name, value) .equ XEN_X86_FEATURE_##name, value

I think we shouldn't go as far as helping assembly use of these
constants - people can arrange for that themselves by including
this header in a "non-default" way. (Also - who knows which
assemblers understand .equ and which don't.) I now view
having done so in public/errno.h as a mistake, even if those
equates may be slightly more likely to be occasionally used in
assembly code. Therefore I'm inclined to make those dependent
upon __XEN_INTERFACE_VERSION__ < 0x00040700.

Jan


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