* [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support
@ 2018-07-13 20:03 Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
` (13 more replies)
0 siblings, 14 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Daniel De Graaf, Roger Pau Monné
This series introduces libx86, a small shared library between the hypervisor
and libxc, and hypercalls to get full CPUID/MSR policies. Future work will
implement XEN_DOMCTL_set_cpumsr_policy, after the auditing and comparison
logic is sorted.
In the meantime, the data marshalling logic is in a suitable state for review.
This series is based on staging, and can be found in git form here:
http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/cpuid-marshal-v2
There are many changes from v1. See individual patches for details.
Andrew Cooper (6):
x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl
libx86: Introduce libx86/cpuid.h
libx86: Introduce libx86/msr.h and share msr_policy with userspace
libx86: Introduce a helper to serialise cpuid_policy objects
libx86: Introduce a helper to deserialise cpuid_policy objects
x86: Introduce struct cpu_policy to refer to a group of individual policies
Roger Pau Monné (5):
libx86: generate cpuid-autogen.h in the libx86 include dir
libx86: Share struct cpuid_policy with userspace
libx86: introduce a libx86 shared library
libx86: Introduce a helper to serialise msr_policy objects
libx86: introduce a helper to deserialise msr_policy objects
Sergey Dyasli (2):
x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
.gitignore | 2 +-
tools/include/Makefile | 5 +
tools/include/xen-tools/libs.h | 8 +
tools/libxc/Makefile | 15 +-
tools/libxc/include/xenctrl.h | 10 +-
tools/libxc/xc_cpuid_x86.c | 112 +++++++++++---
tools/misc/xen-cpuid.c | 133 +++++++++++++++-
tools/xenstore/xenstored_core.h | 2 -
xen/arch/x86/cpu/common.c | 2 +-
xen/arch/x86/cpuid.c | 32 +---
xen/arch/x86/domctl.c | 34 +++++
xen/arch/x86/sysctl.c | 98 ++++++++++++
xen/arch/x86/x86_emulate/x86_emulate.h | 7 +-
xen/common/Makefile | 1 +
xen/common/libx86/Makefile | 2 +
xen/common/libx86/cpuid.c | 228 ++++++++++++++++++++++++++++
xen/common/libx86/msr.c | 107 +++++++++++++
xen/common/libx86/private.h | 73 +++++++++
xen/include/Makefile | 11 +-
xen/include/asm-x86/cpufeatures.h | 2 +-
xen/include/asm-x86/cpuid.h | 228 +---------------------------
xen/include/asm-x86/msr.h | 28 +---
xen/include/public/arch-x86/xen.h | 18 +++
xen/include/public/domctl.h | 18 +++
xen/include/public/sysctl.h | 37 +++++
xen/include/xen/libx86/Makefile | 8 +
xen/include/xen/libx86/cpu-policy.h | 24 +++
xen/include/{asm-x86 => xen/libx86}/cpuid.h | 107 ++++++-------
xen/include/xen/libx86/msr.h | 65 ++++++++
xen/xsm/flask/hooks.c | 2 +
xen/xsm/flask/policy/access_vectors | 3 +-
31 files changed, 1036 insertions(+), 386 deletions(-)
create mode 100644 xen/common/libx86/Makefile
create mode 100644 xen/common/libx86/cpuid.c
create mode 100644 xen/common/libx86/msr.c
create mode 100644 xen/common/libx86/private.h
create mode 100644 xen/include/xen/libx86/Makefile
create mode 100644 xen/include/xen/libx86/cpu-policy.h
copy xen/include/{asm-x86 => xen/libx86}/cpuid.h (77%)
create mode 100644 xen/include/xen/libx86/msr.h
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 7:21 ` Jan Beulich
2018-07-16 9:37 ` Roger Pau Monné
2018-07-13 20:03 ` [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h Andrew Cooper
` (12 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Sergey Dyasli, Wei Liu, Jan Beulich, Roger Pau Monné
More than the bottom two bits are now defined, and the MSR policy work has
shown that using non-architectural representations turns out to be problematic
for more than just asm code. As the architectural representation is the
expected default, we don't need to justify why we are using it.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
New in v2
---
xen/include/asm-x86/msr.h | 5 -----
1 file changed, 5 deletions(-)
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 3a2c799..9b4e4e0 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -287,11 +287,6 @@ struct vcpu_msrs
{
/* 0x00000048 - MSR_SPEC_CTRL */
struct {
- /*
- * Only the bottom two bits are defined, so no need to waste space
- * with uint64_t at the moment, but use uint32_t for the convenience
- * of the assembly code.
- */
uint32_t raw;
} spec_ctrl;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:23 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir Andrew Cooper
` (11 subsequent siblings)
13 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Sergey Dyasli, Ian Jackson, Jan Beulich,
Roger Pau Monné
Begin to untangle the header dependency tangle by moving definition of
struct cpuid_leaf out of x86_emulate.h into the new cpuid.h.
Additionally, plumb the header through to libxc. This is technically a
redundant include at this point, but it helps build-test the later changes,
and will be used eventually.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
Note concerning the positioning of libx86. It turns out after trying to move
it elsewhere that the movement is prohibitive because of the way Xen headers
are included by the tools.
For now, this is consistent with the other libs, and we can move it in the
future after some hygene has been applied to the build system.
---
tools/include/Makefile | 1 +
tools/libxc/xc_cpuid_x86.c | 2 ++
xen/arch/x86/x86_emulate/x86_emulate.h | 7 ++-----
xen/include/asm-x86/cpuid.h | 4 +++-
xen/include/xen/libx86/cpuid.h | 20 ++++++++++++++++++++
5 files changed, 28 insertions(+), 6 deletions(-)
create mode 100644 xen/include/xen/libx86/cpuid.h
diff --git a/tools/include/Makefile b/tools/include/Makefile
index 270a34f..a2403fc 100644
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -23,6 +23,7 @@ xen/.dir:
ln -sf $(XEN_ROOT)/xen/include/acpi acpi
ifeq ($(CONFIG_X86),y)
ln -sf $(XEN_ROOT)/xen/include/asm-x86 xen/asm
+ ln -sf $(XEN_ROOT)/xen/include/xen/libx86 xen/libx86
endif
touch $@
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index c5c3cdc..090e199 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -33,6 +33,8 @@ enum {
};
#include "_xc_cpuid_autogen.h"
+#include <xen/libx86/cpuid.h>
+
#define bitmaskof(idx) (1u << ((idx) & 31))
#define featureword_of(idx) ((idx) >> 5)
#define clear_feature(idx, dst) ((dst) &= ~bitmaskof(idx))
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index c22e774..abd9796 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -23,6 +23,8 @@
#ifndef __X86_EMULATE_H__
#define __X86_EMULATE_H__
+#include <xen/libx86/cpuid.h>
+
#define MAX_INST_LEN 15
#if defined(__i386__)
@@ -172,11 +174,6 @@ enum x86_emulate_fpu_type {
X86EMUL_FPU_none
};
-struct cpuid_leaf
-{
- uint32_t a, b, c, d;
-};
-
struct x86_emulate_state;
/*
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 4113a5e..9637303 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -18,7 +18,9 @@
#ifndef __ASSEMBLY__
#include <xen/types.h>
#include <xen/kernel.h>
-#include <asm/x86_emulate.h>
+
+#include <xen/libx86/cpuid.h>
+
#include <public/sysctl.h>
extern const uint32_t known_features[FSCAPINTS];
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
new file mode 100644
index 0000000..3ccc68e
--- /dev/null
+++ b/xen/include/xen/libx86/cpuid.h
@@ -0,0 +1,20 @@
+/* Common data structures and functions consumed by hypervisor and toolstack */
+#ifndef XEN_LIBX86_CPUID_H
+#define XEN_LIBX86_CPUID_H
+
+struct cpuid_leaf
+{
+ uint32_t a, b, c, d;
+};
+
+#endif /* !XEN_LIBX86_CPUID_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:31 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace Andrew Cooper
` (10 subsequent siblings)
13 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
From: Roger Pau Monné <roger.pau@citrix.com>
This avoids all users needing to opencode local generation of the file.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Rewrite from scratch
---
.gitignore | 2 +-
tools/include/Makefile | 6 +++++-
tools/libxc/Makefile | 9 ---------
tools/libxc/xc_cpuid_x86.c | 1 -
xen/include/Makefile | 11 +++++------
xen/include/asm-x86/cpufeatures.h | 2 +-
xen/include/xen/libx86/Makefile | 8 ++++++++
xen/include/xen/libx86/cpuid.h | 2 ++
8 files changed, 22 insertions(+), 19 deletions(-)
create mode 100644 xen/include/xen/libx86/Makefile
diff --git a/.gitignore b/.gitignore
index 5b8448d..861f2ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -311,7 +311,6 @@ xen/arch/*/efi/runtime.c
xen/include/headers*.chk
xen/include/asm
xen/include/asm-*/asm-offsets.h
-xen/include/asm-x86/cpuid-autogen.h
xen/include/compat/*
xen/include/config/
xen/include/generated/
@@ -319,6 +318,7 @@ xen/include/public/public
xen/include/xen/*.new
xen/include/xen/acm_policy.h
xen/include/xen/compile.h
+xen/include/xen/libx86/cpuid-autogen.h
xen/test/livepatch/config.h
xen/test/livepatch/xen_bye_world.livepatch
xen/test/livepatch/xen_hello_world.livepatch
diff --git a/tools/include/Makefile b/tools/include/Makefile
index a2403fc..07162a7 100644
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -23,7 +23,11 @@ xen/.dir:
ln -sf $(XEN_ROOT)/xen/include/acpi acpi
ifeq ($(CONFIG_X86),y)
ln -sf $(XEN_ROOT)/xen/include/asm-x86 xen/asm
- ln -sf $(XEN_ROOT)/xen/include/xen/libx86 xen/libx86
+ mkdir -p xen/libx86
+ for f in $(filter-out %autogen.h,$(patsubst $(XEN_ROOT)/xen/include/xen/libx86/%,%,Makefile $(wildcard $(XEN_ROOT)/xen/include/xen/libx86/*.h))); do \
+ ln -sf $(XEN_ROOT)/xen/include/xen/libx86/$$f xen/libx86/$$f; \
+ done
+ $(MAKE) -C xen/libx86 all XEN_ROOT=$(XEN_ROOT)
endif
touch $@
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 157553c..ca2b203 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -147,15 +147,6 @@ $(eval $(genpath-target))
xc_private.h: _paths.h
-ifeq ($(CONFIG_X86),y)
-
-_xc_cpuid_autogen.h: $(XEN_ROOT)/xen/include/public/arch-x86/cpufeatureset.h $(XEN_ROOT)/xen/tools/gen-cpuid.py
- $(PYTHON) $(XEN_ROOT)/xen/tools/gen-cpuid.py -i $^ -o $@.new
- $(call move-if-changed,$@.new,$@)
-
-build: _xc_cpuid_autogen.h
-endif
-
$(CTRL_LIB_OBJS) $(GUEST_LIB_OBJS) \
$(CTRL_PIC_OBJS) $(GUEST_PIC_OBJS): xc_private.h
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 090e199..d2f85aa 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -31,7 +31,6 @@ enum {
#define XEN_CPUFEATURE(name, value) X86_FEATURE_##name = value,
#include <xen/arch-x86/cpufeatureset.h>
};
-#include "_xc_cpuid_autogen.h"
#include <xen/libx86/cpuid.h>
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 7c5034e..7b4e862 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -141,14 +141,13 @@ headers++.chk: $(PUBLIC_HEADERS) Makefile
endif
ifeq ($(XEN_TARGET_ARCH),x86_64)
+.PHONY: libx86-all
+libx86-all:
+ $(MAKE) -C xen/libx86 all
-$(BASEDIR)/include/asm-x86/cpuid-autogen.h: $(BASEDIR)/include/public/arch-x86/cpufeatureset.h $(BASEDIR)/tools/gen-cpuid.py FORCE
- $(PYTHON) $(BASEDIR)/tools/gen-cpuid.py -i $< -o $@.new
- $(call move-if-changed,$@.new,$@)
-
-all: $(BASEDIR)/include/asm-x86/cpuid-autogen.h
+all: libx86-all
endif
clean::
rm -rf compat config generated headers*.chk
- rm -f $(BASEDIR)/include/asm-x86/cpuid-autogen.h
+ rm -f $(BASEDIR)/include/xen/libx86/cpuid-autogen.h
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index 8e5cc53..b7ceb20 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -2,7 +2,7 @@
* Explicitly intended for multiple inclusion.
*/
-#include <asm/cpuid-autogen.h>
+#include <xen/libx86/cpuid-autogen.h>
#define FSCAPINTS FEATURESET_NR_ENTRIES
diff --git a/xen/include/xen/libx86/Makefile b/xen/include/xen/libx86/Makefile
new file mode 100644
index 0000000..408d69c
--- /dev/null
+++ b/xen/include/xen/libx86/Makefile
@@ -0,0 +1,8 @@
+include $(XEN_ROOT)/Config.mk
+
+.PHONY: all
+all: cpuid-autogen.h
+
+cpuid-autogen.h: $(XEN_ROOT)/xen/include/public/arch-x86/cpufeatureset.h $(XEN_ROOT)/xen/tools/gen-cpuid.py
+ $(PYTHON) $(XEN_ROOT)/xen/tools/gen-cpuid.py -i $< -o $@.new
+ $(call move-if-changed,$@.new,$@)
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
index 3ccc68e..8f101ba 100644
--- a/xen/include/xen/libx86/cpuid.h
+++ b/xen/include/xen/libx86/cpuid.h
@@ -2,6 +2,8 @@
#ifndef XEN_LIBX86_CPUID_H
#define XEN_LIBX86_CPUID_H
+#include <xen/libx86/cpuid-autogen.h>
+
struct cpuid_leaf
{
uint32_t a, b, c, d;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (2 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:38 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 05/13] libx86: introduce a libx86 shared library Andrew Cooper
` (9 subsequent siblings)
13 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Sergey Dyasli, Ian Jackson, Jan Beulich,
Roger Pau Monné
From: Roger Pau Monné <roger.pau@citrix.com>
Both Xen and the toolstack have need of the same logic when it comes to
manipulation and checking of the CPUID and MSR values offered to guests. To
that end, libx86 is being introduced to allow Xen and the toolstack to share a
single implementation, rather than duplicating the logic.
No functional change.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Move MAX() into xen-tools/libs.h
* Expand commit message.
---
tools/include/xen-tools/libs.h | 4 +
xen/include/asm-x86/cpuid.h | 219 -----------------------------------------
xen/include/xen/libx86/cpuid.h | 219 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 223 insertions(+), 219 deletions(-)
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index 63e3507..f78a3b5 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -13,4 +13,8 @@
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
#endif
+#ifndef MAX
+#define MAX(x, y) ((x) > (y) ? (x) : (y))
+#endif
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 9637303..7a3f2f4 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -4,17 +4,6 @@
#include <asm/cpufeatureset.h>
#include <asm/percpu.h>
-#define FEATURESET_1d 0 /* 0x00000001.edx */
-#define FEATURESET_1c 1 /* 0x00000001.ecx */
-#define FEATURESET_e1d 2 /* 0x80000001.edx */
-#define FEATURESET_e1c 3 /* 0x80000001.ecx */
-#define FEATURESET_Da1 4 /* 0x0000000d:1.eax */
-#define FEATURESET_7b0 5 /* 0x00000007:0.ebx */
-#define FEATURESET_7c0 6 /* 0x00000007:0.ecx */
-#define FEATURESET_e7d 7 /* 0x80000007.edx */
-#define FEATURESET_e8b 8 /* 0x80000008.ebx */
-#define FEATURESET_7d0 9 /* 0x00000007:0.edx */
-
#ifndef __ASSEMBLY__
#include <xen/types.h>
#include <xen/kernel.h>
@@ -60,214 +49,6 @@ DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
/* Default masking MSR values, calculated at boot. */
extern struct cpuidmasks cpuidmask_defaults;
-#define CPUID_GUEST_NR_BASIC (0xdu + 1)
-#define CPUID_GUEST_NR_FEAT (0u + 1)
-#define CPUID_GUEST_NR_CACHE (5u + 1)
-#define CPUID_GUEST_NR_TOPO (1u + 1)
-#define CPUID_GUEST_NR_XSTATE (62u + 1)
-#define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
-#define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1)
-#define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \
- CPUID_GUEST_NR_EXTD_AMD)
-
-struct cpuid_policy
-{
-#define DECL_BITFIELD(word) _DECL_BITFIELD(FEATURESET_ ## word)
-#define _DECL_BITFIELD(x) __DECL_BITFIELD(x)
-#define __DECL_BITFIELD(x) CPUID_BITFIELD_ ## x
-
- /* Basic leaves: 0x000000xx */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_BASIC];
- struct {
- /* Leaf 0x0 - Max and vendor. */
- uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
-
- /* Leaf 0x1 - Family/model/stepping and features. */
- uint32_t raw_fms;
- uint8_t :8, /* Brand ID. */
- clflush_size, /* Number of 8-byte blocks per cache line. */
- lppp, /* Logical processors per package. */
- apic_id; /* Initial APIC ID. */
- union {
- uint32_t _1c;
- struct { DECL_BITFIELD(1c); };
- };
- union {
- uint32_t _1d;
- struct { DECL_BITFIELD(1d); };
- };
-
- /* Leaf 0x2 - TLB/Cache/Prefetch. */
- uint8_t l2_nr_queries; /* Documented as fixed to 1. */
- uint8_t l2_desc[15];
-
- uint64_t :64, :64; /* Leaf 0x3 - PSN. */
- uint64_t :64, :64; /* Leaf 0x4 - Structured Cache. */
- uint64_t :64, :64; /* Leaf 0x5 - MONITOR. */
- uint64_t :64, :64; /* Leaf 0x6 - Therm/Perf. */
- uint64_t :64, :64; /* Leaf 0x7 - Structured Features. */
- uint64_t :64, :64; /* Leaf 0x8 - rsvd */
- uint64_t :64, :64; /* Leaf 0x9 - DCA */
-
- /* Leaf 0xa - Intel PMU. */
- uint8_t pmu_version, _pmu[15];
-
- uint64_t :64, :64; /* Leaf 0xb - Topology. */
- uint64_t :64, :64; /* Leaf 0xc - rsvd */
- uint64_t :64, :64; /* Leaf 0xd - XSTATE. */
- };
- } basic;
-
- /* Structured cache leaf: 0x00000004[xx] */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE];
- struct cpuid_cache_leaf {
- uint32_t type:5,
- :27, :32, :32, :32;
- } subleaf[CPUID_GUEST_NR_CACHE];
- } cache;
-
- /* Structured feature leaf: 0x00000007[xx] */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_FEAT];
- struct {
- /* Subleaf 0. */
- uint32_t max_subleaf;
- union {
- uint32_t _7b0;
- struct { DECL_BITFIELD(7b0); };
- };
- union {
- uint32_t _7c0;
- struct { DECL_BITFIELD(7c0); };
- };
- union {
- uint32_t _7d0;
- struct { DECL_BITFIELD(7d0); };
- };
- };
- } feat;
-
- /* Extended topology enumeration: 0x0000000B[xx] */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_TOPO];
- struct cpuid_topo_leaf {
- uint32_t id_shift:5, :27;
- uint16_t nr_logical, :16;
- uint8_t level, type, :8, :8;
- uint32_t x2apic_id;
- } subleaf[CPUID_GUEST_NR_TOPO];
- } topo;
-
- /* Xstate feature leaf: 0x0000000D[xx] */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_XSTATE];
-
- struct {
- /* Subleaf 0. */
- uint32_t xcr0_low, /* b */:32, max_size, xcr0_high;
-
- /* Subleaf 1. */
- union {
- uint32_t Da1;
- struct { DECL_BITFIELD(Da1); };
- };
- uint32_t /* b */:32, xss_low, xss_high;
- };
-
- /* Per-component common state. Valid for i >= 2. */
- struct {
- uint32_t size, offset;
- bool xss:1, align:1;
- uint32_t _res_d;
- } comp[CPUID_GUEST_NR_XSTATE];
- } xstate;
-
- /* Extended leaves: 0x800000xx */
- union {
- struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
- struct {
- /* Leaf 0x80000000 - Max and vendor. */
- uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
-
- /* Leaf 0x80000001 - Family/model/stepping and features. */
- uint32_t raw_fms, /* b */:32;
- union {
- uint32_t e1c;
- struct { DECL_BITFIELD(e1c); };
- };
- union {
- uint32_t e1d;
- struct { DECL_BITFIELD(e1d); };
- };
-
- uint64_t :64, :64; /* Brand string. */
- uint64_t :64, :64; /* Brand string. */
- uint64_t :64, :64; /* Brand string. */
- uint64_t :64, :64; /* L1 cache/TLB. */
- uint64_t :64, :64; /* L2/3 cache/TLB. */
-
- /* Leaf 0x80000007 - Advanced Power Management. */
- uint32_t /* a */:32, /* b */:32, /* c */:32;
- union {
- uint32_t e7d;
- struct { DECL_BITFIELD(e7d); };
- };
-
- /* Leaf 0x80000008 - Misc addr/feature info. */
- uint8_t maxphysaddr, maxlinaddr, :8, :8;
- union {
- uint32_t e8b;
- struct { DECL_BITFIELD(e8b); };
- };
- uint32_t /* c */:32, /* d */:32;
- };
- } extd;
-
-#undef __DECL_BITFIELD
-#undef _DECL_BITFIELD
-#undef DECL_BITFIELD
-
- /* Toolstack selected Hypervisor max_leaf (if non-zero). */
- uint8_t hv_limit, hv2_limit;
-
- /* Value calculated from raw data above. */
- uint8_t x86_vendor;
-};
-
-/* Fill in a featureset bitmap from a CPUID policy. */
-static inline void cpuid_policy_to_featureset(
- const struct cpuid_policy *p, uint32_t fs[FSCAPINTS])
-{
- fs[FEATURESET_1d] = p->basic._1d;
- fs[FEATURESET_1c] = p->basic._1c;
- fs[FEATURESET_e1d] = p->extd.e1d;
- fs[FEATURESET_e1c] = p->extd.e1c;
- fs[FEATURESET_Da1] = p->xstate.Da1;
- fs[FEATURESET_7b0] = p->feat._7b0;
- fs[FEATURESET_7c0] = p->feat._7c0;
- fs[FEATURESET_e7d] = p->extd.e7d;
- fs[FEATURESET_e8b] = p->extd.e8b;
- fs[FEATURESET_7d0] = p->feat._7d0;
-}
-
-/* Fill in a CPUID policy from a featureset bitmap. */
-static inline void cpuid_featureset_to_policy(
- const uint32_t fs[FSCAPINTS], struct cpuid_policy *p)
-{
- p->basic._1d = fs[FEATURESET_1d];
- p->basic._1c = fs[FEATURESET_1c];
- p->extd.e1d = fs[FEATURESET_e1d];
- p->extd.e1c = fs[FEATURESET_e1c];
- p->xstate.Da1 = fs[FEATURESET_Da1];
- p->feat._7b0 = fs[FEATURESET_7b0];
- p->feat._7c0 = fs[FEATURESET_7c0];
- p->extd.e7d = fs[FEATURESET_e7d];
- p->extd.e8b = fs[FEATURESET_e8b];
- p->feat._7d0 = fs[FEATURESET_7d0];
-}
-
extern struct cpuid_policy raw_cpuid_policy, host_cpuid_policy,
pv_max_cpuid_policy, hvm_max_cpuid_policy;
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
index 8f101ba..69bd8a9 100644
--- a/xen/include/xen/libx86/cpuid.h
+++ b/xen/include/xen/libx86/cpuid.h
@@ -4,11 +4,230 @@
#include <xen/libx86/cpuid-autogen.h>
+#define FEATURESET_1d 0 /* 0x00000001.edx */
+#define FEATURESET_1c 1 /* 0x00000001.ecx */
+#define FEATURESET_e1d 2 /* 0x80000001.edx */
+#define FEATURESET_e1c 3 /* 0x80000001.ecx */
+#define FEATURESET_Da1 4 /* 0x0000000d:1.eax */
+#define FEATURESET_7b0 5 /* 0x00000007:0.ebx */
+#define FEATURESET_7c0 6 /* 0x00000007:0.ecx */
+#define FEATURESET_e7d 7 /* 0x80000007.edx */
+#define FEATURESET_e8b 8 /* 0x80000008.ebx */
+#define FEATURESET_7d0 9 /* 0x00000007:0.edx */
+
struct cpuid_leaf
{
uint32_t a, b, c, d;
};
+#define CPUID_GUEST_NR_BASIC (0xdu + 1)
+#define CPUID_GUEST_NR_FEAT (0u + 1)
+#define CPUID_GUEST_NR_CACHE (5u + 1)
+#define CPUID_GUEST_NR_TOPO (1u + 1)
+#define CPUID_GUEST_NR_XSTATE (62u + 1)
+#define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
+#define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1)
+#define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \
+ CPUID_GUEST_NR_EXTD_AMD)
+
+struct cpuid_policy
+{
+#define DECL_BITFIELD(word) _DECL_BITFIELD(FEATURESET_ ## word)
+#define _DECL_BITFIELD(x) __DECL_BITFIELD(x)
+#define __DECL_BITFIELD(x) CPUID_BITFIELD_ ## x
+
+ /* Basic leaves: 0x000000xx */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_BASIC];
+ struct {
+ /* Leaf 0x0 - Max and vendor. */
+ uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
+
+ /* Leaf 0x1 - Family/model/stepping and features. */
+ uint32_t raw_fms;
+ uint8_t :8, /* Brand ID. */
+ clflush_size, /* Number of 8-byte blocks per cache line. */
+ lppp, /* Logical processors per package. */
+ apic_id; /* Initial APIC ID. */
+ union {
+ uint32_t _1c;
+ struct { DECL_BITFIELD(1c); };
+ };
+ union {
+ uint32_t _1d;
+ struct { DECL_BITFIELD(1d); };
+ };
+
+ /* Leaf 0x2 - TLB/Cache/Prefetch. */
+ uint8_t l2_nr_queries; /* Documented as fixed to 1. */
+ uint8_t l2_desc[15];
+
+ uint64_t :64, :64; /* Leaf 0x3 - PSN. */
+ uint64_t :64, :64; /* Leaf 0x4 - Structured Cache. */
+ uint64_t :64, :64; /* Leaf 0x5 - MONITOR. */
+ uint64_t :64, :64; /* Leaf 0x6 - Therm/Perf. */
+ uint64_t :64, :64; /* Leaf 0x7 - Structured Features. */
+ uint64_t :64, :64; /* Leaf 0x8 - rsvd */
+ uint64_t :64, :64; /* Leaf 0x9 - DCA */
+
+ /* Leaf 0xa - Intel PMU. */
+ uint8_t pmu_version, _pmu[15];
+
+ uint64_t :64, :64; /* Leaf 0xb - Topology. */
+ uint64_t :64, :64; /* Leaf 0xc - rsvd */
+ uint64_t :64, :64; /* Leaf 0xd - XSTATE. */
+ };
+ } basic;
+
+ /* Structured cache leaf: 0x00000004[xx] */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE];
+ struct cpuid_cache_leaf {
+ uint32_t type:5,
+ :27, :32, :32, :32;
+ } subleaf[CPUID_GUEST_NR_CACHE];
+ } cache;
+
+ /* Structured feature leaf: 0x00000007[xx] */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_FEAT];
+ struct {
+ /* Subleaf 0. */
+ uint32_t max_subleaf;
+ union {
+ uint32_t _7b0;
+ struct { DECL_BITFIELD(7b0); };
+ };
+ union {
+ uint32_t _7c0;
+ struct { DECL_BITFIELD(7c0); };
+ };
+ union {
+ uint32_t _7d0;
+ struct { DECL_BITFIELD(7d0); };
+ };
+ };
+ } feat;
+
+ /* Extended topology enumeration: 0x0000000B[xx] */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_TOPO];
+ struct cpuid_topo_leaf {
+ uint32_t id_shift:5, :27;
+ uint16_t nr_logical, :16;
+ uint8_t level, type, :8, :8;
+ uint32_t x2apic_id;
+ } subleaf[CPUID_GUEST_NR_TOPO];
+ } topo;
+
+ /* Xstate feature leaf: 0x0000000D[xx] */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_XSTATE];
+
+ struct {
+ /* Subleaf 0. */
+ uint32_t xcr0_low, /* b */:32, max_size, xcr0_high;
+
+ /* Subleaf 1. */
+ union {
+ uint32_t Da1;
+ struct { DECL_BITFIELD(Da1); };
+ };
+ uint32_t /* b */:32, xss_low, xss_high;
+ };
+
+ /* Per-component common state. Valid for i >= 2. */
+ struct {
+ uint32_t size, offset;
+ bool xss:1, align:1;
+ uint32_t _res_d;
+ } comp[CPUID_GUEST_NR_XSTATE];
+ } xstate;
+
+ /* Extended leaves: 0x800000xx */
+ union {
+ struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
+ struct {
+ /* Leaf 0x80000000 - Max and vendor. */
+ uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
+
+ /* Leaf 0x80000001 - Family/model/stepping and features. */
+ uint32_t raw_fms, /* b */:32;
+ union {
+ uint32_t e1c;
+ struct { DECL_BITFIELD(e1c); };
+ };
+ union {
+ uint32_t e1d;
+ struct { DECL_BITFIELD(e1d); };
+ };
+
+ uint64_t :64, :64; /* Brand string. */
+ uint64_t :64, :64; /* Brand string. */
+ uint64_t :64, :64; /* Brand string. */
+ uint64_t :64, :64; /* L1 cache/TLB. */
+ uint64_t :64, :64; /* L2/3 cache/TLB. */
+
+ /* Leaf 0x80000007 - Advanced Power Management. */
+ uint32_t /* a */:32, /* b */:32, /* c */:32;
+ union {
+ uint32_t e7d;
+ struct { DECL_BITFIELD(e7d); };
+ };
+
+ /* Leaf 0x80000008 - Misc addr/feature info. */
+ uint8_t maxphysaddr, maxlinaddr, :8, :8;
+ union {
+ uint32_t e8b;
+ struct { DECL_BITFIELD(e8b); };
+ };
+ uint32_t /* c */:32, /* d */:32;
+ };
+ } extd;
+
+#undef __DECL_BITFIELD
+#undef _DECL_BITFIELD
+#undef DECL_BITFIELD
+
+ /* Toolstack selected Hypervisor max_leaf (if non-zero). */
+ uint8_t hv_limit, hv2_limit;
+
+ /* Value calculated from raw data above. */
+ uint8_t x86_vendor;
+};
+
+/* Fill in a featureset bitmap from a CPUID policy. */
+static inline void cpuid_policy_to_featureset(
+ const struct cpuid_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
+{
+ fs[FEATURESET_1d] = p->basic._1d;
+ fs[FEATURESET_1c] = p->basic._1c;
+ fs[FEATURESET_e1d] = p->extd.e1d;
+ fs[FEATURESET_e1c] = p->extd.e1c;
+ fs[FEATURESET_Da1] = p->xstate.Da1;
+ fs[FEATURESET_7b0] = p->feat._7b0;
+ fs[FEATURESET_7c0] = p->feat._7c0;
+ fs[FEATURESET_e7d] = p->extd.e7d;
+ fs[FEATURESET_e8b] = p->extd.e8b;
+ fs[FEATURESET_7d0] = p->feat._7d0;
+}
+
+/* Fill in a CPUID policy from a featureset bitmap. */
+static inline void cpuid_featureset_to_policy(
+ const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
+{
+ p->basic._1d = fs[FEATURESET_1d];
+ p->basic._1c = fs[FEATURESET_1c];
+ p->extd.e1d = fs[FEATURESET_e1d];
+ p->extd.e1c = fs[FEATURESET_e1c];
+ p->xstate.Da1 = fs[FEATURESET_Da1];
+ p->feat._7b0 = fs[FEATURESET_7b0];
+ p->feat._7c0 = fs[FEATURESET_7c0];
+ p->extd.e7d = fs[FEATURESET_e7d];
+ p->extd.e8b = fs[FEATURESET_e8b];
+ p->feat._7d0 = fs[FEATURESET_7d0];
+}
+
#endif /* !XEN_LIBX86_CPUID_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 05/13] libx86: introduce a libx86 shared library
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (3 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:02 ` Wei Liu
2018-07-16 10:17 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace Andrew Cooper
` (8 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
From: Roger Pau Monné <roger.pau@citrix.com>
Move x86_cpuid_lookup_deep_deps() into the shared library, removing the
individual copies from the hypervisor and libxc respectively.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Rename libx86-private.h to just private.h
* Only vpath libx86 for CONFIG_X86 builds of libxc
---
tools/libxc/Makefile | 6 ++++++
tools/libxc/include/xenctrl.h | 1 -
tools/libxc/xc_cpuid_x86.c | 29 +---------------------------
xen/arch/x86/cpu/common.c | 2 +-
xen/arch/x86/cpuid.c | 32 +-----------------------------
xen/common/Makefile | 1 +
xen/common/libx86/Makefile | 1 +
xen/common/libx86/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++
xen/common/libx86/private.h | 38 ++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/cpuid.h | 2 --
xen/include/xen/libx86/cpuid.h | 2 ++
11 files changed, 95 insertions(+), 63 deletions(-)
create mode 100644 xen/common/libx86/Makefile
create mode 100644 xen/common/libx86/cpuid.c
create mode 100644 xen/common/libx86/private.h
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index ca2b203..cd4225c 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -80,6 +80,12 @@ GUEST_SRCS-y += $(ELF_SRCS-y)
$(patsubst %.c,%.o,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
$(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
+ifeq ($(CONFIG_X86),y) # Add libx86 to the build
+vpath %.c ../../xen/common/libx86
+
+GUEST_SRCS-y += cpuid.c
+endif
+
# new domain builder
GUEST_SRCS-y += xc_dom_core.c xc_dom_boot.c
GUEST_SRCS-y += xc_dom_elfloader.c
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 5520c94..dd7d8a9 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2564,7 +2564,6 @@ enum xc_static_cpu_featuremask {
XC_FEATUREMASK_DEEP_FEATURES,
};
const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
-const uint32_t *xc_get_feature_deep_deps(uint32_t feature);
#endif
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index d2f85aa..3c214c8 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -131,33 +131,6 @@ const uint32_t *xc_get_static_cpu_featuremask(
}
}
-const uint32_t *xc_get_feature_deep_deps(uint32_t feature)
-{
- static const struct {
- uint32_t feature;
- uint32_t fs[FEATURESET_NR_ENTRIES];
- } deep_deps[] = INIT_DEEP_DEPS;
-
- unsigned int start = 0, end = ARRAY_SIZE(deep_deps);
-
- BUILD_BUG_ON(ARRAY_SIZE(deep_deps) != NR_DEEP_DEPS);
-
- /* deep_deps[] is sorted. Perform a binary search. */
- while ( start < end )
- {
- unsigned int mid = start + ((end - start) / 2);
-
- if ( deep_deps[mid].feature > feature )
- end = mid;
- else if ( deep_deps[mid].feature < feature )
- start = mid + 1;
- else
- return deep_deps[mid].fs;
- }
-
- return NULL;
-}
-
struct cpuid_domain_info
{
enum
@@ -677,7 +650,7 @@ static void sanitise_featureset(struct cpuid_domain_info *info)
const uint32_t *dfs;
if ( !test_bit(b, disabled_features) ||
- !(dfs = xc_get_feature_deep_deps(b)) )
+ !(dfs = x86_cpuid_lookup_deep_deps(b)) )
continue;
for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 6570c2d..185fefe 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -62,7 +62,7 @@ void __init setup_clear_cpu_cap(unsigned int cap)
__builtin_return_address(0), cap);
__clear_bit(cap, boot_cpu_data.x86_capability);
- dfs = lookup_deep_deps(cap);
+ dfs = x86_cpuid_lookup_deep_deps(cap);
if (!dfs)
return;
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 3c29191..731ccb6 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -97,7 +97,7 @@ static void sanitise_featureset(uint32_t *fs)
for_each_set_bit(i, (void *)disabled_features,
sizeof(disabled_features) * 8)
{
- const uint32_t *dfs = lookup_deep_deps(i);
+ const uint32_t *dfs = x86_cpuid_lookup_deep_deps(i);
unsigned int j;
ASSERT(dfs); /* deep_features[] should guarentee this. */
@@ -544,36 +544,6 @@ bool recheck_cpu_features(unsigned int cpu)
return okay;
}
-const uint32_t *lookup_deep_deps(uint32_t feature)
-{
- static const struct {
- uint32_t feature;
- uint32_t fs[FSCAPINTS];
- } deep_deps[] = INIT_DEEP_DEPS;
- unsigned int start = 0, end = ARRAY_SIZE(deep_deps);
-
- BUILD_BUG_ON(ARRAY_SIZE(deep_deps) != NR_DEEP_DEPS);
-
- /* Fast early exit. */
- if ( !test_bit(feature, deep_features) )
- return NULL;
-
- /* deep_deps[] is sorted. Perform a binary search. */
- while ( start < end )
- {
- unsigned int mid = start + ((end - start) / 2);
-
- if ( deep_deps[mid].feature > feature )
- end = mid;
- else if ( deep_deps[mid].feature < feature )
- start = mid + 1;
- else
- return deep_deps[mid].fs;
- }
-
- return NULL;
-}
-
void recalculate_cpuid_policy(struct domain *d)
{
struct cpuid_policy *p = d->arch.cpuid;
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b3e0b0e..a843394 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -80,3 +80,4 @@ subdir-$(CONFIG_UBSAN) += ubsan
subdir-$(CONFIG_NEEDS_LIBELF) += libelf
subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+subdir-$(CONFIG_X86) += libx86
diff --git a/xen/common/libx86/Makefile b/xen/common/libx86/Makefile
new file mode 100644
index 0000000..3fb2e0b
--- /dev/null
+++ b/xen/common/libx86/Makefile
@@ -0,0 +1 @@
+obj-y += cpuid.o
diff --git a/xen/common/libx86/cpuid.c b/xen/common/libx86/cpuid.c
new file mode 100644
index 0000000..0f497d4
--- /dev/null
+++ b/xen/common/libx86/cpuid.c
@@ -0,0 +1,44 @@
+#include "private.h"
+
+#include <xen/libx86/cpuid.h>
+
+const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
+{
+ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
+ static const struct {
+ uint32_t feature;
+ uint32_t fs[FEATURESET_NR_ENTRIES];
+ } deep_deps[] = INIT_DEEP_DEPS;
+ unsigned int start = 0, end = ARRAY_SIZE(deep_deps);
+
+ BUILD_BUG_ON(ARRAY_SIZE(deep_deps) != NR_DEEP_DEPS);
+
+ /* Fast early exit. */
+ if ( !test_bit(feature, deep_features) )
+ return NULL;
+
+ /* deep_deps[] is sorted. Perform a binary search. */
+ while ( start < end )
+ {
+ unsigned int mid = start + ((end - start) / 2);
+
+ if ( deep_deps[mid].feature > feature )
+ end = mid;
+ else if ( deep_deps[mid].feature < feature )
+ start = mid + 1;
+ else
+ return deep_deps[mid].fs;
+ }
+
+ return NULL;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
new file mode 100644
index 0000000..c234b85
--- /dev/null
+++ b/xen/common/libx86/private.h
@@ -0,0 +1,38 @@
+#ifndef XEN_LIBX86_PRIVATE_H
+#define XEN_LIBX86_PRIVATE_H
+
+#ifdef __XEN__
+
+#include <xen/bitops.h>
+#include <xen/kernel.h>
+#include <xen/lib.h>
+#include <xen/types.h>
+
+#else
+
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stddef.h>
+
+#include <xen-tools/libs.h>
+
+static inline bool test_bit(unsigned int bit, const void *vaddr)
+{
+ const char *addr = vaddr;
+
+ return addr[bit / 8] & (1u << (bit % 8));
+}
+
+#endif /* __XEN__ */
+
+#endif /* XEN_LIBX86_PRIVATE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 7a3f2f4..ea79445 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -17,8 +17,6 @@ extern const uint32_t special_features[FSCAPINTS];
void init_guest_cpuid(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).
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
index 69bd8a9..233fa13 100644
--- a/xen/include/xen/libx86/cpuid.h
+++ b/xen/include/xen/libx86/cpuid.h
@@ -228,6 +228,8 @@ static inline void cpuid_featureset_to_policy(
p->feat._7d0 = fs[FEATURESET_7d0];
}
+const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
+
#endif /* !XEN_LIBX86_CPUID_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (4 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 05/13] libx86: introduce a libx86 shared library Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:41 ` Roger Pau Monné
2018-07-16 10:19 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects Andrew Cooper
` (7 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Sergey Dyasli, Ian Jackson, Jan Beulich,
Roger Pau Monné
To facilitate the shared Xen and toolstack code in libx86, struct msr_policy
needs to be available in the same way as struct cpuid_policy.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Rebase over the msr_{domain,vcpu}_policy rename
* Don't move vcpu_msrs into libx86
---
tools/libxc/xc_cpuid_x86.c | 1 +
xen/include/asm-x86/msr.h | 23 +++--------------------
xen/include/xen/libx86/msr.h | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 39 insertions(+), 20 deletions(-)
create mode 100644 xen/include/xen/libx86/msr.h
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 3c214c8..cc7300c 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -33,6 +33,7 @@ enum {
};
#include <xen/libx86/cpuid.h>
+#include <xen/libx86/msr.h>
#define bitmaskof(idx) (1u << ((idx) & 31))
#define featureword_of(idx) ((idx) >> 5)
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 9b4e4e0..e6f6f11 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -8,6 +8,9 @@
#include <xen/types.h>
#include <xen/percpu.h>
#include <xen/errno.h>
+
+#include <xen/libx86/msr.h>
+
#include <asm/asm_defns.h>
#include <asm/cpufeature.h>
@@ -257,26 +260,6 @@ static inline void wrmsr_tsc_aux(uint32_t val)
}
}
-/* MSR policy object for shared per-domain MSRs */
-struct msr_policy
-{
- /*
- * 0x000000ce - MSR_INTEL_PLATFORM_INFO
- *
- * This MSR is non-architectural, but for simplicy we allow it to be read
- * unconditionally. CPUID Faulting support can be fully emulated for HVM
- * guests so can be offered unconditionally, while support for PV guests
- * is dependent on real hardware support.
- */
- union {
- uint32_t raw;
- struct {
- uint32_t :31;
- bool cpuid_faulting:1;
- };
- } plaform_info;
-};
-
extern struct msr_policy raw_msr_policy,
host_msr_policy,
hvm_max_msr_policy,
diff --git a/xen/include/xen/libx86/msr.h b/xen/include/xen/libx86/msr.h
new file mode 100644
index 0000000..b8b1751
--- /dev/null
+++ b/xen/include/xen/libx86/msr.h
@@ -0,0 +1,35 @@
+/* Common data structures and functions consumed by hypervisor and toolstack */
+#ifndef XEN_LIBX86_MSR_H
+#define XEN_LIBX86_MSR_H
+
+/* MSR policy object for shared per-domain MSRs */
+struct msr_policy
+{
+ /*
+ * 0x000000ce - MSR_INTEL_PLATFORM_INFO
+ *
+ * This MSR is non-architectural, but for simplicy we allow it to be read
+ * unconditionally. CPUID Faulting support can be fully emulated for HVM
+ * guests so can be offered unconditionally, while support for PV guests
+ * is dependent on real hardware support.
+ */
+ union {
+ uint32_t raw;
+ struct {
+ uint32_t :31;
+ bool cpuid_faulting:1;
+ };
+ } plaform_info;
+};
+
+#endif /* !XEN_LIBX86_MSR_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (5 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:18 ` Wei Liu
2018-07-16 10:45 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects Andrew Cooper
` (6 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
The serialised form is made up of the leaf, subleaf and data tuple. As this
is the architectural form, it is expected not to change going forwards.
The serialisation of the Xen/Viridian leaves isn't fully implemented yet. It
is just enough to be bug-compatible with the current DOMCTL_set_cpuid
behaviour, but needs further hypervisor work before the toolstack can sensibly
control these values.
x86_cpuid_copy_to_buffer() is implemented using Xen's regular copy_to_guest
primitives, with an API-compatible memcpy() is used for the libxc half of the
build.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Move MIN() into xen-tools/libs.h.
* Leave a TODO for the Xen/Viridian leaf infrastructure, and update the
commit message.
* Rewrite copy_to_buffer_offset() to avoid multiple evaluation of its
parameters.
* Change to an array typedef for constness reasons
---
tools/include/xen-tools/libs.h | 4 ++
tools/xenstore/xenstored_core.h | 2 -
xen/common/libx86/cpuid.c | 90 +++++++++++++++++++++++++++++++++++++++
xen/common/libx86/private.h | 18 ++++++++
xen/include/public/arch-x86/xen.h | 11 +++++
xen/include/xen/libx86/cpuid.h | 30 +++++++++++++
6 files changed, 153 insertions(+), 2 deletions(-)
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index f78a3b5..927e057 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -17,4 +17,8 @@
#define MAX(x, y) ((x) > (y) ? (x) : (y))
#endif
+#ifndef MIN
+#define MIN(x, y) ((x) < (y) ? (x) : (y))
+#endif
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3d7eb91..3d27feb 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -37,8 +37,6 @@
/* DEFAULT_BUFFER_SIZE should be large enough for each errno string. */
#define DEFAULT_BUFFER_SIZE 16
-#define MIN(a, b) (((a) < (b))? (a) : (b))
-
typedef int32_t wrl_creditt;
#define WRL_CREDIT_MAX (1000*1000*1000)
/* ^ satisfies non-overflow condition for wrl_xfer_credit */
diff --git a/xen/common/libx86/cpuid.c b/xen/common/libx86/cpuid.c
index 0f497d4..cf7dbd3 100644
--- a/xen/common/libx86/cpuid.c
+++ b/xen/common/libx86/cpuid.c
@@ -34,6 +34,96 @@ const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
}
/*
+ * Copy a single cpuid_leaf into a provided xen_cpuid_leaf_t buffer,
+ * performing boundary checking against the buffer size.
+ */
+static int copy_leaf_to_buffer(uint32_t leaf, uint32_t subleaf,
+ const struct cpuid_leaf *data,
+ cpuid_leaf_buffer_t leaves,
+ uint32_t *curr_entry, const uint32_t nr_entries)
+{
+ const xen_cpuid_leaf_t val = {
+ leaf, subleaf, data->a, data->b, data->c, data->d,
+ };
+
+ if ( *curr_entry == nr_entries )
+ return -ENOBUFS;
+
+ if ( copy_to_buffer_offset(leaves, *curr_entry, &val, 1) )
+ return -EFAULT;
+
+ ++*curr_entry;
+
+ return 0;
+}
+
+int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
+ cpuid_leaf_buffer_t leaves,
+ uint32_t *nr_entries_p)
+{
+ const uint32_t nr_entries = *nr_entries_p;
+ uint32_t curr_entry = 0, leaf, subleaf;
+
+#define COPY_LEAF(l, s, data) \
+ ({ int ret; \
+ if ( (ret = copy_leaf_to_buffer( \
+ l, s, data, leaves, &curr_entry, nr_entries)) ) \
+ return ret; \
+ })
+
+ /* Basic leaves. */
+ for ( leaf = 0; leaf <= MIN(p->basic.max_leaf,
+ ARRAY_SIZE(p->basic.raw) - 1); ++leaf )
+ {
+ switch ( leaf )
+ {
+ case 0x4:
+ for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); ++subleaf )
+ COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
+ break;
+
+ case 0x7:
+ for ( subleaf = 0;
+ subleaf <= MIN(p->feat.max_subleaf,
+ ARRAY_SIZE(p->feat.raw) - 1); ++subleaf )
+ COPY_LEAF(leaf, subleaf, &p->feat.raw[subleaf]);
+ break;
+
+ case 0xb:
+ for ( subleaf = 0; subleaf < ARRAY_SIZE(p->topo.raw); ++subleaf )
+ COPY_LEAF(leaf, subleaf, &p->topo.raw[subleaf]);
+ break;
+
+ case 0xd:
+ for ( subleaf = 0; subleaf < ARRAY_SIZE(p->xstate.raw); ++subleaf )
+ COPY_LEAF(leaf, subleaf, &p->xstate.raw[subleaf]);
+ break;
+
+ default:
+ COPY_LEAF(leaf, XEN_CPUID_NO_SUBLEAF, &p->basic.raw[leaf]);
+ break;
+ }
+ }
+
+ /* TODO: Port Xen and Viridian leaves to the new CPUID infrastructure. */
+ COPY_LEAF(0x40000000, XEN_CPUID_NO_SUBLEAF,
+ &(struct cpuid_leaf){ p->hv_limit });
+ COPY_LEAF(0x40000100, XEN_CPUID_NO_SUBLEAF,
+ &(struct cpuid_leaf){ p->hv2_limit });
+
+ /* Extended leaves. */
+ for ( leaf = 0; leaf <= MIN(p->extd.max_leaf & 0xfffful,
+ ARRAY_SIZE(p->extd.raw) - 1); ++leaf )
+ COPY_LEAF(0x80000000 | leaf, XEN_CPUID_NO_SUBLEAF, &p->extd.raw[leaf]);
+
+#undef COPY_LEAF
+
+ *nr_entries_p = curr_entry;
+
+ return 0;
+}
+
+/*
* Local variables:
* mode: C
* c-file-style: "BSD"
diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
index c234b85..5f59961 100644
--- a/xen/common/libx86/private.h
+++ b/xen/common/libx86/private.h
@@ -8,8 +8,13 @@
#include <xen/lib.h>
#include <xen/types.h>
+#include <asm/guest_access.h>
+
+#define copy_to_buffer_offset copy_to_guest_offset
+
#else
+#include <errno.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stddef.h>
@@ -23,6 +28,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr)
return addr[bit / 8] & (1u << (bit % 8));
}
+/* memcpy(), but with copy_to_guest_offset()'s API. */
+#define copy_to_buffer_offset(dst, index, src, nr) \
+({ \
+ const typeof(*(dst)) *src_ = (src); \
+ typeof(*(dst)) *dst_ = (dst); \
+ typeof(index) index_ = (index); \
+ typeof(nr) nr_ = (nr), i_; \
+ \
+ for ( i_ = 0; i_ < (nr_); i_++ ) \
+ (dst_)[(index_) + i_] = src_[i_]; \
+ 0; \
+})
+
#endif /* __XEN__ */
#endif /* XEN_LIBX86_PRIVATE_H */
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 69ee4bc..f3bdd83 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -314,6 +314,17 @@ struct xen_arch_domainconfig {
#define XEN_ACPI_GPE0_CPUHP_BIT 2
#endif
+/*
+ * Representations of architectural CPUID information. Used as the
+ * serialised version of Xen's internal representation.
+ */
+typedef struct xen_cpuid_leaf {
+#define XEN_CPUID_NO_SUBLEAF 0xffffffffu
+ uint32_t leaf, subleaf;
+ uint32_t a, b, c, d;
+} xen_cpuid_leaf_t;
+DEFINE_XEN_GUEST_HANDLE(xen_cpuid_leaf_t);
+
#endif /* !__ASSEMBLY__ */
/*
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
index 233fa13..460b102 100644
--- a/xen/include/xen/libx86/cpuid.h
+++ b/xen/include/xen/libx86/cpuid.h
@@ -30,6 +30,19 @@ struct cpuid_leaf
#define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \
CPUID_GUEST_NR_EXTD_AMD)
+/*
+ * Maximum number of leaves a struct cpuid_policy turns into when serialised
+ * for interaction with the toolstack. (Sum of all leaves in each union, less
+ * the entries in basic which sub-unions hang off of.)
+ */
+#define CPUID_MAX_SERIALISED_LEAVES \
+ (CPUID_GUEST_NR_BASIC + \
+ CPUID_GUEST_NR_FEAT - !!CPUID_GUEST_NR_FEAT + \
+ CPUID_GUEST_NR_CACHE - !!CPUID_GUEST_NR_CACHE + \
+ CPUID_GUEST_NR_TOPO - !!CPUID_GUEST_NR_TOPO + \
+ CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE + \
+ CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )
+
struct cpuid_policy
{
#define DECL_BITFIELD(word) _DECL_BITFIELD(FEATURESET_ ## word)
@@ -230,6 +243,23 @@ static inline void cpuid_featureset_to_policy(
const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
+#ifdef __XEN__
+#include <public/arch-x86/xen.h>
+typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
+#else
+#include <xen/arch-x86/xen.h>
+typedef xen_cpuid_leaf_t cpuid_leaf_buffer_t[];
+#endif
+
+/*
+ * Serialise a cpuid_policy object into an array. Writes at most
+ * CPUID_MAX_SERIALISED_LEAVES. On success, nr_entries_p is updated with the
+ * actual number of leaves written.
+ */
+int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
+ cpuid_leaf_buffer_t leaves,
+ uint32_t *nr_entries_p);
+
#endif /* !XEN_LIBX86_CPUID_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (6 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:24 ` Wei Liu
2018-07-16 10:47 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper
` (5 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
From: Roger Pau Monné <roger.pau@citrix.com>
As with CPUID, an architectural form is used for representing the MSR data.
It is expected not to change moving forwards, but does have a 32 bit field
(currently reserved) which can be used compatibly if needs be.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Rebase over the msr_{domain,vcpu}_policy rename
* Only serialise msr_policy
* Change to an array typedef for constness reasons
---
tools/libxc/Makefile | 2 +-
xen/common/libx86/Makefile | 1 +
xen/common/libx86/msr.c | 56 +++++++++++++++++++++++++++++++++++++++
xen/common/libx86/private.h | 3 +++
xen/include/public/arch-x86/xen.h | 9 ++++++-
xen/include/xen/libx86/msr.h | 20 ++++++++++++++
6 files changed, 89 insertions(+), 2 deletions(-)
create mode 100644 xen/common/libx86/msr.c
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index cd4225c..904e026 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -83,7 +83,7 @@ $(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
ifeq ($(CONFIG_X86),y) # Add libx86 to the build
vpath %.c ../../xen/common/libx86
-GUEST_SRCS-y += cpuid.c
+GUEST_SRCS-y += cpuid.c msr.c
endif
# new domain builder
diff --git a/xen/common/libx86/Makefile b/xen/common/libx86/Makefile
index 3fb2e0b..2f9691e 100644
--- a/xen/common/libx86/Makefile
+++ b/xen/common/libx86/Makefile
@@ -1 +1,2 @@
obj-y += cpuid.o
+obj-y += msr.o
diff --git a/xen/common/libx86/msr.c b/xen/common/libx86/msr.c
new file mode 100644
index 0000000..71c8e9a
--- /dev/null
+++ b/xen/common/libx86/msr.c
@@ -0,0 +1,56 @@
+#include "private.h"
+
+#include <xen/libx86/msr.h>
+
+/*
+ * Copy a single MSR into the provided msr_entry_buffer_t buffer, performing a
+ * boundary check against the buffer size.
+ */
+static int copy_msr_to_buffer(uint32_t idx, uint64_t val,
+ msr_entry_buffer_t msrs,
+ uint32_t *curr_entry, const uint32_t nr_entries)
+{
+ const xen_msr_entry_t ent = { .idx = idx, .val = val };
+
+ if ( *curr_entry == nr_entries )
+ return -ENOBUFS;
+
+ if ( copy_to_buffer_offset(msrs, *curr_entry, &ent, 1) )
+ return -EFAULT;
+
+ ++*curr_entry;
+
+ return 0;
+}
+
+int x86_msr_copy_to_buffer(const struct msr_policy *p,
+ msr_entry_buffer_t msrs, uint32_t *nr_entries_p)
+{
+ const uint32_t nr_entries = *nr_entries_p;
+ uint32_t curr_entry = 0;
+
+#define COPY_MSR(idx, val) \
+ ({ int ret; \
+ if ( (ret = copy_msr_to_buffer( \
+ idx, val, msrs, &curr_entry, nr_entries)) ) \
+ return ret; \
+ })
+
+ COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->plaform_info.raw);
+
+#undef COPY_MSR
+
+ *nr_entries_p = curr_entry;
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
index 5f59961..e874fb6 100644
--- a/xen/common/libx86/private.h
+++ b/xen/common/libx86/private.h
@@ -9,6 +9,7 @@
#include <xen/types.h>
#include <asm/guest_access.h>
+#include <asm/msr-index.h>
#define copy_to_buffer_offset copy_to_guest_offset
@@ -19,6 +20,8 @@
#include <stdbool.h>
#include <stddef.h>
+#include <xen/asm/msr-index.h>
+
#include <xen-tools/libs.h>
static inline bool test_bit(unsigned int bit, const void *vaddr)
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index f3bdd83..55a149f 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -315,7 +315,7 @@ struct xen_arch_domainconfig {
#endif
/*
- * Representations of architectural CPUID information. Used as the
+ * Representations of architectural CPUID and MSR information. Used as the
* serialised version of Xen's internal representation.
*/
typedef struct xen_cpuid_leaf {
@@ -325,6 +325,13 @@ typedef struct xen_cpuid_leaf {
} xen_cpuid_leaf_t;
DEFINE_XEN_GUEST_HANDLE(xen_cpuid_leaf_t);
+typedef struct xen_msr_entry {
+ uint32_t idx;
+ uint32_t flags; /* Reserved MBZ. */
+ uint64_t val;
+} xen_msr_entry_t;
+DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
+
#endif /* !__ASSEMBLY__ */
/*
diff --git a/xen/include/xen/libx86/msr.h b/xen/include/xen/libx86/msr.h
index b8b1751..2e4acd4 100644
--- a/xen/include/xen/libx86/msr.h
+++ b/xen/include/xen/libx86/msr.h
@@ -2,6 +2,9 @@
#ifndef XEN_LIBX86_MSR_H
#define XEN_LIBX86_MSR_H
+/* Maximum number of MSRs written when serialising msr_domain_policy. */
+#define MSR_MAX_SERIALISED_ENTRIES 1
+
/* MSR policy object for shared per-domain MSRs */
struct msr_policy
{
@@ -22,6 +25,23 @@ struct msr_policy
} plaform_info;
};
+#ifdef __XEN__
+#include <public/arch-x86/xen.h>
+typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
+#else
+#include <xen/arch-x86/xen.h>
+typedef xen_msr_entry_t msr_entry_buffer_t[];
+#endif
+
+/*
+ * Serialise a msr_policy object into an array. Writes at most
+ * MSR_MAX_SERIALISED_ENTRIES. Returns -ENOBUFS if the buffer array is too
+ * short. On success, nr_entries_p is updated with the actual number of
+ * leaves written.
+ */
+int x86_msr_copy_to_buffer(const struct msr_policy *p,
+ msr_entry_buffer_t msrs, uint32_t *nr_entries_p);
+
#endif /* !XEN_LIBX86_MSR_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (7 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:57 ` Wei Liu
2018-07-13 20:03 ` [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper
` (4 subsequent siblings)
13 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
As with the serialise side, Xen's copy_from_guest API is used, with a
compatibility wrapper for the userspace build.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Rewrite copy_from_buffer_offset() to avoid multiple evaluation of its
arguments.
* Expand boundary justifications.
---
xen/common/libx86/cpuid.c | 94 ++++++++++++++++++++++++++++++++++++++++++
xen/common/libx86/private.h | 14 +++++++
xen/include/xen/libx86/cpuid.h | 11 +++++
3 files changed, 119 insertions(+)
diff --git a/xen/common/libx86/cpuid.c b/xen/common/libx86/cpuid.c
index cf7dbd3..73cd574 100644
--- a/xen/common/libx86/cpuid.c
+++ b/xen/common/libx86/cpuid.c
@@ -123,6 +123,100 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
return 0;
}
+int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
+ const cpuid_leaf_buffer_t leaves,
+ uint32_t nr_leaves, uint32_t *err_leaf,
+ uint32_t *err_subleaf)
+{
+ unsigned int i;
+ xen_cpuid_leaf_t data;
+ struct cpuid_leaf *l = (void *)&data.a;
+
+ /*
+ * A well formed caller is expected pass an array with leaves in order,
+ * and without any repetitions. However, due to per-vendor differences,
+ * and in the case of upgrade or levelled scenarios, we typically expect
+ * fewer than MAX leaves to be passed.
+ *
+ * Detecting repeated entries is prohibitively complicated, so we don't
+ * bother. That said, one way or another if more than MAX leaves are
+ * passed, something is wrong.
+ */
+ if ( nr_leaves > CPUID_MAX_SERIALISED_LEAVES )
+ return -E2BIG;
+
+ for ( i = 0; i < nr_leaves; ++i )
+ {
+ if ( copy_from_buffer_offset(&data, leaves, i, 1) )
+ return -EFAULT;
+
+ switch ( data.leaf )
+ {
+ case 0 ... ARRAY_SIZE(p->basic.raw) - 1:
+ switch ( data.leaf )
+ {
+ case 0x4:
+ if ( data.subleaf >= ARRAY_SIZE(p->cache.raw) )
+ goto out_of_range;
+
+ p->cache.raw[data.subleaf] = *l;
+ break;
+
+ case 0x7:
+ if ( data.subleaf >= ARRAY_SIZE(p->feat.raw) )
+ goto out_of_range;
+
+ p->feat.raw[data.subleaf] = *l;
+ break;
+
+ case 0xb:
+ if ( data.subleaf >= ARRAY_SIZE(p->topo.raw) )
+ goto out_of_range;
+
+ p->topo.raw[data.subleaf] = *l;
+ break;
+
+ case 0xd:
+ if ( data.subleaf >= ARRAY_SIZE(p->xstate.raw) )
+ goto out_of_range;
+
+ p->xstate.raw[data.leaf] = *l;
+ break;
+
+ default:
+ p->basic.raw[data.leaf] = *l;
+ break;
+ }
+ break;
+
+ case 0x40000000:
+ p->hv_limit = l->a;
+ break;
+
+ case 0x40000100:
+ p->hv2_limit = l->a;
+ break;
+
+ case 0x80000000 ... 0x80000000 + ARRAY_SIZE(p->extd.raw) - 1:
+ p->extd.raw[data.leaf & 0xffff] = *l;
+ break;
+
+ default:
+ goto out_of_range;
+ }
+ }
+
+ return 0;
+
+ out_of_range:
+ if ( err_leaf )
+ *err_leaf = data.leaf;
+ if ( err_subleaf )
+ *err_subleaf = data.subleaf;
+
+ return -ERANGE;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
index e874fb6..dc451d0 100644
--- a/xen/common/libx86/private.h
+++ b/xen/common/libx86/private.h
@@ -12,6 +12,7 @@
#include <asm/msr-index.h>
#define copy_to_buffer_offset copy_to_guest_offset
+#define copy_from_buffer_offset copy_from_guest_offset
#else
@@ -44,6 +45,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr)
0; \
})
+/* memcpy(), but with copy_from_guest_offset()'s API. */
+#define copy_from_buffer_offset(dst, src, index, nr) \
+({ \
+ const typeof(*(dst)) *src_ = (src); \
+ typeof(*(dst)) *dst_ = (dst); \
+ typeof(index) index_ = (index); \
+ typeof(nr) nr_ = (nr), i_; \
+ \
+ for ( i_ = 0; i_ < nr_; i_++ ) \
+ dst_[i_] = src_[index_ + i_]; \
+ 0; \
+})
+
#endif /* __XEN__ */
#endif /* XEN_LIBX86_PRIVATE_H */
diff --git a/xen/include/xen/libx86/cpuid.h b/xen/include/xen/libx86/cpuid.h
index 460b102..c4c4bd4 100644
--- a/xen/include/xen/libx86/cpuid.h
+++ b/xen/include/xen/libx86/cpuid.h
@@ -260,6 +260,17 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
cpuid_leaf_buffer_t leaves,
uint32_t *nr_entries_p);
+/*
+ * Copy CPUID data from a buffer, filling in a cpuid_policy object. Performs
+ * boudary checking of the incomming leaves before filling the appropriate
+ * policy unions, but no content validation is performed. On an error, the
+ * optional err_* pointers may help identify where the issue lies.
+ */
+int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
+ const cpuid_leaf_buffer_t leaves,
+ uint32_t nr_leaves, uint32_t *err_leaf,
+ uint32_t *err_subleaf);
+
#endif /* !XEN_LIBX86_CPUID_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (8 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 10:07 ` Wei Liu
2018-07-16 11:36 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
` (3 subsequent siblings)
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
From: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
v2:
* Rebase over the msr_{domain,vcpu}_policy rename
* Only deserialse msr_policy
* Expand boundary justifications
---
xen/common/libx86/msr.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
xen/include/xen/libx86/msr.h | 10 +++++++++
2 files changed, 61 insertions(+)
diff --git a/xen/common/libx86/msr.c b/xen/common/libx86/msr.c
index 71c8e9a..0912ace 100644
--- a/xen/common/libx86/msr.c
+++ b/xen/common/libx86/msr.c
@@ -45,6 +45,57 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
return 0;
}
+int x86_msr_copy_from_buffer(struct msr_policy *p,
+ const msr_entry_buffer_t msrs, uint32_t nr_msrs,
+ uint32_t *err_msr)
+{
+ unsigned int i;
+ xen_msr_entry_t data;
+
+ /*
+ * A well formed caller is expected pass an array with entries in order,
+ * and without any repetitions. However, due to per-vendor differences,
+ * and in the case of upgrade or levelled scenarios, we typically expect
+ * fewer than MAX entries to be passed.
+ *
+ * Detecting repeated entries is prohibitively complicated, so we don't
+ * bother. That said, one way or another if more than MAX entries are
+ * passed, something is wrong.
+ */
+ if ( nr_msrs > MSR_MAX_SERIALISED_ENTRIES )
+ return -E2BIG;
+
+ for ( i = 0; i < nr_msrs; i++ )
+ {
+ if ( copy_from_buffer_offset(&data, msrs, i, 1) )
+ return -EFAULT;
+
+ if ( data.flags ) /* .flags MBZ */
+ goto err;
+
+ switch ( data.idx )
+ {
+ case MSR_INTEL_PLATFORM_INFO:
+ if ( data.val > ~0u )
+ goto err;
+
+ p->plaform_info.raw = data.val;
+ break;
+
+ default:
+ goto err;
+ }
+ }
+
+ return 0;
+
+ err:
+ if ( err_msr )
+ *err_msr = data.idx;
+
+ return -EINVAL;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/libx86/msr.h b/xen/include/xen/libx86/msr.h
index 2e4acd4..d71ad78 100644
--- a/xen/include/xen/libx86/msr.h
+++ b/xen/include/xen/libx86/msr.h
@@ -42,6 +42,16 @@ typedef xen_msr_entry_t msr_entry_buffer_t[];
int x86_msr_copy_to_buffer(const struct msr_policy *p,
msr_entry_buffer_t msrs, uint32_t *nr_entries_p);
+/*
+ * Copy MSR data from a buffer, filling an msr_policy object. MSR indicies
+ * are checked for being in range, but no content validation is performed for
+ * in-range MSRs. On an error, the optional err_* pointer may help identify
+ * where the issue lies.
+ */
+int x86_msr_copy_from_buffer(struct msr_policy *dp,
+ const msr_entry_buffer_t msrs, uint32_t nr_msrs,
+ uint32_t *err_msr);
+
#endif /* !XEN_LIBX86_MSR_H */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (9 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 9:55 ` Roger Pau Monné
` (2 more replies)
2018-07-13 20:03 ` [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy Andrew Cooper
` (2 subsequent siblings)
13 siblings, 3 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
This is prep work for the following patch - please refer to it as well.
When auditing and manipulating policies, it is necessary to do so with a
complete set of policies, due to the interdependences of the contents. A
containing structure like this will allow for clearer APIs and code.
As a first user, this structure is convenient for the mapping used by
XEN_SYSCTL_get_cpu_policy (implemented in the next patch), and for auditing
(later when XEN_DOMCTL_set_cpu_policy is implemented).
At this point, the distinction between *_max and *_default is introduced into
the ABI. For now, *_default is mapped to *_max, but future development work
will result in *_default being a logical subset of *_max.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
v2:
* Drop __read_mostly from MSR declarations. Fixes clang build.
* s/policy_group/cpu_policy/g s/cpumsr_policy/cpu_policy/g
* Rebase over the msr_{domain,vcpu}_policy rename
* Don't include vcpu_msrs in the cpu_policy
---
xen/arch/x86/sysctl.c | 27 +++++++++++++++++++++++++++
xen/include/asm-x86/cpuid.h | 3 +++
xen/include/public/sysctl.h | 20 ++++++++++++++++++++
xen/include/xen/libx86/cpu-policy.h | 24 ++++++++++++++++++++++++
4 files changed, 74 insertions(+)
create mode 100644 xen/include/xen/libx86/cpu-policy.h
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 4d372db..9986393 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -31,6 +31,33 @@
#include <asm/psr.h>
#include <asm/cpuid.h>
+const struct cpu_policy system_policies[] = {
+ [ XEN_SYSCTL_cpu_policy_raw ] = {
+ &raw_cpuid_policy,
+ &raw_msr_policy,
+ },
+ [ XEN_SYSCTL_cpu_policy_host ] = {
+ &host_cpuid_policy,
+ &host_msr_policy,
+ },
+ [ XEN_SYSCTL_cpu_policy_pv_max ] = {
+ &pv_max_cpuid_policy,
+ &pv_max_msr_policy,
+ },
+ [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
+ &hvm_max_cpuid_policy,
+ &hvm_max_msr_policy,
+ },
+ [ XEN_SYSCTL_cpu_policy_pv_default ] = {
+ &pv_max_cpuid_policy,
+ &pv_max_msr_policy,
+ },
+ [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
+ &hvm_max_cpuid_policy,
+ &hvm_max_msr_policy,
+ },
+};
+
struct l3_cache_info {
int ret;
unsigned long size;
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index ea79445..cb51ccb 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -8,6 +8,7 @@
#include <xen/types.h>
#include <xen/kernel.h>
+#include <xen/libx86/cpu-policy.h>
#include <xen/libx86/cpuid.h>
#include <public/sysctl.h>
@@ -50,6 +51,8 @@ extern struct cpuidmasks cpuidmask_defaults;
extern struct cpuid_policy raw_cpuid_policy, host_cpuid_policy,
pv_max_cpuid_policy, hvm_max_cpuid_policy;
+extern const struct cpu_policy system_policies[];
+
/* Check that all previously present features are still available. */
bool recheck_cpu_features(unsigned int cpu);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 839c1b9..7ec2dd9 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1063,6 +1063,26 @@ struct xen_sysctl_set_parameter {
uint16_t pad[3]; /* IN: MUST be zero. */
};
+#if defined(__i386__) || defined(__x86_64__)
+/*
+ * XEN_SYSCTL_get_cpu_policy (x86 specific)
+ *
+ * Return information about CPUID and MSR policies available on this host.
+ * - Raw: The real H/W values.
+ * - Host: The values Xen is using, (after command line overrides, etc).
+ * - Max_*: Maximum set of features a PV or HVM guest can use. Includes
+ * experimental features outside of security support.
+ * - Default_*: Default set of features a PV or HVM guest can use. This is
+ * the security supported set.
+ */
+#define XEN_SYSCTL_cpu_policy_raw 0
+#define XEN_SYSCTL_cpu_policy_host 1
+#define XEN_SYSCTL_cpu_policy_pv_max 2
+#define XEN_SYSCTL_cpu_policy_hvm_max 3
+#define XEN_SYSCTL_cpu_policy_pv_default 4
+#define XEN_SYSCTL_cpu_policy_hvm_default 5
+#endif
+
struct xen_sysctl {
uint32_t cmd;
#define XEN_SYSCTL_readconsole 1
diff --git a/xen/include/xen/libx86/cpu-policy.h b/xen/include/xen/libx86/cpu-policy.h
new file mode 100644
index 0000000..ee1c349
--- /dev/null
+++ b/xen/include/xen/libx86/cpu-policy.h
@@ -0,0 +1,24 @@
+/* Common data structures and functions consumed by hypervisor and toolstack */
+#ifndef XEN_LIBX86_POLICIES_H
+#define XEN_LIBX86_POLICIES_H
+
+#include <xen/libx86/cpuid.h>
+#include <xen/libx86/msr.h>
+
+struct cpu_policy
+{
+ struct cpuid_policy *cpuid;
+ struct msr_policy *msr;
+};
+
+#endif /* !XEN_LIBX86_POLICIES_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (10 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 10:16 ` Roger Pau Monné
2018-07-16 11:54 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
2018-07-30 2:46 ` [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Chao Gao
13 siblings, 2 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Daniel De Graaf, Roger Pau Monné
From: Sergey Dyasli <sergey.dyasli@citrix.com>
Provide a SYSCTL for the toolstack to obtain complete system CPUID and MSR
policy information.
For the XSM side of things, this subop is closely related to
{phys,cputopo,numa}info, so shares the physinfo access vector.
Extend the xen-cpuid utility to be able to dump the system policies. An
example output is:
Xen reports there are maximum 113 leaves and 3 MSRs
Raw policy: 93 leaves, 3 MSRs
CPUID:
leaf subleaf -> eax ebx ecx edx
00000000:ffffffff -> 0000000d:756e6547:6c65746e:49656e69
00000001:ffffffff -> 000306c3:00100800:7ffafbff:bfebfbff
00000002:ffffffff -> 76036301:00f0b5ff:00000000:00c10000
00000004:00000000 -> 1c004121:01c0003f:0000003f:00000000
00000004:00000001 -> 1c004122:01c0003f:0000003f:00000000
00000004:00000002 -> 1c004143:01c0003f:000001ff:00000000
00000004:00000003 -> 1c03c163:03c0003f:00001fff:00000006
00000005:ffffffff -> 00000040:00000040:00000003:00042120
00000006:ffffffff -> 00000077:00000002:00000009:00000000
00000007:00000000 -> 00000000:000027ab:00000000:9c000000
0000000a:ffffffff -> 07300403:00000000:00000000:00000603
0000000b:00000000 -> 00000001:00000002:00000100:00000000
0000000b:00000001 -> 00000004:00000008:00000201:00000000
0000000d:00000000 -> 00000007:00000340:00000340:00000000
0000000d:00000001 -> 00000001:00000000:00000000:00000000
0000000d:00000002 -> 00000100:00000240:00000000:00000000
80000000:ffffffff -> 80000008:00000000:00000000:00000000
80000001:ffffffff -> 00000000:00000000:00000021:2c100800
80000002:ffffffff -> 65746e49:2952286c:6f655820:2952286e
80000003:ffffffff -> 55504320:2d334520:30343231:20337620
80000004:ffffffff -> 2e332040:48473034:0000007a:00000000
80000006:ffffffff -> 00000000:00000000:01006040:00000000
80000007:ffffffff -> 00000000:00000000:00000000:00000100
80000008:ffffffff -> 00003027:00000000:00000000:00000000
MSRs:
index -> value
000000ce -> 0000000080000000
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
v2:
* Avoid introducing a Spectre V1 gadget
---
tools/libxc/include/xenctrl.h | 6 +++
tools/libxc/xc_cpuid_x86.c | 59 +++++++++++++++++++++++++
tools/misc/xen-cpuid.c | 87 +++++++++++++++++++++++++++++++++++--
xen/arch/x86/sysctl.c | 71 ++++++++++++++++++++++++++++++
xen/include/public/sysctl.h | 17 ++++++++
xen/xsm/flask/hooks.c | 1 +
xen/xsm/flask/policy/access_vectors | 2 +-
7 files changed, 239 insertions(+), 4 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index dd7d8a9..ee3ab09 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2553,6 +2553,12 @@ int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
uint32_t *nr_features, uint32_t *featureset);
+int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
+ uint32_t *nr_msrs);
+int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
+ uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+ uint32_t *nr_msrs, xen_msr_entry_t *msrs);
+
uint32_t xc_get_cpu_featureset_size(void);
enum xc_static_cpu_featuremask {
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index cc7300c..8fd04ef 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -132,6 +132,65 @@ const uint32_t *xc_get_static_cpu_featuremask(
}
}
+int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
+ uint32_t *nr_msrs)
+{
+ struct xen_sysctl sysctl = {};
+ int ret;
+
+ sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
+
+ ret = do_sysctl(xch, &sysctl);
+
+ if ( !ret )
+ {
+ *nr_leaves = sysctl.u.cpu_policy.nr_leaves;
+ *nr_msrs = sysctl.u.cpu_policy.nr_msrs;
+ }
+
+ return ret;
+}
+
+int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
+ uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+ uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+{
+ struct xen_sysctl sysctl = {};
+ DECLARE_HYPERCALL_BOUNCE(leaves,
+ *nr_leaves * sizeof(*leaves),
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(msrs,
+ *nr_msrs * sizeof(*msrs),
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ int ret;
+
+ if ( xc_hypercall_bounce_pre(xch, leaves) )
+ return -1;
+
+ if ( xc_hypercall_bounce_pre(xch, msrs) )
+ return -1;
+
+ sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
+ sysctl.u.cpu_policy.index = index;
+ sysctl.u.cpu_policy.nr_leaves = *nr_leaves;
+ set_xen_guest_handle(sysctl.u.cpu_policy.cpuid_policy, leaves);
+ sysctl.u.cpu_policy.nr_msrs = *nr_msrs;
+ set_xen_guest_handle(sysctl.u.cpu_policy.msr_policy, msrs);
+
+ ret = do_sysctl(xch, &sysctl);
+
+ xc_hypercall_bounce_post(xch, leaves);
+ xc_hypercall_bounce_post(xch, msrs);
+
+ if ( !ret )
+ {
+ *nr_leaves = sysctl.u.cpu_policy.nr_leaves;
+ *nr_msrs = sysctl.u.cpu_policy.nr_msrs;
+ }
+
+ return ret;
+}
+
struct cpuid_domain_info
{
enum
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 4a4044a..1c14d93 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -277,9 +277,37 @@ static void dump_info(xc_interface *xch, bool detail)
free(featuresets[i].fs);
}
+static void print_policy(const char *name,
+ xen_cpuid_leaf_t *leaves, uint32_t nr_leaves,
+ xen_msr_entry_t *msrs, uint32_t nr_msrs)
+{
+ unsigned int l;
+
+ printf("%s policy: %u leaves, %u MSRs\n", name, nr_leaves, nr_msrs);
+ printf(" CPUID:\n");
+ printf(" %-8s %-8s -> %-8s %-8s %-8s %-8s\n",
+ "leaf", "subleaf", "eax", "ebx", "ecx", "edx");
+ for ( l = 0; l < nr_leaves; ++l )
+ {
+ /* Skip empty leaves. */
+ if ( !leaves[l].a && !leaves[l].b && !leaves[l].c && !leaves[l].d )
+ continue;
+
+ printf(" %08x:%08x -> %08x:%08x:%08x:%08x\n",
+ leaves[l].leaf, leaves[l].subleaf,
+ leaves[l].a, leaves[l].b, leaves[l].c, leaves[l].d);
+ }
+
+ printf(" MSRs:\n");
+ printf(" %-8s -> %-16s\n", "index", "value");
+ for ( l = 0; l < nr_msrs; ++l )
+ printf(" %08x -> %016lx\n",
+ msrs[l].idx, msrs[l].val);
+}
+
int main(int argc, char **argv)
{
- enum { MODE_UNKNOWN, MODE_INFO, MODE_DETAIL, MODE_INTERPRET }
+ enum { MODE_UNKNOWN, MODE_INFO, MODE_DETAIL, MODE_INTERPRET, MODE_POLICY }
mode = MODE_UNKNOWN;
nr_features = xc_get_cpu_featureset_size();
@@ -293,10 +321,11 @@ int main(int argc, char **argv)
{ "info", no_argument, NULL, 'i' },
{ "detail", no_argument, NULL, 'd' },
{ "verbose", no_argument, NULL, 'v' },
+ { "policy", no_argument, NULL, 'p' },
{ NULL, 0, NULL, 0 },
};
- c = getopt_long(argc, argv, "hidv", long_options, &option_index);
+ c = getopt_long(argc, argv, "hidvp", long_options, &option_index);
if ( c == -1 )
break;
@@ -314,6 +343,10 @@ int main(int argc, char **argv)
mode = MODE_INFO;
break;
+ case 'p':
+ mode = MODE_POLICY;
+ break;
+
case 'd':
case 'v':
mode = MODE_DETAIL;
@@ -344,7 +377,55 @@ int main(int argc, char **argv)
mode = MODE_INTERPRET;
}
- if ( mode == MODE_INFO || mode == MODE_DETAIL )
+ if ( mode == MODE_POLICY )
+ {
+ static const char *const sys_policies[] = {
+ [ XEN_SYSCTL_cpu_policy_raw ] = "Raw",
+ [ XEN_SYSCTL_cpu_policy_host ] = "Host",
+ [ XEN_SYSCTL_cpu_policy_pv_max ] = "PV Max",
+ [ XEN_SYSCTL_cpu_policy_hvm_max ] = "HVM Max",
+ [ XEN_SYSCTL_cpu_policy_pv_default ] = "PV Default",
+ [ XEN_SYSCTL_cpu_policy_hvm_default ] = "HVM Default",
+ };
+ xen_cpuid_leaf_t *leaves;
+ xen_msr_entry_t *msrs;
+ uint32_t pol, max_leaves, max_msrs;
+
+ xc_interface *xch = xc_interface_open(0, 0, 0);
+
+ if ( !xch )
+ err(1, "xc_interface_open");
+
+ if ( xc_get_cpu_policy_size(xch, &max_leaves, &max_msrs) )
+ err(1, "xc_get_cpu_policy_size(...)");
+ printf("Xen reports there are maximum %u leaves and %u MSRs\n",
+ max_leaves, max_msrs);
+
+ leaves = calloc(max_leaves, sizeof(xen_cpuid_leaf_t));
+ if ( !leaves )
+ err(1, "calloc(max_leaves)");
+ msrs = calloc(max_msrs, sizeof(xen_msr_entry_t));
+ if ( !msrs )
+ err(1, "calloc(max_msrs)");
+
+ for ( pol = 0; pol < ARRAY_SIZE(sys_policies); ++pol )
+ {
+ uint32_t nr_leaves = max_leaves;
+ uint32_t nr_msrs = max_msrs;
+
+ if ( xc_get_system_cpu_policy(xch, pol, &nr_leaves, leaves,
+ &nr_msrs, msrs) )
+ err(1, "xc_get_system_cpu_policy(, %s,,)",
+ sys_policies[pol]);
+
+ print_policy(sys_policies[pol], leaves, nr_leaves, msrs, nr_msrs);
+ }
+
+ free(leaves);
+ free(msrs);
+ xc_interface_close(xch);
+ }
+ else if ( mode == MODE_INFO || mode == MODE_DETAIL )
{
xc_interface *xch = xc_interface_open(0, 0, 0);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 9986393..29d403c 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -9,6 +9,7 @@
#include <xen/types.h>
#include <xen/lib.h>
#include <xen/mm.h>
+#include <xen/nospec.h>
#include <xen/guest_access.h>
#include <xen/hypercall.h>
#include <public/sysctl.h>
@@ -322,6 +323,76 @@ long arch_do_sysctl(
break;
}
+ case XEN_SYSCTL_get_cpu_policy:
+ {
+ const struct cpu_policy *policy;
+
+ /* Bad policy index? */
+ if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
+ {
+ ret = -EINVAL;
+ break;
+ }
+ policy = &system_policies[
+ array_index_nospec(sysctl->u.cpu_policy.index,
+ ARRAY_SIZE(system_policies))];
+
+ /* Request for maximum number of leaves/MSRs? */
+ if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
+ {
+ sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
+ if ( __copy_field_to_guest(u_sysctl, sysctl,
+ u.cpu_policy.nr_leaves) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+ if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
+ {
+ sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
+ if ( __copy_field_to_guest(u_sysctl, sysctl,
+ u.cpu_policy.nr_msrs) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+
+ /* Serialise the information the caller wants. */
+ if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
+ {
+ if ( (ret = x86_cpuid_copy_to_buffer(
+ policy->cpuid,
+ sysctl->u.cpu_policy.cpuid_policy,
+ &sysctl->u.cpu_policy.nr_leaves)) )
+ break;
+
+ if ( __copy_field_to_guest(u_sysctl, sysctl,
+ u.cpu_policy.nr_leaves) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+ if ( !guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
+ {
+ if ( (ret = x86_msr_copy_to_buffer(
+ policy->msr,
+ sysctl->u.cpu_policy.msr_policy,
+ &sysctl->u.cpu_policy.nr_msrs)) )
+ break;
+
+ if ( __copy_field_to_guest(u_sysctl, sysctl,
+ u.cpu_policy.nr_msrs) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+ break;
+ }
+
default:
ret = -ENOSYS;
break;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 7ec2dd9..64648bd 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1075,12 +1075,25 @@ struct xen_sysctl_set_parameter {
* - Default_*: Default set of features a PV or HVM guest can use. This is
* the security supported set.
*/
+struct xen_sysctl_cpu_policy {
#define XEN_SYSCTL_cpu_policy_raw 0
#define XEN_SYSCTL_cpu_policy_host 1
#define XEN_SYSCTL_cpu_policy_pv_max 2
#define XEN_SYSCTL_cpu_policy_hvm_max 3
#define XEN_SYSCTL_cpu_policy_pv_default 4
#define XEN_SYSCTL_cpu_policy_hvm_default 5
+ uint32_t index; /* IN: Which policy to query? */
+ uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
+ * 'cpuid_policy', or the maximum number of leaves
+ * if the guest handle is NULL. */
+ uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
+ * 'msr_policy', or the maximum number of MSRs if
+ * the guest handle is NULL. */
+ XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
+ XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* OUT: */
+};
+typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#endif
struct xen_sysctl {
@@ -1112,6 +1125,7 @@ struct xen_sysctl {
#define XEN_SYSCTL_get_cpu_featureset 26
#define XEN_SYSCTL_livepatch_op 27
#define XEN_SYSCTL_set_parameter 28
+#define XEN_SYSCTL_get_cpu_policy 29
uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
union {
struct xen_sysctl_readconsole readconsole;
@@ -1141,6 +1155,9 @@ struct xen_sysctl {
struct xen_sysctl_cpu_featureset cpu_featureset;
struct xen_sysctl_livepatch_op livepatch;
struct xen_sysctl_set_parameter set_parameter;
+#if defined(__i386__) || defined(__x86_64__)
+ struct xen_sysctl_cpu_policy cpu_policy;
+#endif
uint8_t pad[128];
} u;
};
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78bc326..f614272 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -801,6 +801,7 @@ static int flask_sysctl(int cmd)
case XEN_SYSCTL_cputopoinfo:
case XEN_SYSCTL_numainfo:
case XEN_SYSCTL_pcitopoinfo:
+ case XEN_SYSCTL_get_cpu_policy:
return domain_has_xen(current->domain, XEN__PHYSINFO);
case XEN_SYSCTL_psr_cmt_op:
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index c5d8548..8c5baff 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -28,7 +28,7 @@ class xen
# XENPF_microcode_update
microcode
# XEN_SYSCTL_physinfo, XEN_SYSCTL_cputopoinfo, XEN_SYSCTL_numainfo
-# XEN_SYSCTL_pcitopoinfo
+# XEN_SYSCTL_pcitopoinfo, XEN_SYSCTL_get_cpu_policy
physinfo
# XENPF_platform_quirk
quirk
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (11 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy Andrew Cooper
@ 2018-07-13 20:03 ` Andrew Cooper
2018-07-16 10:26 ` Roger Pau Monné
` (3 more replies)
2018-07-30 2:46 ` [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Chao Gao
13 siblings, 4 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-13 20:03 UTC (permalink / raw)
To: Xen-devel
Cc: Sergey Dyasli, Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich,
Daniel De Graaf, Roger Pau Monné
From: Sergey Dyasli <sergey.dyasli@citrix.com>
This finally (after literally years of work!) marks the point where the
toolstack can ask the hypervisor for the current CPUID configuration of a
specific domain.
Also extend xen-cpuid's --policy mode to be able to take a domid and dump a
specific domains CPUID and MSR policy.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
tools/libxc/include/xenctrl.h | 3 ++
tools/libxc/xc_cpuid_x86.c | 40 +++++++++++++++++++++++
tools/misc/xen-cpuid.c | 64 +++++++++++++++++++++++++++++++------
xen/arch/x86/domctl.c | 34 ++++++++++++++++++++
xen/include/public/domctl.h | 18 +++++++++++
xen/xsm/flask/hooks.c | 1 +
xen/xsm/flask/policy/access_vectors | 1 +
7 files changed, 152 insertions(+), 9 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index ee3ab09..3f156c1 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2558,6 +2558,9 @@ int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
uint32_t *nr_msrs, xen_msr_entry_t *msrs);
+int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
+ uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+ uint32_t *nr_msrs, xen_msr_entry_t *msrs);
uint32_t xc_get_cpu_featureset_size(void);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 8fd04ef..e4c7a34 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -191,6 +191,46 @@ int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
return ret;
}
+int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
+ uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+ uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+{
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(leaves,
+ *nr_leaves * sizeof(*leaves),
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(msrs,
+ *nr_msrs * sizeof(*msrs),
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ int ret;
+
+ if ( xc_hypercall_bounce_pre(xch, leaves) )
+ return -1;
+
+ if ( xc_hypercall_bounce_pre(xch, msrs) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_get_cpu_policy;
+ domctl.domain = domid;
+ domctl.u.cpu_policy.nr_leaves = *nr_leaves;
+ set_xen_guest_handle(domctl.u.cpu_policy.cpuid_policy, leaves);
+ domctl.u.cpu_policy.nr_msrs = *nr_msrs;
+ set_xen_guest_handle(domctl.u.cpu_policy.msr_policy, msrs);
+
+ ret = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, leaves);
+ xc_hypercall_bounce_post(xch, msrs);
+
+ if ( !ret )
+ {
+ *nr_leaves = domctl.u.cpu_policy.nr_leaves;
+ *nr_msrs = domctl.u.cpu_policy.nr_msrs;
+ }
+
+ return ret;
+}
+
struct cpuid_domain_info
{
enum
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 1c14d93..dd39268 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -3,6 +3,8 @@
#include <err.h>
#include <getopt.h>
#include <string.h>
+#include <errno.h>
+#include <limits.h>
#include <xenctrl.h>
@@ -309,11 +311,13 @@ int main(int argc, char **argv)
{
enum { MODE_UNKNOWN, MODE_INFO, MODE_DETAIL, MODE_INTERPRET, MODE_POLICY }
mode = MODE_UNKNOWN;
+ int domid = -1;
nr_features = xc_get_cpu_featureset_size();
for ( ;; )
{
+ const char *tmp_optarg;
int option_index = 0, c;
static struct option long_options[] =
{
@@ -321,11 +325,11 @@ int main(int argc, char **argv)
{ "info", no_argument, NULL, 'i' },
{ "detail", no_argument, NULL, 'd' },
{ "verbose", no_argument, NULL, 'v' },
- { "policy", no_argument, NULL, 'p' },
+ { "policy", optional_argument, NULL, 'p' },
{ NULL, 0, NULL, 0 },
};
- c = getopt_long(argc, argv, "hidvp", long_options, &option_index);
+ c = getopt_long(argc, argv, "hidvp::", long_options, &option_index);
if ( c == -1 )
break;
@@ -345,6 +349,28 @@ int main(int argc, char **argv)
case 'p':
mode = MODE_POLICY;
+
+ tmp_optarg = optarg;
+
+ /* Make "--policy $DOMID" and "-p $DOMID" work. */
+ if ( !optarg && optind < argc &&
+ argv[optind] != NULL && argv[optind][0] != '\0' &&
+ argv[optind][0] != '-' )
+ tmp_optarg = argv[optind++];
+
+ if ( tmp_optarg )
+ {
+ char *endptr;
+
+ errno = 0;
+ domid = strtol(tmp_optarg, &endptr, 0);
+
+ if ( (errno == ERANGE &&
+ (domid == LONG_MAX || domid == LONG_MIN)) ||
+ (errno != 0 && domid == 0) ||
+ endptr == tmp_optarg )
+ err(1, "strtol(%s,,)", tmp_optarg);
+ }
break;
case 'd':
@@ -398,8 +424,9 @@ int main(int argc, char **argv)
if ( xc_get_cpu_policy_size(xch, &max_leaves, &max_msrs) )
err(1, "xc_get_cpu_policy_size(...)");
- printf("Xen reports there are maximum %u leaves and %u MSRs\n",
- max_leaves, max_msrs);
+ if ( domid == -1 )
+ printf("Xen reports there are maximum %u leaves and %u MSRs\n",
+ max_leaves, max_msrs);
leaves = calloc(max_leaves, sizeof(xen_cpuid_leaf_t));
if ( !leaves )
@@ -408,17 +435,36 @@ int main(int argc, char **argv)
if ( !msrs )
err(1, "calloc(max_msrs)");
- for ( pol = 0; pol < ARRAY_SIZE(sys_policies); ++pol )
+ if ( domid != -1 )
{
+ char name[20];
uint32_t nr_leaves = max_leaves;
uint32_t nr_msrs = max_msrs;
- if ( xc_get_system_cpu_policy(xch, pol, &nr_leaves, leaves,
+ if ( xc_get_domain_cpu_policy(xch, domid, &nr_leaves, leaves,
&nr_msrs, msrs) )
- err(1, "xc_get_system_cpu_policy(, %s,,)",
- sys_policies[pol]);
+ err(1, "xc_get_domain_cpu_policy(, %d, %d,, %d,)",
+ domid, nr_leaves, nr_msrs);
- print_policy(sys_policies[pol], leaves, nr_leaves, msrs, nr_msrs);
+ snprintf(name, sizeof(name), "Domain %d", domid);
+ print_policy(name, leaves, nr_leaves, msrs, nr_msrs);
+ }
+ else
+ {
+ /* Get system policies */
+ for ( pol = 0; pol < ARRAY_SIZE(sys_policies); ++pol )
+ {
+ uint32_t nr_leaves = max_leaves;
+ uint32_t nr_msrs = max_msrs;
+
+ if ( xc_get_system_cpu_policy(xch, pol, &nr_leaves, leaves,
+ &nr_msrs, msrs) )
+ err(1, "xc_get_system_cpu_policy(, %s,,)",
+ sys_policies[pol]);
+
+ print_policy(sys_policies[pol], leaves, nr_leaves,
+ msrs, nr_msrs);
+ }
}
free(leaves);
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b973629..b20aa7a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1523,6 +1523,40 @@ long arch_do_domctl(
recalculate_cpuid_policy(d);
break;
+ case XEN_DOMCTL_get_cpu_policy:
+ if ( !guest_handle_is_null(domctl->u.cpu_policy.cpuid_policy) )
+ {
+ if ( (ret = x86_cpuid_copy_to_buffer(
+ d->arch.cpuid,
+ domctl->u.cpu_policy.cpuid_policy,
+ &domctl->u.cpu_policy.nr_leaves)) )
+ break;
+
+ if ( __copy_field_to_guest(u_domctl, domctl,
+ u.cpu_policy.nr_leaves) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+
+ if ( !guest_handle_is_null(domctl->u.cpu_policy.msr_policy) )
+ {
+ if ( (ret = x86_msr_copy_to_buffer(
+ d->arch.msr,
+ domctl->u.cpu_policy.msr_policy,
+ &domctl->u.cpu_policy.nr_msrs)) )
+ break;
+
+ if ( __copy_field_to_guest(u_domctl, domctl,
+ u.cpu_policy.nr_msrs) )
+ {
+ ret = -EFAULT;
+ break;
+ }
+ }
+ break;
+
default:
ret = iommu_do_domctl(domctl, d, u_domctl);
break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 5c3916c..2114412 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -635,6 +635,22 @@ struct xen_domctl_cpuid {
uint32_t ecx;
uint32_t edx;
};
+
+/*
+ * XEN_SYSCTL_{get,set}_cpu_policy (x86 specific)
+ *
+ * Query or set the CPUID and MSR policies for a specific domain.
+ */
+struct xen_domctl_cpu_policy {
+ uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
+ * 'cpuid_policy'. */
+ uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
+ * 'msr_domain_policy' */
+ XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT: */
+ XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* IN/OUT: */
+};
+typedef struct xen_domctl_cpu_policy xen_domctl_cpu_policy_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_cpu_policy_t);
#endif
/*
@@ -1174,6 +1190,7 @@ struct xen_domctl {
#define XEN_DOMCTL_soft_reset 79
#define XEN_DOMCTL_set_gnttab_limits 80
#define XEN_DOMCTL_vuart_op 81
+#define XEN_DOMCTL_get_cpu_policy 82
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1218,6 +1235,7 @@ struct xen_domctl {
struct xen_domctl_mem_sharing_op mem_sharing_op;
#if defined(__i386__) || defined(__x86_64__)
struct xen_domctl_cpuid cpuid;
+ struct xen_domctl_cpu_policy cpu_policy;
struct xen_domctl_vcpuextstate vcpuextstate;
struct xen_domctl_vcpu_msrs vcpu_msrs;
#endif
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index f614272..7d4fa1c 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -717,6 +717,7 @@ static int flask_domctl(struct domain *d, int cmd)
return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SET_VIRQ_HANDLER);
case XEN_DOMCTL_set_cpuid:
+ case XEN_DOMCTL_get_cpu_policy:
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_CPUID);
case XEN_DOMCTL_gettscinfo:
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 8c5baff..140d3a5 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -213,6 +213,7 @@ class domain2
# target = the new target domain
set_as_target
# XEN_DOMCTL_set_cpuid
+# XEN_DOMCTL_get_cpu_policy
set_cpuid
# XEN_DOMCTL_gettscinfo
gettsc
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 68+ messages in thread
* Re: [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
@ 2018-07-16 7:21 ` Jan Beulich
2018-07-16 9:37 ` Roger Pau Monné
1 sibling, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 7:21 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> More than the bottom two bits are now defined, and the MSR policy work has
> shown that using non-architectural representations turns out to be
> problematic
> for more than just asm code. As the architectural representation is the
> expected default, we don't need to justify why we are using it.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 05/13] libx86: introduce a libx86 shared library
2018-07-13 20:03 ` [PATCH v2 05/13] libx86: introduce a libx86 shared library Andrew Cooper
@ 2018-07-16 9:02 ` Wei Liu
2018-07-16 10:17 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Wei Liu @ 2018-07-16 9:02 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:06PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
>
> Move x86_cpuid_lookup_deep_deps() into the shared library, removing the
> individual copies from the hypervisor and libxc respectively.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-13 20:03 ` [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects Andrew Cooper
@ 2018-07-16 9:18 ` Wei Liu
2018-07-16 9:45 ` Jan Beulich
2018-07-16 10:45 ` Jan Beulich
1 sibling, 1 reply; 68+ messages in thread
From: Wei Liu @ 2018-07-16 9:18 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:08PM +0100, Andrew Cooper wrote:
> +#include <errno.h>
> #include <inttypes.h>
> #include <stdbool.h>
> #include <stddef.h>
> @@ -23,6 +28,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr)
> return addr[bit / 8] & (1u << (bit % 8));
> }
>
> +/* memcpy(), but with copy_to_guest_offset()'s API. */
> +#define copy_to_buffer_offset(dst, index, src, nr) \
> +({ \
> + const typeof(*(dst)) *src_ = (src); \
I think you mean typeof(*(src)) here?
Otherwise:
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h
2018-07-13 20:03 ` [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h Andrew Cooper
@ 2018-07-16 9:23 ` Jan Beulich
2018-07-16 10:22 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 9:23 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> Begin to untangle the header dependency tangle by moving definition of
> struct cpuid_leaf out of x86_emulate.h into the new cpuid.h.
>
> Additionally, plumb the header through to libxc. This is technically a
> redundant include at this point, but it helps build-test the later changes,
> and will be used eventually.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> Note concerning the positioning of libx86. It turns out after trying to move
> it elsewhere that the movement is prohibitive because of the way Xen headers
> are included by the tools.
If there's really something preventing it to be placed better, then I'm
certainly willing to give my ack here, but I'd like to have a more clear
understanding of what issue(s) you are talking about above.
> For now, this is consistent with the other libs, and we can move it in the
> future after some hygene has been applied to the build system.
After all even better to avoid code churn resulting from such extra
movement.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects
2018-07-13 20:03 ` [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects Andrew Cooper
@ 2018-07-16 9:24 ` Wei Liu
2018-07-16 10:47 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Wei Liu @ 2018-07-16 9:24 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:09PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
>
> As with CPUID, an architectural form is used for representing the MSR data.
> It is expected not to change moving forwards, but does have a 32 bit field
> (currently reserved) which can be used compatibly if needs be.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir
2018-07-13 20:03 ` [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir Andrew Cooper
@ 2018-07-16 9:31 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 9:31 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
>
> This avoids all users needing to opencode local generation of the file.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
2018-07-16 7:21 ` Jan Beulich
@ 2018-07-16 9:37 ` Roger Pau Monné
2018-07-16 11:02 ` Andrew Cooper
1 sibling, 1 reply; 68+ messages in thread
From: Roger Pau Monné @ 2018-07-16 9:37 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Wei Liu, Jan Beulich, Xen-devel
On Fri, Jul 13, 2018 at 09:03:02PM +0100, Andrew Cooper wrote:
> More than the bottom two bits are now defined, and the MSR policy work has
> shown that using non-architectural representations turns out to be problematic
> for more than just asm code. As the architectural representation is the
> expected default, we don't need to justify why we are using it.
If that's indeed the architectural representation shouldn't you use a
uint64_t instead?
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-13 20:03 ` [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace Andrew Cooper
@ 2018-07-16 9:38 ` Jan Beulich
2018-07-16 9:51 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 9:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/libs.h
> @@ -13,4 +13,8 @@
> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
> #endif
>
> +#ifndef MAX
> +#define MAX(x, y) ((x) > (y) ? (x) : (y))
> +#endif
I find asymmetries like this odd: There should then also be MIN() imo.
> +static inline void cpuid_featureset_to_policy(
> + const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
> +{
> + p->basic._1d = fs[FEATURESET_1d];
> + p->basic._1c = fs[FEATURESET_1c];
> + p->extd.e1d = fs[FEATURESET_e1d];
> + p->extd.e1c = fs[FEATURESET_e1c];
> + p->xstate.Da1 = fs[FEATURESET_Da1];
> + p->feat._7b0 = fs[FEATURESET_7b0];
> + p->feat._7c0 = fs[FEATURESET_7c0];
> + p->extd.e7d = fs[FEATURESET_e7d];
> + p->extd.e8b = fs[FEATURESET_e8b];
> + p->feat._7d0 = fs[FEATURESET_7d0];
> +}
I realize this is only code movement, but since you didn't answer the
question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
raise it here again: Shouldn't other fields of p be set to zero here?
Irrespective of both items (i.e. with or without them addressed)
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace
2018-07-13 20:03 ` [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace Andrew Cooper
@ 2018-07-16 9:41 ` Roger Pau Monné
2018-07-16 10:19 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Roger Pau Monné @ 2018-07-16 9:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Ian Jackson, Jan Beulich, Xen-devel
On Fri, Jul 13, 2018 at 09:03:07PM +0100, Andrew Cooper wrote:
> To facilitate the shared Xen and toolstack code in libx86, struct msr_policy
> needs to be available in the same way as struct cpuid_policy.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-16 9:18 ` Wei Liu
@ 2018-07-16 9:45 ` Jan Beulich
2018-07-16 10:39 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 9:45 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu
Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 11:18, <wei.liu2@citrix.com> wrote:
> On Fri, Jul 13, 2018 at 09:03:08PM +0100, Andrew Cooper wrote:
>> +#include <errno.h>
>> #include <inttypes.h>
>> #include <stdbool.h>
>> #include <stddef.h>
>> @@ -23,6 +28,19 @@ static inline bool test_bit(unsigned int bit, const void
> *vaddr)
>> return addr[bit / 8] & (1u << (bit % 8));
>> }
>>
>> +/* memcpy(), but with copy_to_guest_offset()'s API. */
>> +#define copy_to_buffer_offset(dst, index, src, nr) \
>> +({ \
>> + const typeof(*(dst)) *src_ = (src); \
>
> I think you mean typeof(*(src)) here?
To follow copy_to_guest_offset()'s model there's more needed here,
I think: dst and src want to point to similar type objects / arrays (i.e.
the macro wants to enforce this).
I also notice that some of the now-local-variables are now pointlessly
parenthesized.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-16 9:38 ` Jan Beulich
@ 2018-07-16 9:51 ` Andrew Cooper
2018-07-16 10:04 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 9:51 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
On 16/07/18 10:38, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/tools/include/xen-tools/libs.h
>> +++ b/tools/include/xen-tools/libs.h
>> @@ -13,4 +13,8 @@
>> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
>> #endif
>>
>> +#ifndef MAX
>> +#define MAX(x, y) ((x) > (y) ? (x) : (y))
>> +#endif
> I find asymmetries like this odd: There should then also be MIN() imo.
Patch 7, where it is first used.
>
>> +static inline void cpuid_featureset_to_policy(
>> + const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
>> +{
>> + p->basic._1d = fs[FEATURESET_1d];
>> + p->basic._1c = fs[FEATURESET_1c];
>> + p->extd.e1d = fs[FEATURESET_e1d];
>> + p->extd.e1c = fs[FEATURESET_e1c];
>> + p->xstate.Da1 = fs[FEATURESET_Da1];
>> + p->feat._7b0 = fs[FEATURESET_7b0];
>> + p->feat._7c0 = fs[FEATURESET_7c0];
>> + p->extd.e7d = fs[FEATURESET_e7d];
>> + p->extd.e8b = fs[FEATURESET_e8b];
>> + p->feat._7d0 = fs[FEATURESET_7d0];
>> +}
> I realize this is only code movement, but since you didn't answer the
> question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
> raise it here again: Shouldn't other fields of p be set to zero here?
No - why should it?
(In fact, it very deliberately does not, and changing this will break
all of the policy derivation logic.)
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
@ 2018-07-16 9:55 ` Roger Pau Monné
2018-07-16 10:32 ` Wei Liu
2018-07-16 12:04 ` Jan Beulich
2 siblings, 0 replies; 68+ messages in thread
From: Roger Pau Monné @ 2018-07-16 9:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Jan Beulich, Xen-devel
On Fri, Jul 13, 2018 at 09:03:12PM +0100, Andrew Cooper wrote:
> This is prep work for the following patch - please refer to it as well.
>
> When auditing and manipulating policies, it is necessary to do so with a
> complete set of policies, due to the interdependences of the contents. A
> containing structure like this will allow for clearer APIs and code.
>
> As a first user, this structure is convenient for the mapping used by
> XEN_SYSCTL_get_cpu_policy (implemented in the next patch), and for auditing
> (later when XEN_DOMCTL_set_cpu_policy is implemented).
>
> At this point, the distinction between *_max and *_default is introduced into
> the ABI. For now, *_default is mapped to *_max, but future development work
> will result in *_default being a logical subset of *_max.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects
2018-07-13 20:03 ` [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper
@ 2018-07-16 9:57 ` Wei Liu
2018-07-17 10:09 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Wei Liu @ 2018-07-16 9:57 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:10PM +0100, Andrew Cooper wrote:
> As with the serialise side, Xen's copy_from_guest API is used, with a
> compatibility wrapper for the userspace build.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> v2:
> * Rewrite copy_from_buffer_offset() to avoid multiple evaluation of its
> arguments.
> * Expand boundary justifications.
> ---
> xen/common/libx86/cpuid.c | 94 ++++++++++++++++++++++++++++++++++++++++++
> xen/common/libx86/private.h | 14 +++++++
> xen/include/xen/libx86/cpuid.h | 11 +++++
> 3 files changed, 119 insertions(+)
>
> diff --git a/xen/common/libx86/cpuid.c b/xen/common/libx86/cpuid.c
> index cf7dbd3..73cd574 100644
> --- a/xen/common/libx86/cpuid.c
> +++ b/xen/common/libx86/cpuid.c
> @@ -123,6 +123,100 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
> return 0;
> }
>
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
> index e874fb6..dc451d0 100644
> --- a/xen/common/libx86/private.h
> +++ b/xen/common/libx86/private.h
> @@ -12,6 +12,7 @@
> #include <asm/msr-index.h>
>
> #define copy_to_buffer_offset copy_to_guest_offset
> +#define copy_from_buffer_offset copy_from_guest_offset
>
> #else
>
> @@ -44,6 +45,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr)
> 0; \
> })
>
> +/* memcpy(), but with copy_from_guest_offset()'s API. */
> +#define copy_from_buffer_offset(dst, src, index, nr) \
> +({ \
> + const typeof(*(dst)) *src_ = (src); \
Same issue as previous patch here. Also enforcing dst_ and src_ are of
the same type would be good.
Otherwise:
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-16 9:51 ` Andrew Cooper
@ 2018-07-16 10:04 ` Jan Beulich
2018-07-16 10:16 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 10:38, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> +static inline void cpuid_featureset_to_policy(
>>> + const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
>>> +{
>>> + p->basic._1d = fs[FEATURESET_1d];
>>> + p->basic._1c = fs[FEATURESET_1c];
>>> + p->extd.e1d = fs[FEATURESET_e1d];
>>> + p->extd.e1c = fs[FEATURESET_e1c];
>>> + p->xstate.Da1 = fs[FEATURESET_Da1];
>>> + p->feat._7b0 = fs[FEATURESET_7b0];
>>> + p->feat._7c0 = fs[FEATURESET_7c0];
>>> + p->extd.e7d = fs[FEATURESET_e7d];
>>> + p->extd.e8b = fs[FEATURESET_e8b];
>>> + p->feat._7d0 = fs[FEATURESET_7d0];
>>> +}
>> I realize this is only code movement, but since you didn't answer the
>> question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
>> raise it here again: Shouldn't other fields of p be set to zero here?
>
> No - why should it?
>
> (In fact, it very deliberately does not, and changing this will break
> all of the policy derivation logic.)
Did you look at the context in which I've raised the question originally?
I did in particular ask about this effectively still being a form of black
listing (I said "white listing" there by mistake), just taking leaf 6 (and
Intel hardware) as example. guest_cpuid() takes whatever is there in
the policy, and update_domain_cpuid_info() does nothing either. It
was my understanding that with the switch to the new model we're
now strictly while listing features. Luwei's patch would extend the
un-audited handing through to RDT and SGX leaves.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-13 20:03 ` [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper
@ 2018-07-16 10:07 ` Wei Liu
2018-07-16 11:36 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Wei Liu @ 2018-07-16 10:07 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Xen-devel, Jan Beulich, Ian Jackson,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:11PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-16 10:04 ` Jan Beulich
@ 2018-07-16 10:16 ` Andrew Cooper
2018-07-16 10:24 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 10:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
On 16/07/18 11:04, Jan Beulich wrote:
>>>> On 16.07.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> On 16/07/18 10:38, Jan Beulich wrote:
>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>>> +static inline void cpuid_featureset_to_policy(
>>>> + const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
>>>> +{
>>>> + p->basic._1d = fs[FEATURESET_1d];
>>>> + p->basic._1c = fs[FEATURESET_1c];
>>>> + p->extd.e1d = fs[FEATURESET_e1d];
>>>> + p->extd.e1c = fs[FEATURESET_e1c];
>>>> + p->xstate.Da1 = fs[FEATURESET_Da1];
>>>> + p->feat._7b0 = fs[FEATURESET_7b0];
>>>> + p->feat._7c0 = fs[FEATURESET_7c0];
>>>> + p->extd.e7d = fs[FEATURESET_e7d];
>>>> + p->extd.e8b = fs[FEATURESET_e8b];
>>>> + p->feat._7d0 = fs[FEATURESET_7d0];
>>>> +}
>>> I realize this is only code movement, but since you didn't answer the
>>> question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
>>> raise it here again: Shouldn't other fields of p be set to zero here?
>> No - why should it?
>>
>> (In fact, it very deliberately does not, and changing this will break
>> all of the policy derivation logic.)
> Did you look at the context in which I've raised the question originally?
Yes, but I still don't understand what prompted the question.
There is not a single use of caller cpuid_featureset_to_policy() where
zeroing the rest of the policy would be an appropriate or reasonable
thing to do.
> I did in particular ask about this effectively still being a form of black
> listing (I said "white listing" there by mistake), just taking leaf 6 (and
> Intel hardware) as example. guest_cpuid() takes whatever is there in
> the policy, and update_domain_cpuid_info() does nothing either.
See the head of recalculate_misc() which unilaterally zeroes that leaf,
irrespective of toolstack settings.
> It was my understanding that with the switch to the new model we're
> now strictly while listing features. Luwei's patch would extend the
> un-audited handing through to RDT and SGX leaves.
That is one reason why I won't ack the patch in that shape. But as I'm
currently rewriting this logic for a different reason, explaining the
correct steps to extend MAX_*_LEAF will cause unnecessary work for Luwei
as the implementation is about to change under his feet.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy Andrew Cooper
@ 2018-07-16 10:16 ` Roger Pau Monné
2018-07-16 10:58 ` Andrew Cooper
2018-07-16 11:54 ` Jan Beulich
1 sibling, 1 reply; 68+ messages in thread
From: Roger Pau Monné @ 2018-07-16 10:16 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf
On Fri, Jul 13, 2018 at 09:03:13PM +0100, Andrew Cooper wrote:
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index dd7d8a9..ee3ab09 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2553,6 +2553,12 @@ int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
> int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
> uint32_t *nr_features, uint32_t *featureset);
>
> +int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
Nit: I would do s/nr_leaves/nr_cpuid_leaves/.
> +int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
> + uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
> + uint32_t *nr_msrs, xen_msr_entry_t *msrs)
> +{
> + struct xen_sysctl sysctl = {};
> + DECLARE_HYPERCALL_BOUNCE(leaves,
> + *nr_leaves * sizeof(*leaves),
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> + DECLARE_HYPERCALL_BOUNCE(msrs,
> + *nr_msrs * sizeof(*msrs),
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> + int ret;
> +
> + if ( xc_hypercall_bounce_pre(xch, leaves) )
> + return -1;
> +
> + if ( xc_hypercall_bounce_pre(xch, msrs) )
> + return -1;
You can join both in a single if:
if ( xc_hypercall_bounce_pre(xch, leaves) ||
xc_hypercall_bounce_pre(xch, msrs) )
return -1;
> +
> + sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
> + sysctl.u.cpu_policy.index = index;
> + sysctl.u.cpu_policy.nr_leaves = *nr_leaves;
> + set_xen_guest_handle(sysctl.u.cpu_policy.cpuid_policy, leaves);
> + sysctl.u.cpu_policy.nr_msrs = *nr_msrs;
> + set_xen_guest_handle(sysctl.u.cpu_policy.msr_policy, msrs);
sysctl can be initialized at declaration time instead of zeroing it
and then setting the fields:
struct xen_sysctl sysctl = {
.cmd = XEN_SYSCTL_get_cpu_policy;
.u.cpu_policy.index = index;
.u.cpu_policy.nr_leaves = *nr_leaves;
...
};
> @@ -344,7 +377,55 @@ int main(int argc, char **argv)
> mode = MODE_INTERPRET;
> }
>
> - if ( mode == MODE_INFO || mode == MODE_DETAIL )
> + if ( mode == MODE_POLICY )
> + {
> + static const char *const sys_policies[] = {
> + [ XEN_SYSCTL_cpu_policy_raw ] = "Raw",
> + [ XEN_SYSCTL_cpu_policy_host ] = "Host",
> + [ XEN_SYSCTL_cpu_policy_pv_max ] = "PV Max",
> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = "HVM Max",
> + [ XEN_SYSCTL_cpu_policy_pv_default ] = "PV Default",
> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = "HVM Default",
> + };
> + xen_cpuid_leaf_t *leaves;
> + xen_msr_entry_t *msrs;
> + uint32_t pol, max_leaves, max_msrs;
pol could be a plain unsigned int, and named i.
> @@ -322,6 +323,76 @@ long arch_do_sysctl(
> break;
> }
>
> + case XEN_SYSCTL_get_cpu_policy:
> + {
> + const struct cpu_policy *policy;
> +
> + /* Bad policy index? */
> + if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
> + {
> + ret = -EINVAL;
> + break;
> + }
> + policy = &system_policies[
> + array_index_nospec(sysctl->u.cpu_policy.index,
> + ARRAY_SIZE(system_policies))];
> +
> + /* Request for maximum number of leaves/MSRs? */
> + if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
> + {
> + sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
> + if ( __copy_field_to_guest(u_sysctl, sysctl,
> + u.cpu_policy.nr_leaves) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> + }
> + if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
> + {
> + sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
> + if ( __copy_field_to_guest(u_sysctl, sysctl,
> + u.cpu_policy.nr_msrs) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> + }
> +
> + /* Serialise the information the caller wants. */
> + if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
> + {
> + if ( (ret = x86_cpuid_copy_to_buffer(
> + policy->cpuid,
> + sysctl->u.cpu_policy.cpuid_policy,
> + &sysctl->u.cpu_policy.nr_leaves)) )
> + break;
You could have this better aligned by first assigning the result value
to ret and then checking for errors.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 05/13] libx86: introduce a libx86 shared library
2018-07-13 20:03 ` [PATCH v2 05/13] libx86: introduce a libx86 shared library Andrew Cooper
2018-07-16 9:02 ` Wei Liu
@ 2018-07-16 10:17 ` Jan Beulich
2018-07-16 10:35 ` Andrew Cooper
1 sibling, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:17 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> tools/libxc/Makefile | 6 ++++++
> tools/libxc/include/xenctrl.h | 1 -
> tools/libxc/xc_cpuid_x86.c | 29 +---------------------------
> xen/arch/x86/cpu/common.c | 2 +-
> xen/arch/x86/cpuid.c | 32 +-----------------------------
> xen/common/Makefile | 1 +
> xen/common/libx86/Makefile | 1 +
> xen/common/libx86/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> xen/common/libx86/private.h | 38 ++++++++++++++++++++++++++++++++++++
I can certainly accept libx86 headers to live in xen/include/xen/libx86/
for now if the suggested better places cause trouble, but xen/common/
is what its name says - common (i.e. architecture independent) code.
This wants to go into lib/x86 or xen/lib/x86 or some such, or in the worst
case under xen/arch/x86/, the more that I doubt the same issue as with
the header files applies here.
> --- /dev/null
> +++ b/xen/common/libx86/private.h
> @@ -0,0 +1,38 @@
> +#ifndef XEN_LIBX86_PRIVATE_H
> +#define XEN_LIBX86_PRIVATE_H
> +
> +#ifdef __XEN__
> +
> +#include <xen/bitops.h>
> +#include <xen/kernel.h>
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +
> +#else
> +
> +#include <inttypes.h>
> +#include <stdbool.h>
> +#include <stddef.h>
> +
> +#include <xen-tools/libs.h>
> +
> +static inline bool test_bit(unsigned int bit, const void *vaddr)
> +{
> + const char *addr = vaddr;
> +
> + return addr[bit / 8] & (1u << (bit % 8));
> +}
There's nowhere in the tools that this could be taken from? xc_bitops.h
has one - couldn't this be lifted to xen-tools/libs.h?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace
2018-07-13 20:03 ` [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace Andrew Cooper
2018-07-16 9:41 ` Roger Pau Monné
@ 2018-07-16 10:19 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> To facilitate the shared Xen and toolstack code in libx86, struct msr_policy
> needs to be available in the same way as struct cpuid_policy.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h
2018-07-16 9:23 ` Jan Beulich
@ 2018-07-16 10:22 ` Andrew Cooper
2018-07-16 10:51 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 10:22 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
On 16/07/18 10:23, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> Begin to untangle the header dependency tangle by moving definition of
>> struct cpuid_leaf out of x86_emulate.h into the new cpuid.h.
>>
>> Additionally, plumb the header through to libxc. This is technically a
>> redundant include at this point, but it helps build-test the later changes,
>> and will be used eventually.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>
>> Note concerning the positioning of libx86. It turns out after trying to move
>> it elsewhere that the movement is prohibitive because of the way Xen headers
>> are included by the tools.
> If there's really something preventing it to be placed better, then I'm
> certainly willing to give my ack here, but I'd like to have a more clear
> understanding of what issue(s) you are talking about above.
It is better explained in the context of the following patch.
Moving to asm-x86/libx86/ breaks the autogen safety because the entire
tree hierarchy is made available with a single directory symlink.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
2018-07-16 10:16 ` Andrew Cooper
@ 2018-07-16 10:24 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 12:16, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 11:04, Jan Beulich wrote:
>>>>> On 16.07.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> On 16/07/18 10:38, Jan Beulich wrote:
>>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>>>> +static inline void cpuid_featureset_to_policy(
>>>>> + const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
>>>>> +{
>>>>> + p->basic._1d = fs[FEATURESET_1d];
>>>>> + p->basic._1c = fs[FEATURESET_1c];
>>>>> + p->extd.e1d = fs[FEATURESET_e1d];
>>>>> + p->extd.e1c = fs[FEATURESET_e1c];
>>>>> + p->xstate.Da1 = fs[FEATURESET_Da1];
>>>>> + p->feat._7b0 = fs[FEATURESET_7b0];
>>>>> + p->feat._7c0 = fs[FEATURESET_7c0];
>>>>> + p->extd.e7d = fs[FEATURESET_e7d];
>>>>> + p->extd.e8b = fs[FEATURESET_e8b];
>>>>> + p->feat._7d0 = fs[FEATURESET_7d0];
>>>>> +}
>>>> I realize this is only code movement, but since you didn't answer the
>>>> question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
>>>> raise it here again: Shouldn't other fields of p be set to zero here?
>>> No - why should it?
>>>
>>> (In fact, it very deliberately does not, and changing this will break
>>> all of the policy derivation logic.)
>> Did you look at the context in which I've raised the question originally?
>
> Yes, but I still don't understand what prompted the question.
The introduction of (further) intermediate leaves without any
handling.
>> I did in particular ask about this effectively still being a form of black
>> listing (I said "white listing" there by mistake), just taking leaf 6 (and
>> Intel hardware) as example. guest_cpuid() takes whatever is there in
>> the policy, and update_domain_cpuid_info() does nothing either.
>
> See the head of recalculate_misc() which unilaterally zeroes that leaf,
> irrespective of toolstack settings.
Ah, I see - irrespective of its young age things are already sufficiently
much scattered around so one won't always easily spot the necessary
pieces.
>> It was my understanding that with the switch to the new model we're
>> now strictly while listing features. Luwei's patch would extend the
>> un-audited handing through to RDT and SGX leaves.
>
> That is one reason why I won't ack the patch in that shape. But as I'm
> currently rewriting this logic for a different reason, explaining the
> correct steps to extend MAX_*_LEAF will cause unnecessary work for Luwei
> as the implementation is about to change under his feet.
Well, okay then.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
@ 2018-07-16 10:26 ` Roger Pau Monné
2018-07-17 17:08 ` Andrew Cooper
2018-07-16 12:00 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 68+ messages in thread
From: Roger Pau Monné @ 2018-07-16 10:26 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf
On Fri, Jul 13, 2018 at 09:03:14PM +0100, Andrew Cooper wrote:
> +int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> + uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
> + uint32_t *nr_msrs, xen_msr_entry_t *msrs)
> +{
> + DECLARE_DOMCTL;
> + DECLARE_HYPERCALL_BOUNCE(leaves,
> + *nr_leaves * sizeof(*leaves),
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> + DECLARE_HYPERCALL_BOUNCE(msrs,
> + *nr_msrs * sizeof(*msrs),
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> + int ret;
> +
> + if ( xc_hypercall_bounce_pre(xch, leaves) )
> + return -1;
> +
> + if ( xc_hypercall_bounce_pre(xch, msrs) )
> + return -1;
The ifs can be joined into a single one.
> + domctl.cmd = XEN_DOMCTL_get_cpu_policy;
> + domctl.domain = domid;
> + domctl.u.cpu_policy.nr_leaves = *nr_leaves;
> + set_xen_guest_handle(domctl.u.cpu_policy.cpuid_policy, leaves);
> + domctl.u.cpu_policy.nr_msrs = *nr_msrs;
> + set_xen_guest_handle(domctl.u.cpu_policy.msr_policy, msrs);
The fields can be set at declaration time, leaving just the _handle
calls here.
> + ret = do_domctl(xch, &domctl);
> +
> + xc_hypercall_bounce_post(xch, leaves);
> + xc_hypercall_bounce_post(xch, msrs);
> +
> + if ( !ret )
> + {
> + *nr_leaves = domctl.u.cpu_policy.nr_leaves;
> + *nr_msrs = domctl.u.cpu_policy.nr_msrs;
> + }
> +
> + return ret;
> +}
> +
> struct cpuid_domain_info
> {
> enum
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index 1c14d93..dd39268 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -3,6 +3,8 @@
> #include <err.h>
> #include <getopt.h>
> #include <string.h>
> +#include <errno.h>
> +#include <limits.h>
>
> #include <xenctrl.h>
>
> @@ -309,11 +311,13 @@ int main(int argc, char **argv)
> {
> enum { MODE_UNKNOWN, MODE_INFO, MODE_DETAIL, MODE_INTERPRET, MODE_POLICY }
> mode = MODE_UNKNOWN;
> + int domid = -1;
Using domid_t and DOMID_INVALID would be better IMO.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
2018-07-16 9:55 ` Roger Pau Monné
@ 2018-07-16 10:32 ` Wei Liu
2018-07-16 12:04 ` Jan Beulich
2 siblings, 0 replies; 68+ messages in thread
From: Wei Liu @ 2018-07-16 10:32 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:12PM +0100, Andrew Cooper wrote:
> This is prep work for the following patch - please refer to it as well.
>
> When auditing and manipulating policies, it is necessary to do so with a
> complete set of policies, due to the interdependences of the contents. A
> containing structure like this will allow for clearer APIs and code.
>
> As a first user, this structure is convenient for the mapping used by
> XEN_SYSCTL_get_cpu_policy (implemented in the next patch), and for auditing
> (later when XEN_DOMCTL_set_cpu_policy is implemented).
>
> At this point, the distinction between *_max and *_default is introduced into
> the ABI. For now, *_default is mapped to *_max, but future development work
> will result in *_default being a logical subset of *_max.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 05/13] libx86: introduce a libx86 shared library
2018-07-16 10:17 ` Jan Beulich
@ 2018-07-16 10:35 ` Andrew Cooper
2018-07-16 10:52 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 10:35 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
On 16/07/18 11:17, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> tools/libxc/Makefile | 6 ++++++
>> tools/libxc/include/xenctrl.h | 1 -
>> tools/libxc/xc_cpuid_x86.c | 29 +---------------------------
>> xen/arch/x86/cpu/common.c | 2 +-
>> xen/arch/x86/cpuid.c | 32 +-----------------------------
>> xen/common/Makefile | 1 +
>> xen/common/libx86/Makefile | 1 +
>> xen/common/libx86/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>> xen/common/libx86/private.h | 38 ++++++++++++++++++++++++++++++++++++
> I can certainly accept libx86 headers to live in xen/include/xen/libx86/
> for now if the suggested better places cause trouble, but xen/common/
> is what its name says - common (i.e. architecture independent) code.
> This wants to go into lib/x86 or xen/lib/x86 or some such, or in the worst
> case under xen/arch/x86/, the more that I doubt the same issue as with
> the header files applies here.
The C files are trivial to move in comparison to the header files. All
that needs changing is the single vpath in libxc's build.
I'll move the C files into xen/lib/x86/
>
>> --- /dev/null
>> +++ b/xen/common/libx86/private.h
>> @@ -0,0 +1,38 @@
>> +#ifndef XEN_LIBX86_PRIVATE_H
>> +#define XEN_LIBX86_PRIVATE_H
>> +
>> +#ifdef __XEN__
>> +
>> +#include <xen/bitops.h>
>> +#include <xen/kernel.h>
>> +#include <xen/lib.h>
>> +#include <xen/types.h>
>> +
>> +#else
>> +
>> +#include <inttypes.h>
>> +#include <stdbool.h>
>> +#include <stddef.h>
>> +
>> +#include <xen-tools/libs.h>
>> +
>> +static inline bool test_bit(unsigned int bit, const void *vaddr)
>> +{
>> + const char *addr = vaddr;
>> +
>> + return addr[bit / 8] & (1u << (bit % 8));
>> +}
> There's nowhere in the tools that this could be taken from? xc_bitops.h
> has one - couldn't this be lifted to xen-tools/libs.h?
There are already 2 different copies in tools, with different APIs, and
adding this into xen-tools/libs.h broke the build in more complicated
ways than I have time to fix at this point.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-16 9:45 ` Jan Beulich
@ 2018-07-16 10:39 ` Andrew Cooper
2018-07-16 10:55 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 10:39 UTC (permalink / raw)
To: Jan Beulich, Wei Liu
Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
On 16/07/18 10:45, Jan Beulich wrote:
>>>> On 16.07.18 at 11:18, <wei.liu2@citrix.com> wrote:
>> On Fri, Jul 13, 2018 at 09:03:08PM +0100, Andrew Cooper wrote:
>>> +#include <errno.h>
>>> #include <inttypes.h>
>>> #include <stdbool.h>
>>> #include <stddef.h>
>>> @@ -23,6 +28,19 @@ static inline bool test_bit(unsigned int bit, const void
>> *vaddr)
>>> return addr[bit / 8] & (1u << (bit % 8));
>>> }
>>>
>>> +/* memcpy(), but with copy_to_guest_offset()'s API. */
>>> +#define copy_to_buffer_offset(dst, index, src, nr) \
>>> +({ \
>>> + const typeof(*(dst)) *src_ = (src); \
>> I think you mean typeof(*(src)) here?
> To follow copy_to_guest_offset()'s model there's more needed here,
> I think: dst and src want to point to similar type objects / arrays (i.e.
> the macro wants to enforce this).
This wrapper needs to be just enough to compile for userspace. It
doesn't need all the features and misfeatures of the hypervisor
implementation.
Remember that the code gets compiled twice, so there is no chance of
errors actually slipping in.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-13 20:03 ` [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects Andrew Cooper
2018-07-16 9:18 ` Wei Liu
@ 2018-07-16 10:45 ` Jan Beulich
2018-07-17 10:02 ` Andrew Cooper
1 sibling, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:45 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> +int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
> + cpuid_leaf_buffer_t leaves,
> + uint32_t *nr_entries_p)
> +{
> + const uint32_t nr_entries = *nr_entries_p;
> + uint32_t curr_entry = 0, leaf, subleaf;
> +
> +#define COPY_LEAF(l, s, data) \
> + ({ int ret; \
> + if ( (ret = copy_leaf_to_buffer( \
> + l, s, data, leaves, &curr_entry, nr_entries)) ) \
> + return ret; \
> + })
> +
> + /* Basic leaves. */
> + for ( leaf = 0; leaf <= MIN(p->basic.max_leaf,
> + ARRAY_SIZE(p->basic.raw) - 1); ++leaf )
Here and ...
> + {
> + switch ( leaf )
> + {
> + case 0x4:
> + for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); ++subleaf )
> + COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
... here ...
> + break;
> +
> + case 0x7:
> + for ( subleaf = 0;
> + subleaf <= MIN(p->feat.max_subleaf,
> + ARRAY_SIZE(p->feat.raw) - 1); ++subleaf )
> + COPY_LEAF(leaf, subleaf, &p->feat.raw[subleaf]);
... but even more importantly here I wonder whether some form(s) of
for_each_...() wouldn't be helpful to introduce: Such constructs are a
prime source of future copy-and-past mistakes, perhaps just missing
a single of the distinguishing field names. If there was exactly one
instance of those field names, that risk would imo be much reduced.
For example (completely untested)
#define for_each_subleaf(which, limit) \
for ( subleaf = 0; subleaf <= MIN(limit, ARRAY_SIZE(p->which.raw) - 1); ++subleaf )
COPY_LEAF(leaf, subleaf, p->which.raw[subleaf]);
albeit I realize that the specification of "limit" would then still require
an open-coded use of "which", and I have no good idea how to
avoid it.
Other than the hope for some improvement there (and if nothing half
way reasonable turns up, then that'll too be fine for now) there's only
the previously given comment on copy_to_buffer_offset() here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects
2018-07-13 20:03 ` [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects Andrew Cooper
2018-07-16 9:24 ` Wei Liu
@ 2018-07-16 10:47 ` Jan Beulich
1 sibling, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:47 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
>
> As with CPUID, an architectural form is used for representing the MSR data.
> It is expected not to change moving forwards, but does have a 32 bit field
> (currently reserved) which can be used compatibly if needs be.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h
2018-07-16 10:22 ` Andrew Cooper
@ 2018-07-16 10:51 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:51 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 12:22, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 10:23, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> Begin to untangle the header dependency tangle by moving definition of
>>> struct cpuid_leaf out of x86_emulate.h into the new cpuid.h.
>>>
>>> Additionally, plumb the header through to libxc. This is technically a
>>> redundant include at this point, but it helps build-test the later changes,
>>> and will be used eventually.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>
>>> Note concerning the positioning of libx86. It turns out after trying to move
>>> it elsewhere that the movement is prohibitive because of the way Xen headers
>>> are included by the tools.
>> If there's really something preventing it to be placed better, then I'm
>> certainly willing to give my ack here, but I'd like to have a more clear
>> understanding of what issue(s) you are talking about above.
>
> It is better explained in the context of the following patch.
>
> Moving to asm-x86/libx86/ breaks the autogen safety because the entire
> tree hierarchy is made available with a single directory symlink.
But there was never talk of asm-x86/libx86/. Options discussed
were top level lib/x86/ and include/x86/, or xen/lib/x86/ and
xen/include/libx86/ (or close derivatives thereof).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 05/13] libx86: introduce a libx86 shared library
2018-07-16 10:35 ` Andrew Cooper
@ 2018-07-16 10:52 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 12:35, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 11:17, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> tools/libxc/Makefile | 6 ++++++
>>> tools/libxc/include/xenctrl.h | 1 -
>>> tools/libxc/xc_cpuid_x86.c | 29 +---------------------------
>>> xen/arch/x86/cpu/common.c | 2 +-
>>> xen/arch/x86/cpuid.c | 32 +-----------------------------
>>> xen/common/Makefile | 1 +
>>> xen/common/libx86/Makefile | 1 +
>>> xen/common/libx86/cpuid.c | 44
> ++++++++++++++++++++++++++++++++++++++++++
>>> xen/common/libx86/private.h | 38 ++++++++++++++++++++++++++++++++++++
>> I can certainly accept libx86 headers to live in xen/include/xen/libx86/
>> for now if the suggested better places cause trouble, but xen/common/
>> is what its name says - common (i.e. architecture independent) code.
>> This wants to go into lib/x86 or xen/lib/x86 or some such, or in the worst
>> case under xen/arch/x86/, the more that I doubt the same issue as with
>> the header files applies here.
>
> The C files are trivial to move in comparison to the header files. All
> that needs changing is the single vpath in libxc's build.
>
> I'll move the C files into xen/lib/x86/
Thanks, and with that then
Acked-by: Jan Beulich <jbeulich@suse.com>
>>> --- /dev/null
>>> +++ b/xen/common/libx86/private.h
>>> @@ -0,0 +1,38 @@
>>> +#ifndef XEN_LIBX86_PRIVATE_H
>>> +#define XEN_LIBX86_PRIVATE_H
>>> +
>>> +#ifdef __XEN__
>>> +
>>> +#include <xen/bitops.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/types.h>
>>> +
>>> +#else
>>> +
>>> +#include <inttypes.h>
>>> +#include <stdbool.h>
>>> +#include <stddef.h>
>>> +
>>> +#include <xen-tools/libs.h>
>>> +
>>> +static inline bool test_bit(unsigned int bit, const void *vaddr)
>>> +{
>>> + const char *addr = vaddr;
>>> +
>>> + return addr[bit / 8] & (1u << (bit % 8));
>>> +}
>> There's nowhere in the tools that this could be taken from? xc_bitops.h
>> has one - couldn't this be lifted to xen-tools/libs.h?
>
> There are already 2 different copies in tools, with different APIs, and
> adding this into xen-tools/libs.h broke the build in more complicated
> ways than I have time to fix at this point.
Oh, ouch.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-16 10:39 ` Andrew Cooper
@ 2018-07-16 10:55 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 10:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 12:39, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 10:45, Jan Beulich wrote:
>>>>> On 16.07.18 at 11:18, <wei.liu2@citrix.com> wrote:
>>> On Fri, Jul 13, 2018 at 09:03:08PM +0100, Andrew Cooper wrote:
>>>> +#include <errno.h>
>>>> #include <inttypes.h>
>>>> #include <stdbool.h>
>>>> #include <stddef.h>
>>>> @@ -23,6 +28,19 @@ static inline bool test_bit(unsigned int bit, const void
>>> *vaddr)
>>>> return addr[bit / 8] & (1u << (bit % 8));
>>>> }
>>>>
>>>> +/* memcpy(), but with copy_to_guest_offset()'s API. */
>>>> +#define copy_to_buffer_offset(dst, index, src, nr) \
>>>> +({ \
>>>> + const typeof(*(dst)) *src_ = (src); \
>>> I think you mean typeof(*(src)) here?
>> To follow copy_to_guest_offset()'s model there's more needed here,
>> I think: dst and src want to point to similar type objects / arrays (i.e.
>> the macro wants to enforce this).
>
> This wrapper needs to be just enough to compile for userspace. It
> doesn't need all the features and misfeatures of the hypervisor
> implementation.
>
> Remember that the code gets compiled twice, so there is no chance of
> errors actually slipping in.
Well, yes, people would certainly be expected to test both parts of
the build before submitting a change. But someone focusing on the
hypervisor might not always re-build the tools after each step (or
respectively vice versa), yet it would be nice if an issue was noticable
right away even when doing just one of the two builds.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-16 10:16 ` Roger Pau Monné
@ 2018-07-16 10:58 ` Andrew Cooper
2018-07-16 11:04 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 10:58 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf
On 16/07/18 11:16, Roger Pau Monné wrote:
>
>> +
>> + sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
>> + sysctl.u.cpu_policy.index = index;
>> + sysctl.u.cpu_policy.nr_leaves = *nr_leaves;
>> + set_xen_guest_handle(sysctl.u.cpu_policy.cpuid_policy, leaves);
>> + sysctl.u.cpu_policy.nr_msrs = *nr_msrs;
>> + set_xen_guest_handle(sysctl.u.cpu_policy.msr_policy, msrs);
> sysctl can be initialized at declaration time instead of zeroing it
> and then setting the fields:
>
> struct xen_sysctl sysctl = {
> .cmd = XEN_SYSCTL_get_cpu_policy;
> .u.cpu_policy.index = index;
> .u.cpu_policy.nr_leaves = *nr_leaves;
> ...
> };
This doesn't compile in a CentOS 6 era GCC. It can't cope with
initialisers of anonymous unions, and is the reason why a lot of the
toolstack logic is in the form presented in this patch, rather than the
cleaner option you present.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl
2018-07-16 9:37 ` Roger Pau Monné
@ 2018-07-16 11:02 ` Andrew Cooper
0 siblings, 0 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 11:02 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Sergey Dyasli, Wei Liu, Jan Beulich, Xen-devel
On 16/07/18 10:37, Roger Pau Monné wrote:
> On Fri, Jul 13, 2018 at 09:03:02PM +0100, Andrew Cooper wrote:
>> More than the bottom two bits are now defined, and the MSR policy work has
>> shown that using non-architectural representations turns out to be problematic
>> for more than just asm code. As the architectural representation is the
>> expected default, we don't need to justify why we are using it.
> If that's indeed the architectural representation shouldn't you use a
> uint64_t instead?
For MSR_SPEC_CTRL specifically, it makes a meaningful difference in the
complexity of the asm, but in general, there no point wasting space
storing 32 bits of 0's when we don't need to.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-16 10:58 ` Andrew Cooper
@ 2018-07-16 11:04 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 11:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Daniel de Graaf,
Roger Pau Monne
>>> On 16.07.18 at 12:58, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 11:16, Roger Pau Monné wrote:
>>
>>> +
>>> + sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
>>> + sysctl.u.cpu_policy.index = index;
>>> + sysctl.u.cpu_policy.nr_leaves = *nr_leaves;
>>> + set_xen_guest_handle(sysctl.u.cpu_policy.cpuid_policy, leaves);
>>> + sysctl.u.cpu_policy.nr_msrs = *nr_msrs;
>>> + set_xen_guest_handle(sysctl.u.cpu_policy.msr_policy, msrs);
>> sysctl can be initialized at declaration time instead of zeroing it
>> and then setting the fields:
>>
>> struct xen_sysctl sysctl = {
>> .cmd = XEN_SYSCTL_get_cpu_policy;
>> .u.cpu_policy.index = index;
>> .u.cpu_policy.nr_leaves = *nr_leaves;
>> ...
>> };
>
> This doesn't compile in a CentOS 6 era GCC. It can't cope with
> initialisers of anonymous unions, and is the reason why a lot of the
> toolstack logic is in the form presented in this patch, rather than the
> cleaner option you present.
But where's the anonymous union here? Such shouldn't be used in the
public headers anyway.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-13 20:03 ` [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper
2018-07-16 10:07 ` Wei Liu
@ 2018-07-16 11:36 ` Jan Beulich
2018-07-17 10:17 ` Andrew Cooper
1 sibling, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 11:36 UTC (permalink / raw)
To: Andrew Cooper
Cc: Ian Jackson, Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/libx86/msr.c
> +++ b/xen/common/libx86/msr.c
> @@ -45,6 +45,57 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
> return 0;
> }
>
> +int x86_msr_copy_from_buffer(struct msr_policy *p,
> + const msr_entry_buffer_t msrs, uint32_t nr_msrs,
> + uint32_t *err_msr)
> +{
> + unsigned int i;
> + xen_msr_entry_t data;
> +
> + /*
> + * A well formed caller is expected pass an array with entries in order,
> + * and without any repetitions. However, due to per-vendor differences,
> + * and in the case of upgrade or levelled scenarios, we typically expect
> + * fewer than MAX entries to be passed.
> + *
> + * Detecting repeated entries is prohibitively complicated, so we don't
> + * bother. That said, one way or another if more than MAX entries are
> + * passed, something is wrong.
> + */
> + if ( nr_msrs > MSR_MAX_SERIALISED_ENTRIES )
> + return -E2BIG;
> +
> + for ( i = 0; i < nr_msrs; i++ )
> + {
> + if ( copy_from_buffer_offset(&data, msrs, i, 1) )
> + return -EFAULT;
> +
> + if ( data.flags ) /* .flags MBZ */
> + goto err;
> +
> + switch ( data.idx )
> + {
> + case MSR_INTEL_PLATFORM_INFO:
> + if ( data.val > ~0u )
I suppose this is to guard against truncation. I think it would be
more obvious (and future proof) if you used
(typeof(p->plaform_info.raw))~0, or an intermediate variable
of that type, or data.val >> (sizeof(p->plaform_info.raw) * 8),
some of which would likely even trigger a compiler warning once
the policy field was grown to uint64_t.
> + goto err;
> +
> + p->plaform_info.raw = data.val;
No other sanity checking? If the implication is that the policy
stored into isn't an instance attached to a domain (but some
intermediate object), and sanity checking is going to be done
later, then storing and then comparing the values would perhaps
be even more obvious an approach above.
The intentions here are hard to judge, because both this
function and its CPUID counterpart appear to remain unused
by the end of the series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy Andrew Cooper
2018-07-16 10:16 ` Roger Pau Monné
@ 2018-07-16 11:54 ` Jan Beulich
2018-07-17 16:50 ` Andrew Cooper
1 sibling, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 11:54 UTC (permalink / raw)
To: Andrew Cooper, Sergey Dyasli
Cc: Ian Jackson, Daniel de Graaf, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> @@ -322,6 +323,76 @@ long arch_do_sysctl(
> break;
> }
>
> + case XEN_SYSCTL_get_cpu_policy:
> + {
> + const struct cpu_policy *policy;
> +
> + /* Bad policy index? */
> + if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
> + {
> + ret = -EINVAL;
> + break;
> + }
> + policy = &system_policies[
> + array_index_nospec(sysctl->u.cpu_policy.index,
> + ARRAY_SIZE(system_policies))];
> +
> + /* Request for maximum number of leaves/MSRs? */
> + if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
> + {
> + sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
> + if ( __copy_field_to_guest(u_sysctl, sysctl,
> + u.cpu_policy.nr_leaves) )
> + {
> + ret = -EFAULT;
> + break;
> + }
Could I talk you into avoiding the redundancy here and fold this
copying with ...
> + }
> + if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
> + {
> + sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
> + if ( __copy_field_to_guest(u_sysctl, sysctl,
> + u.cpu_policy.nr_msrs) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> + }
> +
> + /* Serialise the information the caller wants. */
> + if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
> + {
> + if ( (ret = x86_cpuid_copy_to_buffer(
> + policy->cpuid,
> + sysctl->u.cpu_policy.cpuid_policy,
> + &sysctl->u.cpu_policy.nr_leaves)) )
> + break;
> +
> + if ( __copy_field_to_guest(u_sysctl, sysctl,
> + u.cpu_policy.nr_leaves) )
> + {
> + ret = -EFAULT;
> + break;
> + }
... this (and the MSR ones respectively), by moving both out of and
past their outer if()-s? This would the also call for two if/else-if pairs
instead of four if()-s.
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1075,12 +1075,25 @@ struct xen_sysctl_set_parameter {
> * - Default_*: Default set of features a PV or HVM guest can use. This is
> * the security supported set.
> */
> +struct xen_sysctl_cpu_policy {
> #define XEN_SYSCTL_cpu_policy_raw 0
> #define XEN_SYSCTL_cpu_policy_host 1
> #define XEN_SYSCTL_cpu_policy_pv_max 2
> #define XEN_SYSCTL_cpu_policy_hvm_max 3
> #define XEN_SYSCTL_cpu_policy_pv_default 4
> #define XEN_SYSCTL_cpu_policy_hvm_default 5
> + uint32_t index; /* IN: Which policy to query? */
> + uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
> + * 'cpuid_policy', or the maximum number of leaves
> + * if the guest handle is NULL. */
> + uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
> + * 'msr_policy', or the maximum number of MSRs if
> + * the guest handle is NULL. */
> + XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
> + XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* OUT: */
Stray colons in the comments?
I'm also not overly happy to see there's still no explicit padding here.
I know you dislike it, but I think as long as we have no better
replacement to the interface versioning, we should at least try to
limit the number of bumps it needs, and that calls for making padding
explicit, zeroing it for output and checking it to be zero when input,
so that the field can be assigned meaning subsequently. Otherwise
why did we tell others to add explicit padding over the last so many
years, without you voicing your opinion to the contrary?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
2018-07-16 10:26 ` Roger Pau Monné
@ 2018-07-16 12:00 ` Jan Beulich
2018-07-30 2:14 ` Chao Gao
2018-08-17 21:22 ` Daniel De Graaf
3 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 12:00 UTC (permalink / raw)
To: Andrew Cooper, Sergey Dyasli
Cc: Ian Jackson, Daniel de Graaf, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1523,6 +1523,40 @@ long arch_do_domctl(
> recalculate_cpuid_policy(d);
> break;
>
> + case XEN_DOMCTL_get_cpu_policy:
> + if ( !guest_handle_is_null(domctl->u.cpu_policy.cpuid_policy) )
> + {
> + if ( (ret = x86_cpuid_copy_to_buffer(
> + d->arch.cpuid,
> + domctl->u.cpu_policy.cpuid_policy,
> + &domctl->u.cpu_policy.nr_leaves)) )
> + break;
> +
> + if ( __copy_field_to_guest(u_domctl, domctl,
> + u.cpu_policy.nr_leaves) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> + }
> +
> + if ( !guest_handle_is_null(domctl->u.cpu_policy.msr_policy) )
> + {
> + if ( (ret = x86_msr_copy_to_buffer(
> + d->arch.msr,
> + domctl->u.cpu_policy.msr_policy,
> + &domctl->u.cpu_policy.nr_msrs)) )
> + break;
> +
> + if ( __copy_field_to_guest(u_domctl, domctl,
> + u.cpu_policy.nr_msrs) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> + }
> + break;
Am I getting it right that the array sizing here is supposed to be done
based on the sysctl output? That's probably okay because we expect
callers to use libxc, but it doesn't make for a very consistent interface
(set).
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -635,6 +635,22 @@ struct xen_domctl_cpuid {
> uint32_t ecx;
> uint32_t edx;
> };
> +
> +/*
> + * XEN_SYSCTL_{get,set}_cpu_policy (x86 specific)
> + *
> + * Query or set the CPUID and MSR policies for a specific domain.
> + */
> +struct xen_domctl_cpu_policy {
> + uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
> + * 'cpuid_policy'. */
> + uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
> + * 'msr_domain_policy' */
> + XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT: */
> + XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* IN/OUT: */
Stray colons again in the comments? With them dropped, and
despite the remark above
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
2018-07-16 9:55 ` Roger Pau Monné
2018-07-16 10:32 ` Wei Liu
@ 2018-07-16 12:04 ` Jan Beulich
2018-07-16 12:16 ` Andrew Cooper
2 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 12:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -31,6 +31,33 @@
> #include <asm/psr.h>
> #include <asm/cpuid.h>
>
> +const struct cpu_policy system_policies[] = {
By the end of the series the array remains unused outside this
source file. I'd appreciate if it was made extern only when actually
needed, not the least because ...
> + [ XEN_SYSCTL_cpu_policy_raw ] = {
> + &raw_cpuid_policy,
> + &raw_msr_policy,
> + },
> + [ XEN_SYSCTL_cpu_policy_host ] = {
> + &host_cpuid_policy,
> + &host_msr_policy,
> + },
> + [ XEN_SYSCTL_cpu_policy_pv_max ] = {
> + &pv_max_cpuid_policy,
> + &pv_max_msr_policy,
> + },
> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
> + &hvm_max_cpuid_policy,
> + &hvm_max_msr_policy,
> + },
> + [ XEN_SYSCTL_cpu_policy_pv_default ] = {
> + &pv_max_cpuid_policy,
> + &pv_max_msr_policy,
> + },
> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
> + &hvm_max_cpuid_policy,
> + &hvm_max_msr_policy,
> + },
> +};
... this does not make obvious (without consulting sysctl.h) that
there are now holes (and hence hidden NULL pointers); this is
perhaps already undesirable with the user of this array that the
next patch adds.
With "static" added and the "extern" dropped from the header
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-16 12:04 ` Jan Beulich
@ 2018-07-16 12:16 ` Andrew Cooper
2018-07-16 12:29 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 12:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
On 16/07/18 13:04, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -31,6 +31,33 @@
>> #include <asm/psr.h>
>> #include <asm/cpuid.h>
>>
>> +const struct cpu_policy system_policies[] = {
> By the end of the series the array remains unused outside this
> source file. I'd appreciate if it was made extern only when actually
> needed, not the least because ...
>
>> + [ XEN_SYSCTL_cpu_policy_raw ] = {
>> + &raw_cpuid_policy,
>> + &raw_msr_policy,
>> + },
>> + [ XEN_SYSCTL_cpu_policy_host ] = {
>> + &host_cpuid_policy,
>> + &host_msr_policy,
>> + },
>> + [ XEN_SYSCTL_cpu_policy_pv_max ] = {
>> + &pv_max_cpuid_policy,
>> + &pv_max_msr_policy,
>> + },
>> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
>> + &hvm_max_cpuid_policy,
>> + &hvm_max_msr_policy,
>> + },
>> + [ XEN_SYSCTL_cpu_policy_pv_default ] = {
>> + &pv_max_cpuid_policy,
>> + &pv_max_msr_policy,
>> + },
>> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
>> + &hvm_max_cpuid_policy,
>> + &hvm_max_msr_policy,
>> + },
>> +};
> ... this does not make obvious (without consulting sysctl.h) that
> there are now holes (and hence hidden NULL pointers); this is
> perhaps already undesirable with the user of this array that the
> next patch adds.
What holes? There shouldn't be any, and gdb confirms my expectations:
(gdb) p/x system_policies
$1 = {{cpuid = 0xffff82d080474a80, msr = 0xffff82d080475960}, {cpuid =
0xffff82d080474340, msr = 0xffff82d08047595c}, {cpuid =
0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
0xffff82d0804734c0, msr = 0xffff82d080475958}, {cpuid =
0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
0xffff82d0804734c0, msr = 0xffff82d080475958}}
>
> With "static" added and the "extern" dropped from the header
> Acked-by: Jan Beulich <jbeulich@suse.com>
I'm not going to waste even more time by committing something which is
wrong, and having to undo it again in a later patch.
The user, DOMCTL_set_cpu_policy, is deferred from this series because it
is still under development, but there is absolutely no question that
this array needs to be externally accessible.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-16 12:16 ` Andrew Cooper
@ 2018-07-16 12:29 ` Jan Beulich
2018-07-16 13:15 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 12:29 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 14:16, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 13:04, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/sysctl.c
>>> +++ b/xen/arch/x86/sysctl.c
>>> @@ -31,6 +31,33 @@
>>> #include <asm/psr.h>
>>> #include <asm/cpuid.h>
>>>
>>> +const struct cpu_policy system_policies[] = {
>> By the end of the series the array remains unused outside this
>> source file. I'd appreciate if it was made extern only when actually
>> needed, not the least because ...
>>
>>> + [ XEN_SYSCTL_cpu_policy_raw ] = {
>>> + &raw_cpuid_policy,
>>> + &raw_msr_policy,
>>> + },
>>> + [ XEN_SYSCTL_cpu_policy_host ] = {
>>> + &host_cpuid_policy,
>>> + &host_msr_policy,
>>> + },
>>> + [ XEN_SYSCTL_cpu_policy_pv_max ] = {
>>> + &pv_max_cpuid_policy,
>>> + &pv_max_msr_policy,
>>> + },
>>> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
>>> + &hvm_max_cpuid_policy,
>>> + &hvm_max_msr_policy,
>>> + },
>>> + [ XEN_SYSCTL_cpu_policy_pv_default ] = {
>>> + &pv_max_cpuid_policy,
>>> + &pv_max_msr_policy,
>>> + },
>>> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
>>> + &hvm_max_cpuid_policy,
>>> + &hvm_max_msr_policy,
>>> + },
>>> +};
>> ... this does not make obvious (without consulting sysctl.h) that
>> there are now holes (and hence hidden NULL pointers); this is
>> perhaps already undesirable with the user of this array that the
>> next patch adds.
>
> What holes? There shouldn't be any, and gdb confirms my expectations:
>
> (gdb) p/x system_policies
> $1 = {{cpuid = 0xffff82d080474a80, msr = 0xffff82d080475960}, {cpuid =
> 0xffff82d080474340, msr = 0xffff82d08047595c}, {cpuid =
> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
> 0xffff82d0804734c0, msr = 0xffff82d080475958}, {cpuid =
> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
> 0xffff82d0804734c0, msr = 0xffff82d080475958}}
I didn't say there are holes, I've said "does not make obvious". For
example, it is not unreasonable to imagine for the
XEN_SYSCTL_cpu_policy_* values to start at 1 rather than zero, in
which case there would be two hidden NULLs at the start of the
array.
>> With "static" added and the "extern" dropped from the header
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I'm not going to waste even more time by committing something which is
> wrong, and having to undo it again in a later patch.
>
> The user, DOMCTL_set_cpu_policy, is deferred from this series because it
> is still under development, but there is absolutely no question that
> this array needs to be externally accessible.
Well, maybe I should have phrased this differently: I'm unconvinced
sysctl.c is the right place for this to live. Granted neither cpuid.c nor
msr.c are any better.
In the end the point of wanting things static for as long as they need
to be so is such that reviewers can notice when something gains
wider use. I fully accept that the "set" operation will want access to
the array, but we can't demand of other contributors to avoid
needlessly making symbols global and at the same time behave
differently ourselves. My ack stands without the requested adjustment
if this goes in together with the patch implementing "set".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-16 12:29 ` Jan Beulich
@ 2018-07-16 13:15 ` Andrew Cooper
2018-07-16 13:23 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-16 13:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Roger Pau Monne, Wei Liu, Ian Jackson, Xen-devel
On 16/07/18 13:29, Jan Beulich wrote:
>>>> On 16.07.18 at 14:16, <andrew.cooper3@citrix.com> wrote:
>> On 16/07/18 13:04, Jan Beulich wrote:
>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/sysctl.c
>>>> +++ b/xen/arch/x86/sysctl.c
>>>> @@ -31,6 +31,33 @@
>>>> #include <asm/psr.h>
>>>> #include <asm/cpuid.h>
>>>>
>>>> +const struct cpu_policy system_policies[] = {
>>> By the end of the series the array remains unused outside this
>>> source file. I'd appreciate if it was made extern only when actually
>>> needed, not the least because ...
>>>
>>>> + [ XEN_SYSCTL_cpu_policy_raw ] = {
>>>> + &raw_cpuid_policy,
>>>> + &raw_msr_policy,
>>>> + },
>>>> + [ XEN_SYSCTL_cpu_policy_host ] = {
>>>> + &host_cpuid_policy,
>>>> + &host_msr_policy,
>>>> + },
>>>> + [ XEN_SYSCTL_cpu_policy_pv_max ] = {
>>>> + &pv_max_cpuid_policy,
>>>> + &pv_max_msr_policy,
>>>> + },
>>>> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
>>>> + &hvm_max_cpuid_policy,
>>>> + &hvm_max_msr_policy,
>>>> + },
>>>> + [ XEN_SYSCTL_cpu_policy_pv_default ] = {
>>>> + &pv_max_cpuid_policy,
>>>> + &pv_max_msr_policy,
>>>> + },
>>>> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
>>>> + &hvm_max_cpuid_policy,
>>>> + &hvm_max_msr_policy,
>>>> + },
>>>> +};
>>> ... this does not make obvious (without consulting sysctl.h) that
>>> there are now holes (and hence hidden NULL pointers); this is
>>> perhaps already undesirable with the user of this array that the
>>> next patch adds.
>> What holes? There shouldn't be any, and gdb confirms my expectations:
>>
>> (gdb) p/x system_policies
>> $1 = {{cpuid = 0xffff82d080474a80, msr = 0xffff82d080475960}, {cpuid =
>> 0xffff82d080474340, msr = 0xffff82d08047595c}, {cpuid =
>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>> 0xffff82d0804734c0, msr = 0xffff82d080475958}, {cpuid =
>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>> 0xffff82d0804734c0, msr = 0xffff82d080475958}}
> I didn't say there are holes, I've said "does not make obvious".
You did, but I guess what you meant to write was "... are no holes"
rather "... are now holes".
> For example, it is not unreasonable to imagine for the
> XEN_SYSCTL_cpu_policy_* values to start at 1 rather than zero, in
> which case there would be two hidden NULLs at the start of the
> array.
Why does this matter? We have similar patterns elsewhere, and the array
cannot reasonably be used without the symbolic names (as it is really an
unordered set happening to be layed out in array form).
>
>>> With "static" added and the "extern" dropped from the header
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> I'm not going to waste even more time by committing something which is
>> wrong, and having to undo it again in a later patch.
>>
>> The user, DOMCTL_set_cpu_policy, is deferred from this series because it
>> is still under development, but there is absolutely no question that
>> this array needs to be externally accessible.
> Well, maybe I should have phrased this differently: I'm unconvinced
> sysctl.c is the right place for this to live. Granted neither cpuid.c nor
> msr.c are any better.
If you can suggest a better place then I'm all ears, but it has to live
somewhere and here was the least-bad option I could come up with.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
2018-07-16 13:15 ` Andrew Cooper
@ 2018-07-16 13:23 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-16 13:23 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 16.07.18 at 15:15, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 13:29, Jan Beulich wrote:
>>>>> On 16.07.18 at 14:16, <andrew.cooper3@citrix.com> wrote:
>>> On 16/07/18 13:04, Jan Beulich wrote:
>>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/sysctl.c
>>>>> +++ b/xen/arch/x86/sysctl.c
>>>>> @@ -31,6 +31,33 @@
>>>>> #include <asm/psr.h>
>>>>> #include <asm/cpuid.h>
>>>>>
>>>>> +const struct cpu_policy system_policies[] = {
>>>> By the end of the series the array remains unused outside this
>>>> source file. I'd appreciate if it was made extern only when actually
>>>> needed, not the least because ...
>>>>
>>>>> + [ XEN_SYSCTL_cpu_policy_raw ] = {
>>>>> + &raw_cpuid_policy,
>>>>> + &raw_msr_policy,
>>>>> + },
>>>>> + [ XEN_SYSCTL_cpu_policy_host ] = {
>>>>> + &host_cpuid_policy,
>>>>> + &host_msr_policy,
>>>>> + },
>>>>> + [ XEN_SYSCTL_cpu_policy_pv_max ] = {
>>>>> + &pv_max_cpuid_policy,
>>>>> + &pv_max_msr_policy,
>>>>> + },
>>>>> + [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
>>>>> + &hvm_max_cpuid_policy,
>>>>> + &hvm_max_msr_policy,
>>>>> + },
>>>>> + [ XEN_SYSCTL_cpu_policy_pv_default ] = {
>>>>> + &pv_max_cpuid_policy,
>>>>> + &pv_max_msr_policy,
>>>>> + },
>>>>> + [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
>>>>> + &hvm_max_cpuid_policy,
>>>>> + &hvm_max_msr_policy,
>>>>> + },
>>>>> +};
>>>> ... this does not make obvious (without consulting sysctl.h) that
>>>> there are now holes (and hence hidden NULL pointers); this is
>>>> perhaps already undesirable with the user of this array that the
>>>> next patch adds.
>>> What holes? There shouldn't be any, and gdb confirms my expectations:
>>>
>>> (gdb) p/x system_policies
>>> $1 = {{cpuid = 0xffff82d080474a80, msr = 0xffff82d080475960}, {cpuid =
>>> 0xffff82d080474340, msr = 0xffff82d08047595c}, {cpuid =
>>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>>> 0xffff82d0804734c0, msr = 0xffff82d080475958}, {cpuid =
>>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>>> 0xffff82d0804734c0, msr = 0xffff82d080475958}}
>> I didn't say there are holes, I've said "does not make obvious".
>
> You did, but I guess what you meant to write was "... are no holes"
> rather "... are now holes".
Oops.
>> For example, it is not unreasonable to imagine for the
>> XEN_SYSCTL_cpu_policy_* values to start at 1 rather than zero, in
>> which case there would be two hidden NULLs at the start of the
>> array.
>
> Why does this matter? We have similar patterns elsewhere, and the array
> cannot reasonably be used without the symbolic names (as it is really an
> unordered set happening to be layed out in array form).
It was you even more than me who was worried about NULL pointers
sitting in random places, waiting to be de-referenced. Besides the
obvious reference by symbolic, there are clearly other ways to index
into this array, not to mention someone setting up a loop over it.
Anyway - I can live with it being the way it is, and I've given the ack.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-16 10:45 ` Jan Beulich
@ 2018-07-17 10:02 ` Andrew Cooper
2018-07-17 11:58 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 10:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
On 16/07/18 11:45, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> +int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
>> + cpuid_leaf_buffer_t leaves,
>> + uint32_t *nr_entries_p)
>> +{
>> + const uint32_t nr_entries = *nr_entries_p;
>> + uint32_t curr_entry = 0, leaf, subleaf;
>> +
>> +#define COPY_LEAF(l, s, data) \
>> + ({ int ret; \
>> + if ( (ret = copy_leaf_to_buffer( \
>> + l, s, data, leaves, &curr_entry, nr_entries)) ) \
>> + return ret; \
>> + })
>> +
>> + /* Basic leaves. */
>> + for ( leaf = 0; leaf <= MIN(p->basic.max_leaf,
>> + ARRAY_SIZE(p->basic.raw) - 1); ++leaf )
> Here and ...
>
>> + {
>> + switch ( leaf )
>> + {
>> + case 0x4:
>> + for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); ++subleaf )
>> + COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
> ... here ...
>
>> + break;
>> +
>> + case 0x7:
>> + for ( subleaf = 0;
>> + subleaf <= MIN(p->feat.max_subleaf,
>> + ARRAY_SIZE(p->feat.raw) - 1); ++subleaf )
>> + COPY_LEAF(leaf, subleaf, &p->feat.raw[subleaf]);
> ... but even more importantly here I wonder whether some form(s) of
> for_each_...() wouldn't be helpful to introduce: Such constructs are a
> prime source of future copy-and-past mistakes, perhaps just missing
> a single of the distinguishing field names. If there was exactly one
> instance of those field names, that risk would imo be much reduced.
>
> For example (completely untested)
>
> #define for_each_subleaf(which, limit) \
> for ( subleaf = 0; subleaf <= MIN(limit, ARRAY_SIZE(p->which.raw) - 1); ++subleaf )
> COPY_LEAF(leaf, subleaf, p->which.raw[subleaf]);
>
> albeit I realize that the specification of "limit" would then still require
> an open-coded use of "which", and I have no good idea how to
> avoid it.
This pattern shows up in several locations, but in addition to the
problems you've found here, such a construct would be even harder for
p->extd.max_leaf which has to account for truncating the top bits out of
the limit.
I already tried, and failed, to come up with a reasonable way to
encapsulate this. The CPUID leaves aren't actually as consistent as
they appear at a first glance.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects
2018-07-16 9:57 ` Wei Liu
@ 2018-07-17 10:09 ` Andrew Cooper
0 siblings, 0 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 10:09 UTC (permalink / raw)
To: Wei Liu
Cc: Sergey Dyasli, Roger Pau Monné, Ian Jackson, Jan Beulich, Xen-devel
On 16/07/18 10:57, Wei Liu wrote:
> On Fri, Jul 13, 2018 at 09:03:10PM +0100, Andrew Cooper wrote:
>> As with the serialise side, Xen's copy_from_guest API is used, with a
>> compatibility wrapper for the userspace build.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>
>> v2:
>> * Rewrite copy_from_buffer_offset() to avoid multiple evaluation of its
>> arguments.
>> * Expand boundary justifications.
>> ---
>> xen/common/libx86/cpuid.c | 94 ++++++++++++++++++++++++++++++++++++++++++
>> xen/common/libx86/private.h | 14 +++++++
>> xen/include/xen/libx86/cpuid.h | 11 +++++
>> 3 files changed, 119 insertions(+)
>>
>> diff --git a/xen/common/libx86/cpuid.c b/xen/common/libx86/cpuid.c
>> index cf7dbd3..73cd574 100644
>> --- a/xen/common/libx86/cpuid.c
>> +++ b/xen/common/libx86/cpuid.c
>> @@ -123,6 +123,100 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
>> return 0;
>> }
>>
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/xen/common/libx86/private.h b/xen/common/libx86/private.h
>> index e874fb6..dc451d0 100644
>> --- a/xen/common/libx86/private.h
>> +++ b/xen/common/libx86/private.h
>> @@ -12,6 +12,7 @@
>> #include <asm/msr-index.h>
>>
>> #define copy_to_buffer_offset copy_to_guest_offset
>> +#define copy_from_buffer_offset copy_from_guest_offset
>>
>> #else
>>
>> @@ -44,6 +45,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr)
>> 0; \
>> })
>>
>> +/* memcpy(), but with copy_from_guest_offset()'s API. */
>> +#define copy_from_buffer_offset(dst, src, index, nr) \
>> +({ \
>> + const typeof(*(dst)) *src_ = (src); \
> Same issue as previous patch here. Also enforcing dst_ and src_ are of
> the same type would be good.
>
> Otherwise:
>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
The use of dst *is* the enforcement of them being the same type.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-16 11:36 ` Jan Beulich
@ 2018-07-17 10:17 ` Andrew Cooper
2018-07-17 12:01 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 10:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Jackson, Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monne
On 16/07/18 12:36, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/libx86/msr.c
>> +++ b/xen/common/libx86/msr.c
>> @@ -45,6 +45,57 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
>> return 0;
>> }
>>
>> +int x86_msr_copy_from_buffer(struct msr_policy *p,
>> + const msr_entry_buffer_t msrs, uint32_t nr_msrs,
>> + uint32_t *err_msr)
>> +{
>> + unsigned int i;
>> + xen_msr_entry_t data;
>> +
>> + /*
>> + * A well formed caller is expected pass an array with entries in order,
>> + * and without any repetitions. However, due to per-vendor differences,
>> + * and in the case of upgrade or levelled scenarios, we typically expect
>> + * fewer than MAX entries to be passed.
>> + *
>> + * Detecting repeated entries is prohibitively complicated, so we don't
>> + * bother. That said, one way or another if more than MAX entries are
>> + * passed, something is wrong.
>> + */
>> + if ( nr_msrs > MSR_MAX_SERIALISED_ENTRIES )
>> + return -E2BIG;
>> +
>> + for ( i = 0; i < nr_msrs; i++ )
>> + {
>> + if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>> + return -EFAULT;
>> +
>> + if ( data.flags ) /* .flags MBZ */
>> + goto err;
>> +
>> + switch ( data.idx )
>> + {
>> + case MSR_INTEL_PLATFORM_INFO:
>> + if ( data.val > ~0u )
> I suppose this is to guard against truncation. I think it would be
> more obvious (and future proof) if you used
> (typeof(p->plaform_info.raw))~0,
ITYM ~((typeof(p->plaform_info.raw)0) ...
> or an intermediate variable
> of that type, or data.val >> (sizeof(p->plaform_info.raw) * 8),
> some of which would likely even trigger a compiler warning once
> the policy field was grown to uint64_t.
... but this is probably better.
>
>> + goto err;
>> +
>> + p->plaform_info.raw = data.val;
> No other sanity checking?
Correct. This is a data marshalling function, not an auditing function.
The auditing functions are also needed for in-place modification to an
existing policy.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects
2018-07-17 10:02 ` Andrew Cooper
@ 2018-07-17 11:58 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-17 11:58 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Xen-devel, Wei Liu, Ian Jackson, Roger Pau Monne
>>> On 17.07.18 at 12:02, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 11:45, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> +int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
>>> + cpuid_leaf_buffer_t leaves,
>>> + uint32_t *nr_entries_p)
>>> +{
>>> + const uint32_t nr_entries = *nr_entries_p;
>>> + uint32_t curr_entry = 0, leaf, subleaf;
>>> +
>>> +#define COPY_LEAF(l, s, data) \
>>> + ({ int ret; \
>>> + if ( (ret = copy_leaf_to_buffer( \
>>> + l, s, data, leaves, &curr_entry, nr_entries)) ) \
>>> + return ret; \
>>> + })
>>> +
>>> + /* Basic leaves. */
>>> + for ( leaf = 0; leaf <= MIN(p->basic.max_leaf,
>>> + ARRAY_SIZE(p->basic.raw) - 1); ++leaf )
>> Here and ...
>>
>>> + {
>>> + switch ( leaf )
>>> + {
>>> + case 0x4:
>>> + for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); ++subleaf )
>>> + COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
>> ... here ...
>>
>>> + break;
>>> +
>>> + case 0x7:
>>> + for ( subleaf = 0;
>>> + subleaf <= MIN(p->feat.max_subleaf,
>>> + ARRAY_SIZE(p->feat.raw) - 1); ++subleaf )
>>> + COPY_LEAF(leaf, subleaf, &p->feat.raw[subleaf]);
>> ... but even more importantly here I wonder whether some form(s) of
>> for_each_...() wouldn't be helpful to introduce: Such constructs are a
>> prime source of future copy-and-past mistakes, perhaps just missing
>> a single of the distinguishing field names. If there was exactly one
>> instance of those field names, that risk would imo be much reduced.
>>
>> For example (completely untested)
>>
>> #define for_each_subleaf(which, limit) \
>> for ( subleaf = 0; subleaf <= MIN(limit, ARRAY_SIZE(p->which.raw) - 1);
> ++subleaf )
>> COPY_LEAF(leaf, subleaf, p->which.raw[subleaf]);
>>
>> albeit I realize that the specification of "limit" would then still require
>> an open-coded use of "which", and I have no good idea how to
>> avoid it.
>
> This pattern shows up in several locations, but in addition to the
> problems you've found here, such a construct would be even harder for
> p->extd.max_leaf which has to account for truncating the top bits out of
> the limit.
Dropping the top bits could be done universally, e.g. by AND-ing the
leaf with 0xffff.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-17 10:17 ` Andrew Cooper
@ 2018-07-17 12:01 ` Jan Beulich
2018-07-17 16:06 ` Andrew Cooper
0 siblings, 1 reply; 68+ messages in thread
From: Jan Beulich @ 2018-07-17 12:01 UTC (permalink / raw)
To: Andrew Cooper
Cc: Ian Jackson, Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monne
>>> On 17.07.18 at 12:17, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 12:36, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/libx86/msr.c
>>> +++ b/xen/common/libx86/msr.c
>>> @@ -45,6 +45,57 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
>>> return 0;
>>> }
>>>
>>> +int x86_msr_copy_from_buffer(struct msr_policy *p,
>>> + const msr_entry_buffer_t msrs, uint32_t
> nr_msrs,
>>> + uint32_t *err_msr)
>>> +{
>>> + unsigned int i;
>>> + xen_msr_entry_t data;
>>> +
>>> + /*
>>> + * A well formed caller is expected pass an array with entries in
> order,
>>> + * and without any repetitions. However, due to per-vendor
> differences,
>>> + * and in the case of upgrade or levelled scenarios, we typically
> expect
>>> + * fewer than MAX entries to be passed.
>>> + *
>>> + * Detecting repeated entries is prohibitively complicated, so we don't
>>> + * bother. That said, one way or another if more than MAX entries are
>>> + * passed, something is wrong.
>>> + */
>>> + if ( nr_msrs > MSR_MAX_SERIALISED_ENTRIES )
>>> + return -E2BIG;
>>> +
>>> + for ( i = 0; i < nr_msrs; i++ )
>>> + {
>>> + if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>>> + return -EFAULT;
>>> +
>>> + if ( data.flags ) /* .flags MBZ */
>>> + goto err;
>>> +
>>> + switch ( data.idx )
>>> + {
>>> + case MSR_INTEL_PLATFORM_INFO:
>>> + if ( data.val > ~0u )
>> I suppose this is to guard against truncation. I think it would be
>> more obvious (and future proof) if you used
>> (typeof(p->plaform_info.raw))~0,
>
> ITYM ~((typeof(p->plaform_info.raw)0) ...
Both have the same effect afaict, due to ~0 being signed int.
>> or an intermediate variable
>> of that type, or data.val >> (sizeof(p->plaform_info.raw) * 8),
>> some of which would likely even trigger a compiler warning once
>> the policy field was grown to uint64_t.
>
> ... but this is probably better.
>
>>
>>> + goto err;
>>> +
>>> + p->plaform_info.raw = data.val;
>> No other sanity checking?
>
> Correct. This is a data marshalling function, not an auditing function.
>
> The auditing functions are also needed for in-place modification to an
> existing policy.
Right, but the primary problem with understanding whether the lack
of checking here is a problem is the lack of a caller of this function.
As I think I did say in the earlier reply - it matters quite a bit where p
points.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-17 12:01 ` Jan Beulich
@ 2018-07-17 16:06 ` Andrew Cooper
2018-07-17 16:23 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 16:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Jackson, Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monne
On 17/07/18 13:01, Jan Beulich wrote:
>>>> + goto err;
>>>> +
>>>> + p->plaform_info.raw = data.val;
>>> No other sanity checking?
>> Correct. This is a data marshalling function, not an auditing function.
>>
>> The auditing functions are also needed for in-place modification to an
>> existing policy.
> Right, but the primary problem with understanding whether the lack
> of checking here is a problem is the lack of a caller of this function.
The reason there is no caller is because you objected to my stub
implementation in v1.
This marshalling support is currently blocking other work, which is why
I've split it out, to allow development to continue in parallel.
> As I think I did say in the earlier reply - it matters quite a bit where p
> points.
No - it doesn't. This is a function to convert data between two binary
representations, as is explained by its documentation.
Auditing the contents of the data would a) need to happen in combination
with a cpuid_policy object, and b) would be a layering violation.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects
2018-07-17 16:06 ` Andrew Cooper
@ 2018-07-17 16:23 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-17 16:23 UTC (permalink / raw)
To: Andrew Cooper
Cc: Ian Jackson, Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monne
>>> On 17.07.18 at 18:06, <andrew.cooper3@citrix.com> wrote:
> On 17/07/18 13:01, Jan Beulich wrote:
>>>>> + goto err;
>>>>> +
>>>>> + p->plaform_info.raw = data.val;
>>>> No other sanity checking?
>>> Correct. This is a data marshalling function, not an auditing function.
>>>
>>> The auditing functions are also needed for in-place modification to an
>>> existing policy.
>> Right, but the primary problem with understanding whether the lack
>> of checking here is a problem is the lack of a caller of this function.
>
> The reason there is no caller is because you objected to my stub
> implementation in v1.
>
> This marshalling support is currently blocking other work, which is why
> I've split it out, to allow development to continue in parallel.
>
>> As I think I did say in the earlier reply - it matters quite a bit where p
>> points.
>
> No - it doesn't. This is a function to convert data between two binary
> representations, as is explained by its documentation.
>
> Auditing the contents of the data would a) need to happen in combination
> with a cpuid_policy object, and b) would be a layering violation.
I understand what you mean, but that wasn't my point. If p was the
policy of a live domain, blindly putting something in there without
auditing would (a) make unrolling in case of error impossible and (b)
would transiently show too wide a policy to the guest. Yes, you mean
to disallow policy updates once a guest was started, but this again is
not visible here. As in so many other cases - introducing functions
without callers is problematic.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-16 11:54 ` Jan Beulich
@ 2018-07-17 16:50 ` Andrew Cooper
2018-07-18 6:45 ` Jan Beulich
0 siblings, 1 reply; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 16:50 UTC (permalink / raw)
To: Jan Beulich, Sergey Dyasli
Cc: Ian Jackson, Daniel de Graaf, Xen-devel, Wei Liu, Roger Pau Monne
On 16/07/18 12:54, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> @@ -322,6 +323,76 @@ long arch_do_sysctl(
>> break;
>> }
>>
>> + case XEN_SYSCTL_get_cpu_policy:
>> + {
>> + const struct cpu_policy *policy;
>> +
>> + /* Bad policy index? */
>> + if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
>> + {
>> + ret = -EINVAL;
>> + break;
>> + }
>> + policy = &system_policies[
>> + array_index_nospec(sysctl->u.cpu_policy.index,
>> + ARRAY_SIZE(system_policies))];
>> +
>> + /* Request for maximum number of leaves/MSRs? */
>> + if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>> + {
>> + sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>> + u.cpu_policy.nr_leaves) )
>> + {
>> + ret = -EFAULT;
>> + break;
>> + }
> Could I talk you into avoiding the redundancy here and fold this
> copying with ...
>
>> + }
>> + if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
>> + {
>> + sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>> + u.cpu_policy.nr_msrs) )
>> + {
>> + ret = -EFAULT;
>> + break;
>> + }
>> + }
>> +
>> + /* Serialise the information the caller wants. */
>> + if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>> + {
>> + if ( (ret = x86_cpuid_copy_to_buffer(
>> + policy->cpuid,
>> + sysctl->u.cpu_policy.cpuid_policy,
>> + &sysctl->u.cpu_policy.nr_leaves)) )
>> + break;
>> +
>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>> + u.cpu_policy.nr_leaves) )
>> + {
>> + ret = -EFAULT;
>> + break;
>> + }
> ... this (and the MSR ones respectively), by moving both out of and
> past their outer if()-s? This would the also call for two if/else-if pairs
> instead of four if()-s.
I
>
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1075,12 +1075,25 @@ struct xen_sysctl_set_parameter {
>> * - Default_*: Default set of features a PV or HVM guest can use. This is
>> * the security supported set.
>> */
>> +struct xen_sysctl_cpu_policy {
>> #define XEN_SYSCTL_cpu_policy_raw 0
>> #define XEN_SYSCTL_cpu_policy_host 1
>> #define XEN_SYSCTL_cpu_policy_pv_max 2
>> #define XEN_SYSCTL_cpu_policy_hvm_max 3
>> #define XEN_SYSCTL_cpu_policy_pv_default 4
>> #define XEN_SYSCTL_cpu_policy_hvm_default 5
>> + uint32_t index; /* IN: Which policy to query? */
>> + uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
>> + * 'cpuid_policy', or the maximum number of leaves
>> + * if the guest handle is NULL. */
>> + uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
>> + * 'msr_policy', or the maximum number of MSRs if
>> + * the guest handle is NULL. */
>> + XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
>> + XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* OUT: */
> Stray colons in the comments?
>
> I'm also not overly happy to see there's still no explicit padding here.
> I know you dislike it, but I think as long as we have no better
> replacement to the interface versioning, we should at least try to
> limit the number of bumps it needs, and that calls for making padding
> explicit, zeroing it for output and checking it to be zero when input,
> so that the field can be assigned meaning subsequently. Otherwise
> why did we tell others to add explicit padding over the last so many
> years, without you voicing your opinion to the contrary?
I've never (knowingly) requested padding in the unstable interface.
I've certainly requested rearrangements for better packing, and
requested padding for bits of the stable ABI.
The sysctl structure itself is full of holes, and this is not an
appropriate time or place to be making partial changes to the ABI.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-16 10:26 ` Roger Pau Monné
@ 2018-07-17 17:08 ` Andrew Cooper
0 siblings, 0 replies; 68+ messages in thread
From: Andrew Cooper @ 2018-07-17 17:08 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf
On 16/07/18 11:26, Roger Pau Monné wrote:
> On Fri, Jul 13, 2018 at 09:03:14PM +0100, Andrew Cooper wrote:
>> @@ -309,11 +311,13 @@ int main(int argc, char **argv)
>> {
>> enum { MODE_UNKNOWN, MODE_INFO, MODE_DETAIL, MODE_INTERPRET, MODE_POLICY }
>> mode = MODE_UNKNOWN;
>> + int domid = -1;
> Using domid_t and DOMID_INVALID would be better IMO.
Please see 5b42c82f5584ca8b0e169c6de1b6d81214ea07f2 for why domid_t is
unusable in toolstack code.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy
2018-07-17 16:50 ` Andrew Cooper
@ 2018-07-18 6:45 ` Jan Beulich
0 siblings, 0 replies; 68+ messages in thread
From: Jan Beulich @ 2018-07-18 6:45 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Daniel de Graaf,
Roger Pau Monne
>>> On 17.07.18 at 18:50, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 12:54, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> @@ -322,6 +323,76 @@ long arch_do_sysctl(
>>> break;
>>> }
>>>
>>> + case XEN_SYSCTL_get_cpu_policy:
>>> + {
>>> + const struct cpu_policy *policy;
>>> +
>>> + /* Bad policy index? */
>>> + if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
>>> + {
>>> + ret = -EINVAL;
>>> + break;
>>> + }
>>> + policy = &system_policies[
>>> + array_index_nospec(sysctl->u.cpu_policy.index,
>>> + ARRAY_SIZE(system_policies))];
>>> +
>>> + /* Request for maximum number of leaves/MSRs? */
>>> + if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>>> + {
>>> + sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
>>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> + u.cpu_policy.nr_leaves) )
>>> + {
>>> + ret = -EFAULT;
>>> + break;
>>> + }
>> Could I talk you into avoiding the redundancy here and fold this
>> copying with ...
>>
>>> + }
>>> + if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
>>> + {
>>> + sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
>>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> + u.cpu_policy.nr_msrs) )
>>> + {
>>> + ret = -EFAULT;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + /* Serialise the information the caller wants. */
>>> + if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>>> + {
>>> + if ( (ret = x86_cpuid_copy_to_buffer(
>>> + policy->cpuid,
>>> + sysctl->u.cpu_policy.cpuid_policy,
>>> + &sysctl->u.cpu_policy.nr_leaves)) )
>>> + break;
>>> +
>>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> + u.cpu_policy.nr_leaves) )
>>> + {
>>> + ret = -EFAULT;
>>> + break;
>>> + }
>> ... this (and the MSR ones respectively), by moving both out of and
>> past their outer if()-s? This would the also call for two if/else-if pairs
>> instead of four if()-s.
>
> I
???
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1075,12 +1075,25 @@ struct xen_sysctl_set_parameter {
>>> * - Default_*: Default set of features a PV or HVM guest can use. This is
>>> * the security supported set.
>>> */
>>> +struct xen_sysctl_cpu_policy {
>>> #define XEN_SYSCTL_cpu_policy_raw 0
>>> #define XEN_SYSCTL_cpu_policy_host 1
>>> #define XEN_SYSCTL_cpu_policy_pv_max 2
>>> #define XEN_SYSCTL_cpu_policy_hvm_max 3
>>> #define XEN_SYSCTL_cpu_policy_pv_default 4
>>> #define XEN_SYSCTL_cpu_policy_hvm_default 5
>>> + uint32_t index; /* IN: Which policy to query? */
>>> + uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
>>> + * 'cpuid_policy', or the maximum number of leaves
>>> + * if the guest handle is NULL. */
>>> + uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
>>> + * 'msr_policy', or the maximum number of MSRs if
>>> + * the guest handle is NULL. */
>>> + XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
>>> + XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* OUT: */
>> Stray colons in the comments?
>>
>> I'm also not overly happy to see there's still no explicit padding here.
>> I know you dislike it, but I think as long as we have no better
>> replacement to the interface versioning, we should at least try to
>> limit the number of bumps it needs, and that calls for making padding
>> explicit, zeroing it for output and checking it to be zero when input,
>> so that the field can be assigned meaning subsequently. Otherwise
>> why did we tell others to add explicit padding over the last so many
>> years, without you voicing your opinion to the contrary?
>
> I've never (knowingly) requested padding in the unstable interface.
> I've certainly requested rearrangements for better packing, and
> requested padding for bits of the stable ABI.
I don't recall you requesting such, indeed, but I also don't recall you
objecting to me (and iirc also a few others) doing so.
> The sysctl structure itself is full of holes, and this is not an
> appropriate time or place to be making partial changes to the ABI.
Well, I'm not going to insist, since as you say it's inconsistent at
present anyway. But we'll want to settle on one of the other
model for new additions, and then consider converting existing
ones accordingly.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
2018-07-16 10:26 ` Roger Pau Monné
2018-07-16 12:00 ` Jan Beulich
@ 2018-07-30 2:14 ` Chao Gao
2018-08-17 21:22 ` Daniel De Graaf
3 siblings, 0 replies; 68+ messages in thread
From: Chao Gao @ 2018-07-30 2:14 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf, Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:14PM +0100, Andrew Cooper wrote:
>From: Sergey Dyasli <sergey.dyasli@citrix.com>
>
>This finally (after literally years of work!) marks the point where the
>toolstack can ask the hypervisor for the current CPUID configuration of a
>specific domain.
>
>Also extend xen-cpuid's --policy mode to be able to take a domid and dump a
>specific domains CPUID and MSR policy.
>
>Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>---
>+
>+/*
>+ * XEN_SYSCTL_{get,set}_cpu_policy (x86 specific)
>+ *
>+ * Query or set the CPUID and MSR policies for a specific domain.
Setting policies isn't introduced in this series. The comments here
should be aligned with what have been implemented.
Thanks
Chao
>+ */
>+struct xen_domctl_cpu_policy {
>+ uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
>+ * 'cpuid_policy'. */
>+ uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
>+ * 'msr_domain_policy' */
>+ XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT: */
>+ XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy; /* IN/OUT: */
>+};
>+typedef struct xen_domctl_cpu_policy xen_domctl_cpu_policy_t;
>+DEFINE_XEN_GUEST_HANDLE(xen_domctl_cpu_policy_t);
> #endif
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
` (12 preceding siblings ...)
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
@ 2018-07-30 2:46 ` Chao Gao
13 siblings, 0 replies; 68+ messages in thread
From: Chao Gao @ 2018-07-30 2:46 UTC (permalink / raw)
To: Andrew Cooper
Cc: Sergey Dyasli, Wei Liu, Ian Jackson, Xen-devel, Jan Beulich,
Daniel De Graaf, Roger Pau Monné
On Fri, Jul 13, 2018 at 09:03:01PM +0100, Andrew Cooper wrote:
>This series introduces libx86, a small shared library between the hypervisor
>and libxc, and hypercalls to get full CPUID/MSR policies. Future work will
>implement XEN_DOMCTL_set_cpumsr_policy, after the auditing and comparison
>logic is sorted.
>
>In the meantime, the data marshalling logic is in a suitable state for review.
Hi, Andrew:
Glad to see your CPUID series. I noticed that this series doesn't
include things like, setting policies, removing a long-standing
issue - vAPIC ID is hard-coded to "2 * vcpu_id", and vCPU topology.
Will you post patches for them? If yes, are they intended to be
merged in Xen 4.12?
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
` (2 preceding siblings ...)
2018-07-30 2:14 ` Chao Gao
@ 2018-08-17 21:22 ` Daniel De Graaf
3 siblings, 0 replies; 68+ messages in thread
From: Daniel De Graaf @ 2018-08-17 21:22 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Sergey Dyasli, Ian Jackson, Wei Liu, Jan Beulich, Roger Pau Monné
On 07/13/2018 04:03 PM, Andrew Cooper wrote:
> From: Sergey Dyasli <sergey.dyasli@citrix.com>
>
> This finally (after literally years of work!) marks the point where the
> toolstack can ask the hypervisor for the current CPUID configuration of a
> specific domain.
>
> Also extend xen-cpuid's --policy mode to be able to take a domid and dump a
> specific domains CPUID and MSR policy.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Since this is a read operation, it should have a new access vector (probably
get_cpuid) instead of sitting under set_cpuid so that it is possible to allow
viewing the policy without allowing it to be changed (useful for migration out,
introspection, or duplicating the settings on a now-protected domain).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 68+ messages in thread
end of thread, other threads:[~2018-08-17 21:22 UTC | newest]
Thread overview: 68+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-13 20:03 [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 01/13] x86/msr: Drop stale comment for vcpu_msrs.spec_ctrl Andrew Cooper
2018-07-16 7:21 ` Jan Beulich
2018-07-16 9:37 ` Roger Pau Monné
2018-07-16 11:02 ` Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 02/13] libx86: Introduce libx86/cpuid.h Andrew Cooper
2018-07-16 9:23 ` Jan Beulich
2018-07-16 10:22 ` Andrew Cooper
2018-07-16 10:51 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 03/13] libx86: generate cpuid-autogen.h in the libx86 include dir Andrew Cooper
2018-07-16 9:31 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace Andrew Cooper
2018-07-16 9:38 ` Jan Beulich
2018-07-16 9:51 ` Andrew Cooper
2018-07-16 10:04 ` Jan Beulich
2018-07-16 10:16 ` Andrew Cooper
2018-07-16 10:24 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 05/13] libx86: introduce a libx86 shared library Andrew Cooper
2018-07-16 9:02 ` Wei Liu
2018-07-16 10:17 ` Jan Beulich
2018-07-16 10:35 ` Andrew Cooper
2018-07-16 10:52 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 06/13] libx86: Introduce libx86/msr.h and share msr_policy with userspace Andrew Cooper
2018-07-16 9:41 ` Roger Pau Monné
2018-07-16 10:19 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects Andrew Cooper
2018-07-16 9:18 ` Wei Liu
2018-07-16 9:45 ` Jan Beulich
2018-07-16 10:39 ` Andrew Cooper
2018-07-16 10:55 ` Jan Beulich
2018-07-16 10:45 ` Jan Beulich
2018-07-17 10:02 ` Andrew Cooper
2018-07-17 11:58 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 08/13] libx86: Introduce a helper to serialise msr_policy objects Andrew Cooper
2018-07-16 9:24 ` Wei Liu
2018-07-16 10:47 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 09/13] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper
2018-07-16 9:57 ` Wei Liu
2018-07-17 10:09 ` Andrew Cooper
2018-07-13 20:03 ` [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper
2018-07-16 10:07 ` Wei Liu
2018-07-16 11:36 ` Jan Beulich
2018-07-17 10:17 ` Andrew Cooper
2018-07-17 12:01 ` Jan Beulich
2018-07-17 16:06 ` Andrew Cooper
2018-07-17 16:23 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies Andrew Cooper
2018-07-16 9:55 ` Roger Pau Monné
2018-07-16 10:32 ` Wei Liu
2018-07-16 12:04 ` Jan Beulich
2018-07-16 12:16 ` Andrew Cooper
2018-07-16 12:29 ` Jan Beulich
2018-07-16 13:15 ` Andrew Cooper
2018-07-16 13:23 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy Andrew Cooper
2018-07-16 10:16 ` Roger Pau Monné
2018-07-16 10:58 ` Andrew Cooper
2018-07-16 11:04 ` Jan Beulich
2018-07-16 11:54 ` Jan Beulich
2018-07-17 16:50 ` Andrew Cooper
2018-07-18 6:45 ` Jan Beulich
2018-07-13 20:03 ` [PATCH v2 13/13] x86/domctl: Implement XEN_DOMCTL_get_cpu_policy Andrew Cooper
2018-07-16 10:26 ` Roger Pau Monné
2018-07-17 17:08 ` Andrew Cooper
2018-07-16 12:00 ` Jan Beulich
2018-07-30 2:14 ` Chao Gao
2018-08-17 21:22 ` Daniel De Graaf
2018-07-30 2:46 ` [PATCH v2 00/13] x86: CPUID and MSR policy marshalling support Chao Gao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.