11:41 a.m.
This series introduces the concept of a 'Secure Guest' feature
which covers on s390 IBM Secure Execution and on x86 AMD Secure
Encrypted Virtualization.
Besides adding documentation for IBM Secure Execution it also adds
checks during validation of the qemu capabilities cache.
These checks per architecture can be performed for IBM Secure
Execution on s390 and AMD Secure Encrypted Virtualization on AMD x86
CPUs (both checks implemented in this series).
For s390 the verification consists of:
- checking if /sys/firmware/uv is available: meaning the HW
facility is available and the host OS supports it;
- checking if the kernel cmdline contains 'prot_virt=1': meaning
the host OS wants to use the feature.
For AMD Secure Encrypted Virtualization the verification consists of:
- checking if /sys/module/kvm_amd/parameters/sev contains the
value '1': meaning SEV is enabled in the host kernel;
- checking if the kernel cmdline contains 'mem_encrypt=on': meaning
SME memory encryption feature on the host is enabled
Whenever the availability of the feature does not match the secure
guest flag in the cache then libvirt will re-build it in order to
pick up the new set of capabilities available.
Additionally, this series adds the same aforementioned checks to the
virt-host-validate tool to facilitate the manual verification
process for users.
Boris Fiuczynski (2):
tools: secure guest check on s390 in virt-host-validate
tools: secure guest check for AMD in virt-host-validate
Paulo de Rezende Pinatti (3):
util: introduce a parser for kernel cmdline arguments
qemu: check if s390 secure guest support is enabled
qemu: check if AMD secure guest support is enabled
Viktor Mihajlovski (1):
docs: Describe protected virtualization guest setup
docs/kbase.html.in | 3 +
docs/kbase/launch_security_sev.rst | 9 +-
docs/kbase/s390_protected_virt.rst | 189 +++++++++++++++++++++++++++++
src/libvirt_private.syms | 2 +
src/qemu/qemu_capabilities.c | 83 +++++++++++++
src/util/virutil.c | 173 ++++++++++++++++++++++++++
src/util/virutil.h | 18 +++
tests/utiltest.c | 130 ++++++++++++++++++++
tools/virt-host-validate-common.c | 90 +++++++++++++-
tools/virt-host-validate-common.h | 5 +
tools/virt-host-validate-qemu.c | 4 +
11 files changed, 701 insertions(+), 5 deletions(-)
create mode 100644 docs/kbase/s390_protected_virt.rst
--
2.25.1
11:41 a.m.
New subject: [PATCH 1/6] util: introduce a parser for kernel cmdline arguments
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Introduce two utility functions to parse a kernel command
line string according to the kernel code parsing rules in
order to enable the caller to perform operations such as
verifying whether certain argument=value combinations are
present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/libvirt_private.syms | 2 +
src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 18 ++++
tests/utiltest.c | 130 +++++++++++++++++++++++++++++
4 files changed, 323 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 935ef7303b..25b22a3e3f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
virHostHasIOMMU;
virIndexToDiskName;
virIsDevMapperDevice;
+virKernelCmdlineGetValue;
+virKernelCmdlineMatchParam;
virMemoryLimitIsSet;
virMemoryLimitTruncate;
virMemoryMaxValue;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index fb46501142..264aae8d01 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
return ret;
}
+
+#define VIR_IS_CMDLINE_SPACE(value) \
+ (g_ascii_isspace(value) || (unsigned char) value == 160)
+
+
+static bool virKernelArgsAreEqual(const char *arg1,
+ const char *arg2,
+ size_t n)
+{
+ size_t i;
+
+ for (i = 0; i < n; i++) {
+ if ((arg1[i] == '-' && arg2[i] == '_') ||
+ (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] ==
arg2[i])) {
+ if (arg1[i] == '\0')
+ return true;
+ continue;
+ }
+ return false;
+ }
+ return true;
+}
+
+
+/*
+ * Parse the kernel cmdline and store the value of @arg in @val
+ * which can be NULL if @arg has no value or if it's not found.
+ * In addition, store in @next the address right after
+ * @arg=@value for possible further processing.
+ *
+ * @arg: kernel command line argument
+ * @cmdline: kernel command line string to be checked for @arg
+ * @val: pointer to hold retrieved value of @arg
+ * @next: pointer to hold address right after @arg=@val, will
+ * point to the string's end (NULL) in case @arg is not found
+ *
+ * Returns 0 if @arg is present in @cmdline, 1 otherwise
+ */
+int virKernelCmdlineGetValue(const char *arg,
+ const char *cmdline,
+ char **val,
+ const char **next)
+{
+ size_t i = 0, param_i;
+ size_t arg_len = strlen(arg);
+ bool is_quoted, match;
+
+ *next = cmdline;
+ *val = NULL;
+
+ while (cmdline[i] != '\0') {
+ /* remove leading spaces */
+ while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
+ i++;
+ if (cmdline[i] == '"') {
+ i++;
+ is_quoted = true;
+ } else {
+ is_quoted = false;
+ }
+ for (param_i = i; cmdline[param_i]; param_i++) {
+ if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
+ cmdline[param_i] == '=')
+ break;
+ if (cmdline[param_i] == '"')
+ is_quoted = !is_quoted;
+ }
+ if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg,
arg_len))
+ match = true;
+ else
+ match = false;
+ /* arg has no value */
+ if (cmdline[param_i] != '=') {
+ if (match) {
+ *next = cmdline+param_i;
+ return 0;
+ }
+ i = param_i;
+ continue;
+ }
+ param_i++;
+ if (cmdline[param_i] == '\0')
+ break;
+
+ if (cmdline[param_i] == '"') {
+ param_i++;
+ is_quoted = !is_quoted;
+ }
+ i = param_i;
+
+ for (; cmdline[param_i]; param_i++) {
+ if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
+ break;
+ if (cmdline[param_i] == '"')
+ is_quoted = !is_quoted;
+ }
+ if (match) {
+ *next = cmdline+param_i;
+ if (cmdline[param_i-1] == '"')
+ *val = g_strndup(cmdline+i, param_i-i-1);
+ else
+ *val = g_strndup(cmdline+i, param_i-i);
+ return 0;
+ }
+ i = param_i;
+ }
+ *next = cmdline+i;
+ return 1;
+}
+
+
+#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \
+ (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \
+ STREQ(kernel_val, caller_val)) || ((flags &
VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \
+ STRPREFIX(kernel_val, caller_val)))
+
+
+/*
+ * Try to match the provided kernel cmdline string with the provided @arg
+ * and the list @values of possible values according to the matching strategy
+ * defined in @flags. Possible options include:
+ * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values
+ * (uses size of value provided as input)
+ * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
+ * (in case of multiple argument occurrences)
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST: use the result of last argument occurence
+ * (in case of multiple argument occurrences)
+ *
+ *
+ * @cmdline: kernel command line string to be checked for @arg
+ * @arg: kernel command line argument
+ * @values: array of possible values to match @arg
+ * @len_values: size of array, it can be 0 meaning a match will be positive if the
+ * argument has no value.
+ * @flags: flag mask defining the strategy for matching and comparing
+ *
+ * Returns true if a match is found, false otherwise
+ */
+bool virKernelCmdlineMatchParam(const char *cmdline,
+ const char *arg,
+ const char **values,
+ size_t len_values,
+ virKernelCmdlineFlags flags)
+{
+ bool match = false;
+ size_t i;
+ const char *next = cmdline;
+ g_autofree char *kval = NULL;
+
+ while (next[0] != '\0') {
+ VIR_FREE(kval);
+ if (virKernelCmdlineGetValue(arg, next, &kval, &next) != 0)
+ break;
+ if (!kval) {
+ match = (len_values == 0) ? true : false;
+ } else {
+ match = false;
+ for (i = 0; i < len_values; i++) {
+ if (VIR_CMDLINE_STR_CMP(kval, values[i], flags)) {
+ match = true;
+ break;
+ }
+ }
+ }
+ if (match && (flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY))
+ break;
+ }
+
+ return match;
+}
+
+
/*
* Get a password from the console input stream.
* The caller must free the returned password.
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 49b4bf440f..1edb415fbe 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -147,6 +147,24 @@ bool virHostHasIOMMU(void);
char *virHostGetDRMRenderNode(void) G_GNUC_NO_INLINE;
+typedef enum {
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX = 1,
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ = 2,
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY = 4,
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST = 8,
+} virKernelCmdlineFlags;
+
+int virKernelCmdlineGetValue(const char *arg,
+ const char *cmdline,
+ char **val,
+ const char **next);
+
+bool virKernelCmdlineMatchParam(const char *cmdline,
+ const char *arg,
+ const char **values,
+ size_t len_values,
+ virKernelCmdlineFlags flags);
+
/**
* VIR_ASSIGN_IS_OVERFLOW:
* @rvalue: value that is checked (evaluated twice)
diff --git a/tests/utiltest.c b/tests/utiltest.c
index 5ae04585cb..1edc6a728e 100644
--- a/tests/utiltest.c
+++ b/tests/utiltest.c
@@ -254,6 +254,134 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED)
}
+struct testKernelCmdlineGetValueData
+{
+ const char *cmdline;
+ const char *arg;
+ int rc;
+ const char *val;
+ size_t next;
+};
+
+static struct testKernelCmdlineGetValueData kEntries[] = {
+ { "arg1 arg2 arg3=val1", "arg4",
1, NULL, 19 },
+ { "arg1=val1 arg2 arg3=val3 arg4", "arg2",
0, NULL, 14 },
+ { "arg1=val1 arg2 arg3=val3 arg4", "arg3",
0, "val3", 24 },
+ { "arg1=val1 arg2 arg-3=val3 arg4", "arg_3",
0, "val3", 25 },
+ { "arg1=val1 arg2 arg_3=val3 arg4", "arg-3",
0, "val3", 25 },
+ { "arg1=val1 arg2 arg_3=val3 arg4", "arg_3",
0, "val3", 25 },
+ { "arg1=val1 arg2 arg-3=val3 arg4", "arg-3",
0, "val3", 25 },
+ { "arg1=val1 arg2=\"value with spaces\" arg3=val3",
"arg2", 0, "value with spaces", 34 },
+ { "arg1=val1 arg2=\"value with spaces\" arg3=val3",
"arg3", 0, "val3", 44 },
+ { "arg1=val1 \"arg2=value with spaces\" arg3=val3",
"arg2", 0, "value with spaces", 34 },
+ { "arg1=val1 \"arg2=value with spaces\" arg3=val3",
"arg3", 0, "val3", 44 },
+ { "arg1=val1 arg2=\"val\"ue arg3",
"arg2", 0, "val\"ue", 22 },
+ { "arg1=val1 arg2=\"val\"ue arg3\" escaped=val2\"",
"arg3\" escaped", 0, "val2", 42 },
+ { "arg1=val1 arg2longer=someval arg2=val2 arg3", "arg2",
0, "val2", 38 },
+};
+
+static int
+testKernelCmdlineGetValue(const void *data G_GNUC_UNUSED)
+{
+ int rc;
+ char *val = NULL;
+ const char *next;
+ size_t i;
+
+ for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) {
+ VIR_FREE(val);
+
+ rc = virKernelCmdlineGetValue(kEntries[i].arg, kEntries[i].cmdline,
+ &val, &next);
+
+ if (rc != kEntries[i].rc || STRNEQ_NULLABLE(val, kEntries[i].val) ||
+ (next - kEntries[i].cmdline) != kEntries[i].next) {
+ VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline);
+ VIR_TEST_DEBUG("Kernel argument [%s]", kEntries[i].arg);
+ VIR_TEST_DEBUG("Expect rc [%d]", kEntries[i].rc);
+ VIR_TEST_DEBUG("Actual rc [%d]", rc);
+ VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val);
+ VIR_TEST_DEBUG("Actual value [%s]", val);
+ VIR_TEST_DEBUG("Expect next index [%lu]", kEntries[i].next);
+ VIR_TEST_DEBUG("Actual next index [%lu]",
+ (size_t)(next - kEntries[i].cmdline));
+
+ VIR_FREE(val);
+
+ return -1;
+ }
+ }
+
+ VIR_FREE(val);
+
+ return 0;
+}
+
+
+struct testKernelCmdlineMatchData
+{
+ const char *cmdline;
+ const char *arg;
+ const char *values[2];
+ virKernelCmdlineFlags flags;
+ bool result;
+};
+
+static struct testKernelCmdlineMatchData kMatchEntries[] = {
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"1", "y"},
VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, false },
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"on", "yes"},
VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true },
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"1", "y"},
VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, true },
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"a", "b"},
VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, false },
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST
| VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, false},
+ {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5",
"myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST
| VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, false},
+ {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5",
"myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST
| VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true },
+ {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5",
"myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST
| VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, true },
+ {"arg1 myarg=no arg2=val2 arg4=val4 myarg arg5",
"myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST,
true },
+ {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5",
"myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY,
true },
+ {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5",
"myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST,
false },
+};
+
+
+static int
+testKernelCmdlineMatchParam(const void *data G_GNUC_UNUSED)
+{
+ bool result;
+ size_t i, lenValues;
+
+ for (i = 0; i < G_N_ELEMENTS(kMatchEntries); ++i) {
+ if (kMatchEntries[i].values[0] == NULL)
+ lenValues = 0;
+ else
+ lenValues = G_N_ELEMENTS(kMatchEntries[i].values);
+
+ result = virKernelCmdlineMatchParam(kMatchEntries[i].cmdline,
+ kMatchEntries[i].arg,
+ kMatchEntries[i].values,
+ lenValues,
+ kMatchEntries[i].flags);
+
+ if (result != kMatchEntries[i].result) {
+ VIR_TEST_DEBUG("\nKernel cmdline [%s]", kMatchEntries[i].cmdline);
+ VIR_TEST_DEBUG("Kernel argument [%s]", kMatchEntries[i].arg);
+ VIR_TEST_DEBUG("Kernel values [%s] [%s]",
kMatchEntries[i].values[0],
+ kMatchEntries[i].values[1]);
+ if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)
+ VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX]");
+ if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)
+ VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ]");
+ if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY)
+ VIR_TEST_DEBUG("Flag
[VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY]");
+ if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST)
+ VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST]");
+ VIR_TEST_DEBUG("Expect result [%d]", kMatchEntries[i].result);
+ VIR_TEST_DEBUG("Actual result [%d]", result);
+
+ return -1;
+ }
+ }
+
+ return 0;
+}
static int
@@ -277,6 +405,8 @@ mymain(void)
DO_TEST(ParseVersionString);
DO_TEST(RoundValueToPowerOfTwo);
DO_TEST(OverflowCheckMacro);
+ DO_TEST(KernelCmdlineGetValue);
+ DO_TEST(KernelCmdlineMatchParam);
return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.25.1
9:19 a.m.
New subject: [PATCH 1/6] util: introduce a parser for kernel cmdline arguments
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Introduce two utility functions to parse a kernel command
line string according to the kernel code parsing rules in
order to enable the caller to perform operations such as
verifying whether certain argument=value combinations are
present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/libvirt_private.syms | 2 +
src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 18 ++++
tests/utiltest.c | 130 +++++++++++++++++++++++++++++
4 files changed, 323 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 935ef7303b..25b22a3e3f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
virHostHasIOMMU;
virIndexToDiskName;
virIsDevMapperDevice;
+virKernelCmdlineGetValue;
+virKernelCmdlineMatchParam;
virMemoryLimitIsSet;
virMemoryLimitTruncate;
virMemoryMaxValue;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index fb46501142..264aae8d01 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
return ret;
}
+
+#define VIR_IS_CMDLINE_SPACE(value) \
+ (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I
think we'd be fine not checking it here either, so plain g_ascii_isspace()
would suffice in the code
+
+
+static bool virKernelArgsAreEqual(const char *arg1,
+ const char *arg2,
+ size_t n)
+{
+ size_t i;
+
+ for (i = 0; i < n; i++) {
+ if ((arg1[i] == '-' && arg2[i] == '_') ||
+ (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] ==
arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than
introducing
another string equality function just because of this, we should normalize the
inputs by replacing occurrences of one with the other and then simply do STREQ
at the appropriate spot in the code
+ if (arg1[i] == '\0')
+ return true;
+ continue;
+ }
+ return false;
+ }
+ return true;
+}
+
+
+/*
+ * Parse the kernel cmdline and store the value of @arg in @val
+ * which can be NULL if @arg has no value or if it's not found.
+ * In addition, store in @next the address right after
+ * @arg=@value for possible further processing.
+ *
+ * @arg: kernel command line argument
+ * @cmdline: kernel command line string to be checked for @arg
+ * @val: pointer to hold retrieved value of @arg
+ * @next: pointer to hold address right after @arg=@val, will
+ * point to the string's end (NULL) in case @arg is not found
+ *
+ * Returns 0 if @arg is present in @cmdline, 1 otherwise
+ */
+int virKernelCmdlineGetValue(const char *arg,
+ const char *cmdline,
+ char **val,
+ const char **next)
+{
+ size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is
more confusing than actually settling down with something like "offset"
especially when you're doing arithmetics with it.
+ size_t arg_len = strlen(arg);
+ bool is_quoted, match;
1 declaration/definition per line please
+
+ *next = cmdline;
+ *val = NULL;
+
+ while (cmdline[i] != '\0') {
+ /* remove leading spaces */
+ while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
+ i++;
For ^this, we already have a primitive virSkipSpaces()
+ if (cmdline[i] == '"') {
+ i++;
+ is_quoted = true;
+ } else {
+ is_quoted = false;
+ }
+ for (param_i = i; cmdline[param_i]; param_i++) {
+ if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
+ cmdline[param_i] == '=')
+ break;
+ if (cmdline[param_i] == '"')
+ is_quoted = !is_quoted;
+ }
+ if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg,
arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
+ match = true;
+ else
+ match = false;
+ /* arg has no value */
+ if (cmdline[param_i] != '=') {
+ if (match) {
+ *next = cmdline+param_i;
please use a space in between the operands and the operator
+ return 0;
+ }
+ i = param_i;
+ continue;
+ }
+ param_i++;
+ if (cmdline[param_i] == '\0')
+ break;
+
+ if (cmdline[param_i] == '"') {
+ param_i++;
+ is_quoted = !is_quoted;
+ }
+ i = param_i;
+
+ for (; cmdline[param_i]; param_i++) {
+ if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
+ break;
+ if (cmdline[param_i] == '"')
+ is_quoted = !is_quoted;
+ }
+ if (match) {
+ *next = cmdline+param_i;
+ if (cmdline[param_i-1] == '"')
+ *val = g_strndup(cmdline+i, param_i-i-1);
+ else
+ *val = g_strndup(cmdline+i, param_i-i);
+ return 0;
+ }
+ i = param_i;
+ }
+ *next = cmdline+i;
+ return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens,
matching the arguments, and extracting parameters and their values. All of that
should be split into helpers to enhance readability. Unfortunately you can't
use our existing tokenizer virStringSplit because of values containing spaces,
so you'll have to write your own that takes that into account, it should return
a token (parameter or parameter=value), then parse the returned token into
key-value pair, match the key with the input key and return whatever is needed
to be returned.
+}
+
+
+#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \
+ (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \
+ STREQ(kernel_val, caller_val)) || ((flags &
VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \
+ STRPREFIX(kernel_val, caller_val)))
+
+
+/*
+ * Try to match the provided kernel cmdline string with the provided @arg
+ * and the list @values of possible values according to the matching strategy
+ * defined in @flags. Possible options include:
+ * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values
+ * (uses size of value provided as input)
+ * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the
parameters sequentially, so the last one wins. We should match that behaviour
when determining the value of a setting and ignore any previous occurrences of
a parameter.
Regards,
Erik
9:31 a.m.
New subject: [PATCH 1/6] util: introduce a parser for kernel cmdline arguments
On 5/15/20 4:19 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>
> Introduce two utility functions to parse a kernel command
> line string according to the kernel code parsing rules in
> order to enable the caller to perform operations such as
> verifying whether certain argument=value combinations are
> present or retrieving an argument's value.
>
> Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> src/libvirt_private.syms | 2 +
> src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 18 ++++
> tests/utiltest.c | 130 +++++++++++++++++++++++++++++
> 4 files changed, 323 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 935ef7303b..25b22a3e3f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
> virHostHasIOMMU;
> virIndexToDiskName;
> virIsDevMapperDevice;
> +virKernelCmdlineGetValue;
> +virKernelCmdlineMatchParam;
> virMemoryLimitIsSet;
> virMemoryLimitTruncate;
> virMemoryMaxValue;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index fb46501142..264aae8d01 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
> return ret;
> }
>
> +
> +#define VIR_IS_CMDLINE_SPACE(value) \
> + (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I
think we'd be fine not checking it here either, so plain g_ascii_isspace()
would suffice in the code
Paulo wanted to make sure the function would always produce the same
result as the kernel code itself and the kernel code recognizes
character 160 as space.
In case you'd like to check that, see lib/cmdline.c -> function next_arg
-> isspace -> include/linux/ctype.h -> __ismask & _S -> lib/ctype.c has
a table with the entries for _S, including the character 160 which
stands for "non breaking space" character.
Anyway since it seems an unlikely case I removed it as requested.
> +
> +
> +static bool virKernelArgsAreEqual(const char *arg1,
> + const char *arg2,
> + size_t n)
> +{
> + size_t i;
> +
> + for (i = 0; i < n; i++) {
> + if ((arg1[i] == '-' && arg2[i] == '_') ||
> + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i]
== arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than
introducing
another string equality function just because of this, we should normalize the
inputs by replacing occurrences of one with the other and then simply do STREQ
at the appropriate spot in the code
While reworking the parsing it has been changed.
> + if (arg1[i] == '\0')
> + return true;
> + continue;
> + }
> + return false;
> + }
> + return true;
> +}
> +
> +
> +/*
> + * Parse the kernel cmdline and store the value of @arg in @val
> + * which can be NULL if @arg has no value or if it's not found.
> + * In addition, store in @next the address right after
> + * @arg=@value for possible further processing.
> + *
> + * @arg: kernel command line argument
> + * @cmdline: kernel command line string to be checked for @arg
> + * @val: pointer to hold retrieved value of @arg
> + * @next: pointer to hold address right after @arg=@val, will
> + * point to the string's end (NULL) in case @arg is not found
> + *
> + * Returns 0 if @arg is present in @cmdline, 1 otherwise
> + */
> +int virKernelCmdlineGetValue(const char *arg,
> + const char *cmdline,
> + char **val,
> + const char **next)
> +{
> + size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is
more confusing than actually settling down with something like "offset"
especially when you're doing arithmetics with it.
Changed it.
> + size_t arg_len = strlen(arg);
> + bool is_quoted, match;
1 declaration/definition per line please
Changed it.
> +
> + *next = cmdline;
> + *val = NULL;
> +
> + while (cmdline[i] != '\0') {
> + /* remove leading spaces */
> + while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
> + i++;
For ^this, we already have a primitive virSkipSpaces()
Changed it.
> + if (cmdline[i] == '"') {
> + i++;
> + is_quoted = true;
> + } else {
> + is_quoted = false;
> + }
> + for (param_i = i; cmdline[param_i]; param_i++) {
> + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
> + cmdline[param_i] == '=')
> + break;
> + if (cmdline[param_i] == '"')
> + is_quoted = !is_quoted;
> + }
> + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg,
arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
Corrected
> + match = true;
> + else
> + match = false;
> + /* arg has no value */
> + if (cmdline[param_i] != '=') {
> + if (match) {
> + *next = cmdline+param_i;
please use a space in between the operands and the operator
Corrected
> + return 0;
> + }
> + i = param_i;
> + continue;
> + }
> + param_i++;
> + if (cmdline[param_i] == '\0')
> + break;
> +
> + if (cmdline[param_i] == '"') {
> + param_i++;
> + is_quoted = !is_quoted;
> + }
> + i = param_i;
> +
> + for (; cmdline[param_i]; param_i++) {
> + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
> + break;
> + if (cmdline[param_i] == '"')
> + is_quoted = !is_quoted;
> + }
> + if (match) {
> + *next = cmdline+param_i;
> + if (cmdline[param_i-1] == '"')
> + *val = g_strndup(cmdline+i, param_i-i-1);
> + else
> + *val = g_strndup(cmdline+i, param_i-i);
> + return 0;
> + }
> + i = param_i;
> + }
> + *next = cmdline+i;
> + return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens,
matching the arguments, and extracting parameters and their values. All of that
should be split into helpers to enhance readability. Unfortunately you can't
use our existing tokenizer virStringSplit because of values containing spaces,
so you'll have to write your own that takes that into account, it should return
a token (parameter or parameter=value), then parse the returned token into
key-value pair, match the key with the input key and return whatever is needed
to be returned.
v2 will have a reworked approach which is more like you outlined above.
> +}
> +
> +
> +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \
> + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \
> + STREQ(kernel_val, caller_val)) || ((flags &
VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \
> + STRPREFIX(kernel_val, caller_val)))
> +
> +
> +/*
> + * Try to match the provided kernel cmdline string with the provided @arg
> + * and the list @values of possible values according to the matching strategy
> + * defined in @flags. Possible options include:
> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values
> + * (uses size of value provided as input)
> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
> + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the
parameters sequentially, so the last one wins. We should match that behaviour
when determining the value of a setting and ignore any previous occurrences of
a parameter.
Regards,
Erik
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
9:50 a.m.
New subject: [PATCH 1/6] util: introduce a parser for kernel cmdline arguments
On 15/05/20 16:19, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>
> Introduce two utility functions to parse a kernel command
> line string according to the kernel code parsing rules in
> order to enable the caller to perform operations such as
> verifying whether certain argument=value combinations are
> present or retrieving an argument's value.
>
> Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> src/libvirt_private.syms | 2 +
> src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 18 ++++
> tests/utiltest.c | 130 +++++++++++++++++++++++++++++
> 4 files changed, 323 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 935ef7303b..25b22a3e3f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
> virHostHasIOMMU;
> virIndexToDiskName;
> virIsDevMapperDevice;
> +virKernelCmdlineGetValue;
> +virKernelCmdlineMatchParam;
> virMemoryLimitIsSet;
> virMemoryLimitTruncate;
> virMemoryMaxValue;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index fb46501142..264aae8d01 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
> return ret;
> }
>
> +
> +#define VIR_IS_CMDLINE_SPACE(value) \
> + (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I
think we'd be fine not checking it here either, so plain g_ascii_isspace()
would suffice in the code
> +
> +
> +static bool virKernelArgsAreEqual(const char *arg1,
> + const char *arg2,
> + size_t n)
> +{
> + size_t i;
> +
> + for (i = 0; i < n; i++) {
> + if ((arg1[i] == '-' && arg2[i] == '_') ||
> + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i]
== arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than
introducing
another string equality function just because of this, we should normalize the
inputs by replacing occurrences of one with the other and then simply do STREQ
at the appropriate spot in the code
> + if (arg1[i] == '\0')
> + return true;
> + continue;
> + }
> + return false;
> + }
> + return true;
> +}
> +
> +
> +/*
> + * Parse the kernel cmdline and store the value of @arg in @val
> + * which can be NULL if @arg has no value or if it's not found.
> + * In addition, store in @next the address right after
> + * @arg=@value for possible further processing.
> + *
> + * @arg: kernel command line argument
> + * @cmdline: kernel command line string to be checked for @arg
> + * @val: pointer to hold retrieved value of @arg
> + * @next: pointer to hold address right after @arg=@val, will
> + * point to the string's end (NULL) in case @arg is not found
> + *
> + * Returns 0 if @arg is present in @cmdline, 1 otherwise
> + */
> +int virKernelCmdlineGetValue(const char *arg,
> + const char *cmdline,
> + char **val,
> + const char **next)
> +{
> + size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is
more confusing than actually settling down with something like "offset"
especially when you're doing arithmetics with it.
> + size_t arg_len = strlen(arg);
> + bool is_quoted, match;
1 declaration/definition per line please
> +
> + *next = cmdline;
> + *val = NULL;
> +
> + while (cmdline[i] != '\0') {
> + /* remove leading spaces */
> + while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
> + i++;
For ^this, we already have a primitive virSkipSpaces()
> + if (cmdline[i] == '"') {
> + i++;
> + is_quoted = true;
> + } else {
> + is_quoted = false;
> + }
> + for (param_i = i; cmdline[param_i]; param_i++) {
> + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
> + cmdline[param_i] == '=')
> + break;
> + if (cmdline[param_i] == '"')
> + is_quoted = !is_quoted;
> + }
> + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg,
arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
> + match = true;
> + else
> + match = false;
> + /* arg has no value */
> + if (cmdline[param_i] != '=') {
> + if (match) {
> + *next = cmdline+param_i;
please use a space in between the operands and the operator
> + return 0;
> + }
> + i = param_i;
> + continue;
> + }
> + param_i++;
> + if (cmdline[param_i] == '\0')
> + break;
> +
> + if (cmdline[param_i] == '"') {
> + param_i++;
> + is_quoted = !is_quoted;
> + }
> + i = param_i;
> +
> + for (; cmdline[param_i]; param_i++) {
> + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
> + break;
> + if (cmdline[param_i] == '"')
> + is_quoted = !is_quoted;
> + }
> + if (match) {
> + *next = cmdline+param_i;
> + if (cmdline[param_i-1] == '"')
> + *val = g_strndup(cmdline+i, param_i-i-1);
> + else
> + *val = g_strndup(cmdline+i, param_i-i);
> + return 0;
> + }
> + i = param_i;
> + }
> + *next = cmdline+i;
> + return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens,
matching the arguments, and extracting parameters and their values. All of that
should be split into helpers to enhance readability. Unfortunately you can't
use our existing tokenizer virStringSplit because of values containing spaces,
so you'll have to write your own that takes that into account, it should return
a token (parameter or parameter=value), then parse the returned token into
key-value pair, match the key with the input key and return whatever is needed
to be returned.
> +}
> +
> +
> +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \
> + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \
> + STREQ(kernel_val, caller_val)) || ((flags &
VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \
> + STRPREFIX(kernel_val, caller_val)))
> +
> +
> +/*
> + * Try to match the provided kernel cmdline string with the provided @arg
> + * and the list @values of possible values according to the matching strategy
> + * defined in @flags. Possible options include:
> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values
> + * (uses size of value provided as input)
> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
> + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the
parameters sequentially, so the last one wins. We should match that behaviour
when determining the value of a setting and ignore any previous occurrences of
a parameter.
It's not always the case that the last one wins. In the case of the
prot_virt parameter for example as soon as a positive value (prot_virt=1
or prot_virt=y) shows up in the kernel cmdline the feature will be
enabled, even if the last entry is negative. In order to handle the two
possible behaviors we introduced the *_SEARCH flags SEARCH_STICKY
(prot_virt behavior) and SEARCH_LAST (usual behavior, last one wins).
As to the *_CMP flags, prot_virt again accepts multiple values and does
a prefix-based check (see arch/s390/kernel/uv.c -> prot_virt_setup ->
kstrtobool) so we introduced CMP_PREFIX to handle that, while CMP_EQ is
for the usual case when the whole value is considered.
Regards,
Erik
--
Best regards,
Paulo de Rezende Pinatti
11:41 a.m.
New subject: [PATCH 2/6] qemu: check if s390 secure guest support is enabled
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
This patch introduces a common function to verify if the
availability of the so-called Secure Guest feature on the host
has changed in order to invalidate the qemu capabilities cache.
It can be used as an entry point for verification on different
architectures.
For s390 the verification consists of:
- checking if /sys/firmware/uv is available: meaning the HW
facility is available and the host OS supports it;
- checking if the kernel cmdline contains 'prot_virt=1': meaning
the host OS wants to use the feature.
Whenever the availability of the feature does not match the secure
guest flag in the cache then libvirt will re-build it in order to
pick up the new set of capabilities available.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Tested-by: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/qemu/qemu_capabilities.c | 56 ++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2c6e36685e..2874bb1e7c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -23,6 +23,7 @@
#include "qemu_capabilities.h"
#include "viralloc.h"
+#include "virarch.h"
#include "vircrypto.h"
#include "virlog.h"
#include "virerror.h"
@@ -654,6 +655,7 @@ struct _virQEMUCaps {
virObject parent;
bool kvmSupportsNesting;
+ bool kvmSupportsSecureGuest;
char *binary;
time_t ctime;
@@ -1864,6 +1866,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
ret->invalidation = qemuCaps->invalidation;
ret->kvmSupportsNesting = qemuCaps->kvmSupportsNesting;
+ ret->kvmSupportsSecureGuest = qemuCaps->kvmSupportsSecureGuest;
ret->ctime = qemuCaps->ctime;
@@ -4302,6 +4305,9 @@ virQEMUCapsLoadCache(virArch hostArch,
if (virXPathBoolean("boolean(./kvmSupportsNesting)", ctxt) > 0)
qemuCaps->kvmSupportsNesting = true;
+ if (virXPathBoolean("boolean(./kvmSupportsSecureGuest)", ctxt) > 0)
+ qemuCaps->kvmSupportsSecureGuest = true;
+
ret = 0;
cleanup:
VIR_FREE(str);
@@ -4535,6 +4541,9 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
if (qemuCaps->kvmSupportsNesting)
virBufferAddLit(&buf, "<kvmSupportsNesting/>\n");
+ if (qemuCaps->kvmSupportsSecureGuest)
+ virBufferAddLit(&buf, "<kvmSupportsSecureGuest/>\n");
+
virBufferAdjustIndent(&buf, -2);
virBufferAddLit(&buf, "</qemuCaps>\n");
@@ -4572,6 +4581,44 @@ virQEMUCapsSaveFile(void *data,
}
+/*
+ * Check whether IBM Secure Execution (S390) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestS390(void)
+{
+
+ g_autofree char *cmdline = NULL;
+ static const char *kValues[] = {"y", "Y", "1"};
+
+ if (!virFileIsDir("/sys/firmware/uv"))
+ return false;
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return false;
+ if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kValues,
+ G_N_ELEMENTS(kValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX))
+ return true;
+ return false;
+}
+
+
+/*
+ * Check whether the secure guest functionality is enabled.
+ * See the specific architecture function for details on the verifications made.
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuest(void)
+{
+ virArch arch = virArchFromHost();
+
+ if (ARCH_IS_S390(arch))
+ return virQEMUCapsKVMSupportsSecureGuestS390();
+ return false;
+}
+
+
/* Check the kernel module parameters 'nested' file to determine if enabled
*
* Intel: 'kvm_intel' uses 'Y'
@@ -4750,6 +4797,13 @@ virQEMUCapsIsValid(void *data,
qemuCaps->binary, qemuCaps->kvmSupportsNesting);
return false;
}
+
+ if (virQEMUCapsKVMSupportsSecureGuest() != qemuCaps->kvmSupportsSecureGuest)
{
+ VIR_DEBUG("Outdated capabilities for '%s': kvm kernel secure
guest "
+ "value changed from %d",
+ qemuCaps->binary, qemuCaps->kvmSupportsSecureGuest);
+ return false;
+ }
}
return true;
@@ -5236,6 +5290,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
qemuCaps->kernelVersion = g_strdup(kernelVersion);
qemuCaps->kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
+
+ qemuCaps->kvmSupportsSecureGuest = virQEMUCapsKVMSupportsSecureGuest();
}
return qemuCaps;
--
2.25.1
11:41 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted
Virtualization) in order to invalidate the qemu capabilities
cache in case the availability of the feature changed.
For AMD SEV the verification consists of:
- checking if /sys/module/kvm_amd/parameters/sev contains the
value '1': meaning SEV is enabled in the host kernel;
- checking if the kernel cmdline contains 'mem_encrypt=on': meaning
SME memory encryption feature on the host is enabled
Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
docs/kbase/launch_security_sev.rst | 2 +-
src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
index 65f258587d..fa602c7432 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -109,7 +109,7 @@ following:
</features>
</domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before
+Note that if libvirt (<6.4.0) was already installed and libvirtd running before
enabling SEV in the kernel followed by the host reboot you need to force
libvirtd to re-probe both the host and QEMU capabilities. First stop
libvirtd:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2874bb1e7c..6cf926d52d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
}
+/*
+ * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestAMD(void)
+{
+ g_autofree char *cmdline = NULL;
+ g_autofree char *modValue = NULL;
+ static const char *kValues[] = {"on"};
+
+ if (virFileReadValueString(&modValue,
"/sys/module/kvm_amd/parameters/sev") < 0)
+ return false;
+ if (modValue[0] != '1')
+ return false;
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return false;
+ if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
+ G_N_ELEMENTS(kValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
+ return true;
+ return false;
+}
+
+
/*
* Check whether the secure guest functionality is enabled.
* See the specific architecture function for details on the verifications made.
@@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch))
return virQEMUCapsKVMSupportsSecureGuestS390();
+ if (ARCH_IS_X86(arch))
+ return virQEMUCapsKVMSupportsSecureGuestAMD();
return false;
}
--
2.25.1
2:40 p.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted
Virtualization) in order to invalidate the qemu capabilities
cache in case the availability of the feature changed.
For AMD SEV the verification consists of:
- checking if /sys/module/kvm_amd/parameters/sev contains the
value '1': meaning SEV is enabled in the host kernel;
- checking if the kernel cmdline contains 'mem_encrypt=on': meaning
SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to
check whether QEMU supports the feature. e.g, what if user has newer
kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the
SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set
2) check if /dev/sev device exist (aka firmware is detected)
3) Check if Qemu supports SEV feature (maybe we can detect the existence
of the query-sev QMP command or detect Qemu version >= 2.12)
thanks
Signed-off-by: Paulo de Rezende Pinatti
<ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
docs/kbase/launch_security_sev.rst | 2 +-
src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
index 65f258587d..fa602c7432 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -109,7 +109,7 @@ following:
</features>
</domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before
+Note that if libvirt (<6.4.0) was already installed and libvirtd running before
enabling SEV in the kernel followed by the host reboot you need to force
libvirtd to re-probe both the host and QEMU capabilities. First stop
libvirtd:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2874bb1e7c..6cf926d52d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
}
+/*
+ * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestAMD(void)
+{
+ g_autofree char *cmdline = NULL;
+ g_autofree char *modValue = NULL;
+ static const char *kValues[] = {"on"};
+
+ if (virFileReadValueString(&modValue,
"/sys/module/kvm_amd/parameters/sev") < 0)
+ return false;
+ if (modValue[0] != '1')
+ return false;
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return false;
+ if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
+ G_N_ELEMENTS(kValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
+ return true;
+ return false;
+}
+
+
/*
* Check whether the secure guest functionality is enabled.
* See the specific architecture function for details on the verifications made.
@@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch))
return virQEMUCapsKVMSupportsSecureGuestS390();
+ if (ARCH_IS_X86(arch))
+ return virQEMUCapsKVMSupportsSecureGuestAMD();
return false;
}
2:08 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/11/20 9:40 PM, Brijesh Singh wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>
> Implement secure guest check for AMD SEV (Secure Encrypted
> Virtualization) in order to invalidate the qemu capabilities
> cache in case the availability of the feature changed.
>
> For AMD SEV the verification consists of:
> - checking if /sys/module/kvm_amd/parameters/sev contains the
> value '1': meaning SEV is enabled in the host kernel;
> - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
> SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to
check whether QEMU supports the feature. e.g, what if user has newer
kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the
SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set
Paulo implemented the checks following the
documentation in
docs/kbase/launch_security_sev.rst. The check for the module parameter
sev is included. Is the check for the kernel cmdline parameter
"mem_encrypt" not necessary? Or would that be covered by your suggested
check in 2)?
2) check if /dev/sev device exist (aka firmware is detected)
This seems reasonable.
Shouldn't it have been documented in
docs/kbase/launch_security_sev.rst?
3) Check if Qemu supports SEV feature (maybe we can detect the existence
of the query-sev QMP command or detect Qemu version >= 2.12)
The idea is to
check the host capabilities to detect if qemus
capabilities need to be reprobed. Therefore this would be a result if
checks 1+2 change but it would not be a cache invalidation trigger.
thanks
> Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> docs/kbase/launch_security_sev.rst | 2 +-
> src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
> index 65f258587d..fa602c7432 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -109,7 +109,7 @@ following:
> </features>
> </domainCapabilities>
>
> -Note that if libvirt was already installed and libvirtd running before
> +Note that if libvirt (<6.4.0) was already installed and libvirtd running before
> enabling SEV in the kernel followed by the host reboot you need to force
> libvirtd to re-probe both the host and QEMU capabilities. First stop
> libvirtd:
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2874bb1e7c..6cf926d52d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
> }
>
>
> +/*
> + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
> + */
> +static bool
> +virQEMUCapsKVMSupportsSecureGuestAMD(void)
> +{
> + g_autofree char *cmdline = NULL;
> + g_autofree char *modValue = NULL;
> + static const char *kValues[] = {"on"};
> +
> + if (virFileReadValueString(&modValue,
"/sys/module/kvm_amd/parameters/sev") < 0)
> + return false;
> + if (modValue[0] != '1')
> + return false;
> + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
> + return false;
> + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
> + G_N_ELEMENTS(kValues),
> + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
> + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
> + return true;
> + return false;
> +}
> +
> +
> /*
> * Check whether the secure guest functionality is enabled.
> * See the specific architecture function for details on the verifications made.
> @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
>
> if (ARCH_IS_S390(arch))
> return virQEMUCapsKVMSupportsSecureGuestS390();
> + if (ARCH_IS_X86(arch))
> + return virQEMUCapsKVMSupportsSecureGuestAMD();
> return false;
> }
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:32 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
On 5/11/20 9:40 PM, Brijesh Singh wrote:
> Thanks for the patch Paulo, Few comments.
>
> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
> > From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> >
> > Implement secure guest check for AMD SEV (Secure Encrypted
> > Virtualization) in order to invalidate the qemu capabilities
> > cache in case the availability of the feature changed.
> >
> > For AMD SEV the verification consists of:
> > - checking if /sys/module/kvm_amd/parameters/sev contains the
> > value '1': meaning SEV is enabled in the host kernel;
> > - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
> > SME memory encryption feature on the host is enabled
>
>
> In addition to the kernel module parameter, we probably also need to
> check whether QEMU supports the feature. e.g, what if user has newer
> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the
> SEV feature support, we should verify the following conditions:
>
> 1) check kernel module parameter is set
Paulo implemented the checks following the documentation in
docs/kbase/launch_security_sev.rst. The check for the module parameter sev
is included. Is the check for the kernel cmdline parameter "mem_encrypt" not
necessary? Or would that be covered by your suggested check in 2)?
Brijesh correct me here please, but IIRC having mem_encrypt set is merely
a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW
SEV should work without SME. If my memory serves well here, we don't need
and also should not parse the kernel cmdline in this case.
>
> 2) check if /dev/sev device exist (aka firmware is detected)
This seems reasonable. Shouldn't it have been documented in
docs/kbase/launch_security_sev.rst?
Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can
you have the kernel module parameter set to 1 and yet kernel doesn't expose the
/dev/sev node?
>
> 3) Check if Qemu supports SEV feature (maybe we can detect the existence
> of the query-sev QMP command or detect Qemu version >= 2.12)
^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus
capabilities
need to be reprobed. Therefore this would be a result if checks 1+2 change
but it would not be a cache invalidation trigger.
I agree in the sense that the SEV support that is currently being reported in
QEMU capabilities wasn't a good choice because it reports only platform data
which are constant to the host and have nothing to do with QEMU. It would be
okay to just say <sev supported="yes|no"/> in qemu capabilities and report
the
rest in host capabilities. I haven't added this info into host capabilities
when I realized the mistake because I didn't want to duplicate the platform
info on 2 places. For the IBM secure guest feature, it makes total sense to
report support within host capabilities, I'm just not sure whether we should do
the same for SEV and try really hard to "fix" the mess.
Regards,
--
Erik Skultety
11:39 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/12/20 5:32 PM, Erik Skultety wrote:
I leave 1) and 2) to the AMD experts... :-)
>> 3) Check if Qemu supports SEV feature (maybe we can detect
the existence
>> of the query-sev QMP command or detect Qemu version >= 2.12)
^This is already achieved by qemuMonitorJSONGetSEVCapabilities
> The idea is to check the host capabilities to detect if qemus capabilities
> need to be reprobed. Therefore this would be a result if checks 1+2 change
> but it would not be a cache invalidation trigger.
I agree in the sense that the SEV support that is currently being reported in
QEMU capabilities wasn't a good choice because it reports only platform data
which are constant to the host and have nothing to do with QEMU. It would be
okay to just say <sev supported="yes|no"/> in qemu capabilities and
report the
rest in host capabilities. I haven't added this info into host capabilities
when I realized the mistake because I didn't want to duplicate the platform
info on 2 places. For the IBM secure guest feature, it makes total sense to
report support within host capabilities, I'm just not sure whether we should do
the same for SEV and try really hard to "fix" the mess.
I am not sure I understand the "mess" correctly.
The idea is to prevent the cached qemu capabilities becoming stale which
Daniel PB has called earlier a bug in the caching algorithm. This can
occur if the kernel or firmware configuration change. To find out these
situations we try to implement qemu capability cache invalidation
triggers. The monitor method you mentioned is called during a (re-)probe
and for IBM Secure Execution the host CPU model is also updated during a
(re-)probe. Wouldn't that clean up the "mess"?
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
12:52 p.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/12/20 10:32 AM, Erik Skultety wrote:
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
> On 5/11/20 9:40 PM, Brijesh Singh wrote:
>> Thanks for the patch Paulo, Few comments.
>>
>> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
>>> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>>>
>>> Implement secure guest check for AMD SEV (Secure Encrypted
>>> Virtualization) in order to invalidate the qemu capabilities
>>> cache in case the availability of the feature changed.
>>>
>>> For AMD SEV the verification consists of:
>>> - checking if /sys/module/kvm_amd/parameters/sev contains the
>>> value '1': meaning SEV is enabled in the host kernel;
>>> - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
>>> SME memory encryption feature on the host is enabled
>>
>> In addition to the kernel module parameter, we probably also need to
>> check whether QEMU supports the feature. e.g, what if user has newer
>> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the
>> SEV feature support, we should verify the following conditions:
>>
>> 1) check kernel module parameter is set
> Paulo implemented the checks following the documentation in
> docs/kbase/launch_security_sev.rst. The check for the module parameter sev
> is included. Is the check for the kernel cmdline parameter "mem_encrypt"
not
> necessary? Or would that be covered by your suggested check in 2)?
Brijesh correct me here please, but IIRC having mem_encrypt set is merely
a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW
SEV should work without SME. If my memory serves well here, we don't need
and also should not parse the kernel cmdline in this case.
Yes, that is correct. mem_encrypt=on is not required for the SEV to
work. The mem_encrypt=on option is meant to enable the SME feature on
the host machine. In addition to the guest, if customer wants to protect
the Hypervsior memory from physical attacks then they can use SME or
TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can
be enabled in the BIOS and does not require any kernel changes. AMD is
mostly recommending TSME to protect against the physical attacks.
>> 2) check if /dev/sev device exist (aka firmware is detected)
> This seems reasonable. Shouldn't it have been documented in
> docs/kbase/launch_security_sev.rst?
Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can
you have the kernel module parameter set to 1 and yet kernel doesn't expose the
/dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the
firmware during the feature probe (i.e does not access the /dev/sev).
The firmware initialization is delayed until the first guest launch. So
only sane way to know whether firmware is been detected is check the
existence of the /dev/sev or issue a query-sev command . The query-sev
command will send the platform_status request to the firmware, if the
firmware is not ready then this command will fail.
>> 3) Check if Qemu supports SEV feature (maybe we can detect
the existence
>> of the query-sev QMP command or detect Qemu version >= 2.12)
^This is already achieved by qemuMonitorJSONGetSEVCapabilities
> The idea is to check the host capabilities to detect if qemus capabilities
> need to be reprobed. Therefore this would be a result if checks 1+2 change
> but it would not be a cache invalidation trigger.
I agree in the sense that the SEV support that is currently being reported in
QEMU capabilities wasn't a good choice because it reports only platform data
which are constant to the host and have nothing to do with QEMU. It would be
okay to just say <sev supported="yes|no"/> in qemu capabilities and
report the
rest in host capabilities. I haven't added this info into host capabilities
when I realized the mistake because I didn't want to duplicate the platform
info on 2 places. For the IBM secure guest feature, it makes total sense to
report support within host capabilities, I'm just not sure whether we should do
the same for SEV and try really hard to "fix" the mess.
Regards,
1:21 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
>>> 2) check if /dev/sev device exist (aka firmware is
detected)
>> This seems reasonable. Shouldn't it have been documented in
>> docs/kbase/launch_security_sev.rst?
> Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can
> you have the kernel module parameter set to 1 and yet kernel doesn't expose the
> /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the
firmware during the feature probe (i.e does not access the /dev/sev).
The firmware initialization is delayed until the first guest launch. So
only sane way to know whether firmware is been detected is check the
existence of the /dev/sev or issue a query-sev command . The query-sev
command will send the platform_status request to the firmware, if the
firmware is not ready then this command will fail.
I see. Can query-sev fail or return that it's disabled, aka {"enabled":
false,...} in the SevInfo QMP response, but at the same time succeed in
returning the platform capabilities via query-sev-capabilities? I'm asking,
because libvirt only issues the latter to fill in the QEMU capabilities
structure.
--
Erik Skultety
11:26 p.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/13/20 1:21 AM, Erik Skultety wrote:
>>>> 2) check if /dev/sev device exist (aka firmware is
detected)
>>> This seems reasonable. Shouldn't it have been documented in
>>> docs/kbase/launch_security_sev.rst?
>> Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can
>> you have the kernel module parameter set to 1 and yet kernel doesn't expose
the
>> /dev/sev node?
>
> Currently, 1 does not imply 2, KVM driver does not initialize the
> firmware during the feature probe (i.e does not access the /dev/sev).
> The firmware initialization is delayed until the first guest launch. So
> only sane way to know whether firmware is been detected is check the
> existence of the /dev/sev or issue a query-sev command . The query-sev
> command will send the platform_status request to the firmware, if the
> firmware is not ready then this command will fail.
I see. Can query-sev fail or return that it's disabled, aka {"enabled":
false,...} in the SevInfo QMP response, but at the same time succeed in
returning the platform capabilities via query-sev-capabilities? I'm asking,
because libvirt only issues the latter to fill in the QEMU capabilities
structure.
Just looked at qemu code, If /dev/sev does not exist then
query-sev-capabilities should fail, and if SEV is not enabled in the
guest then query-sev should returns false. So, basically what libvirt is
doing correct, it should be using query-sev-capabilities to populate
QEMU capabilities. It was my bad, I should have mentioned the
query-sev-capabilities and not the query-sev check.
thanks
2:06 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On Wed, May 13, 2020 at 11:26:52PM -0500, Brijesh Singh wrote:
On 5/13/20 1:21 AM, Erik Skultety wrote:
>>>>> 2) check if /dev/sev device exist (aka firmware is detected)
>>>> This seems reasonable. Shouldn't it have been documented in
>>>> docs/kbase/launch_security_sev.rst?
>>> Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW
can
>>> you have the kernel module parameter set to 1 and yet kernel doesn't
expose the
>>> /dev/sev node?
>>
>> Currently, 1 does not imply 2, KVM driver does not initialize the
>> firmware during the feature probe (i.e does not access the /dev/sev).
>> The firmware initialization is delayed until the first guest launch. So
>> only sane way to know whether firmware is been detected is check the
>> existence of the /dev/sev or issue a query-sev command . The query-sev
>> command will send the platform_status request to the firmware, if the
>> firmware is not ready then this command will fail.
> I see. Can query-sev fail or return that it's disabled, aka
{"enabled":
> false,...} in the SevInfo QMP response, but at the same time succeed in
> returning the platform capabilities via query-sev-capabilities? I'm asking,
> because libvirt only issues the latter to fill in the QEMU capabilities
> structure.
Just looked at qemu code, If /dev/sev does not exist then
query-sev-capabilities should fail, and if SEV is not enabled in the
guest then query-sev should returns false. So, basically what libvirt is
doing correct, it should be using query-sev-capabilities to populate
QEMU capabilities. It was my bad, I should have mentioned the
query-sev-capabilities and not the query-sev check.
Perfect, now it makes much more sense to me after quite a period not closely
following this feature, thanks Brijesh.
Boris, I'll start with the review today, but for the IBM part, I'm not sure I
can get my hands on the necessary s390 HW (I'm also not familiar with s390), so
I can do code-only review here, but at least for the AMD bits I do have access
to the necessary HW and apart from scanning the filesystem (or parsing the
kernel cmdline) the outcome is the same, so I'll do some mirroring in the
review process.
Regards,
--
Erik Skultety
5:40 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/12/20 7:52 PM, Brijesh Singh wrote:
On 5/12/20 10:32 AM, Erik Skultety wrote:
> On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
>> On 5/11/20 9:40 PM, Brijesh Singh wrote:
>>> Thanks for the patch Paulo, Few comments.
>>>
>>> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
>>>> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>>>>
>>>> Implement secure guest check for AMD SEV (Secure Encrypted
>>>> Virtualization) in order to invalidate the qemu capabilities
>>>> cache in case the availability of the feature changed.
>>>>
>>>> For AMD SEV the verification consists of:
>>>> - checking if /sys/module/kvm_amd/parameters/sev contains the
>>>> value '1': meaning SEV is enabled in the host kernel;
>>>> - checking if the kernel cmdline contains 'mem_encrypt=on':
meaning
>>>> SME memory encryption feature on the host is enabled
>>>
>>> In addition to the kernel module parameter, we probably also need to
>>> check whether QEMU supports the feature. e.g, what if user has newer
>>> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check
the
>>> SEV feature support, we should verify the following conditions:
>>>
>>> 1) check kernel module parameter is set
>> Paulo implemented the checks following the documentation in
>> docs/kbase/launch_security_sev.rst. The check for the module parameter sev
>> is included. Is the check for the kernel cmdline parameter
"mem_encrypt" not
>> necessary? Or would that be covered by your suggested check in 2)?
> Brijesh correct me here please, but IIRC having mem_encrypt set is merely
> a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW
> SEV should work without SME. If my memory serves well here, we don't need
> and also should not parse the kernel cmdline in this case.
>
Yes, that is correct. mem_encrypt=on is not required for the SEV to
work. The mem_encrypt=on option is meant to enable the SME feature on
the host machine. In addition to the guest, if customer wants to protect
the Hypervsior memory from physical attacks then they can use SME or
TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can
be enabled in the BIOS and does not require any kernel changes. AMD is
mostly recommending TSME to protect against the physical attacks.
>>> 2) check if /dev/sev device exist (aka firmware is detected)
>> This seems reasonable. Shouldn't it have been documented in
>> docs/kbase/launch_security_sev.rst?
> Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can
> you have the kernel module parameter set to 1 and yet kernel doesn't expose the
> /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the
firmware during the feature probe (i.e does not access the /dev/sev).
The firmware initialization is delayed until the first guest launch. So
only sane way to know whether firmware is been detected is check the
existence of the /dev/sev or issue a query-sev command . The query-sev
command will send the platform_status request to the firmware, if the
firmware is not ready then this command will fail.
Just to summarize the checks you want implemented for AMD SEV support:
1) check kernel module parameter is set (as already implemented)
2) check if /dev/sev device exist
Please note that these checks will also go into virt-host-validate in
patch 5.
>>> 3) Check if Qemu supports SEV feature (maybe we can
detect the existence
>>> of the query-sev QMP command or detect Qemu version >= 2.12)
> ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
>
>> The idea is to check the host capabilities to detect if qemus capabilities
>> need to be reprobed. Therefore this would be a result if checks 1+2 change
>> but it would not be a cache invalidation trigger.
> I agree in the sense that the SEV support that is currently being reported in
> QEMU capabilities wasn't a good choice because it reports only platform data
> which are constant to the host and have nothing to do with QEMU. It would be
> okay to just say <sev supported="yes|no"/> in qemu capabilities and
report the
> rest in host capabilities. I haven't added this info into host capabilities
> when I realized the mistake because I didn't want to duplicate the platform
> info on 2 places. For the IBM secure guest feature, it makes total sense to
> report support within host capabilities, I'm just not sure whether we should do
> the same for SEV and try really hard to "fix" the mess.
>
> Regards,
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:27 p.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/13/20 5:40 AM, Boris Fiuczynski wrote:
On 5/12/20 7:52 PM, Brijesh Singh wrote:
>
> On 5/12/20 10:32 AM, Erik Skultety wrote:
>> On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
>>> On 5/11/20 9:40 PM, Brijesh Singh wrote:
>>>> Thanks for the patch Paulo, Few comments.
>>>>
>>>> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
>>>>> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>>>>>
>>>>> Implement secure guest check for AMD SEV (Secure Encrypted
>>>>> Virtualization) in order to invalidate the qemu capabilities
>>>>> cache in case the availability of the feature changed.
>>>>>
>>>>> For AMD SEV the verification consists of:
>>>>> - checking if /sys/module/kvm_amd/parameters/sev contains the
>>>>> value '1': meaning SEV is enabled in the host kernel;
>>>>> - checking if the kernel cmdline contains 'mem_encrypt=on':
meaning
>>>>> SME memory encryption feature on the host is enabled
>>>>
>>>> In addition to the kernel module parameter, we probably also need to
>>>> check whether QEMU supports the feature. e.g, what if user has newer
>>>> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To
>>>> check the
>>>> SEV feature support, we should verify the following conditions:
>>>>
>>>> 1) check kernel module parameter is set
>>> Paulo implemented the checks following the documentation in
>>> docs/kbase/launch_security_sev.rst. The check for the module
>>> parameter sev
>>> is included. Is the check for the kernel cmdline parameter
>>> "mem_encrypt" not
>>> necessary? Or would that be covered by your suggested check in 2)?
>> Brijesh correct me here please, but IIRC having mem_encrypt set is
>> merely
>> a good security recommendation but is orthogonal with kvm.amd_sev
>> feature, IOW
>> SEV should work without SME. If my memory serves well here, we don't
>> need
>> and also should not parse the kernel cmdline in this case.
>>
>
> Yes, that is correct. mem_encrypt=on is not required for the SEV to
> work. The mem_encrypt=on option is meant to enable the SME feature on
> the host machine. In addition to the guest, if customer wants to protect
> the Hypervsior memory from physical attacks then they can use SME or
> TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can
> be enabled in the BIOS and does not require any kernel changes. AMD is
> mostly recommending TSME to protect against the physical attacks.
>
>
>>>> 2) check if /dev/sev device exist (aka firmware is detected)
>>> This seems reasonable. Shouldn't it have been documented in
>>> docs/kbase/launch_security_sev.rst?
>> Sure, we can add a mention about this. Although, doesn't 1 imply 2?
>> IOW can
>> you have the kernel module parameter set to 1 and yet kernel doesn't
>> expose the
>> /dev/sev node?
>
>
> Currently, 1 does not imply 2, KVM driver does not initialize the
> firmware during the feature probe (i.e does not access the /dev/sev).
> The firmware initialization is delayed until the first guest launch. So
> only sane way to know whether firmware is been detected is check the
> existence of the /dev/sev or issue a query-sev command . The query-sev
> command will send the platform_status request to the firmware, if the
> firmware is not ready then this command will fail.
>
>
Just to summarize the checks you want implemented for AMD SEV support:
1) check kernel module parameter is set (as already implemented)
2) check if /dev/sev device exist
Please note that these checks will also go into virt-host-validate in
patch 5.
Sounds good to me, thanks.
>>>> 3) Check if Qemu supports SEV feature (maybe we can detect the
>>>> existence
>>>> of the query-sev QMP command or detect Qemu version >= 2.12)
>> ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
>>
>>> The idea is to check the host capabilities to detect if qemus
>>> capabilities
>>> need to be reprobed. Therefore this would be a result if checks 1+2
>>> change
>>> but it would not be a cache invalidation trigger.
>> I agree in the sense that the SEV support that is currently being
>> reported in
>> QEMU capabilities wasn't a good choice because it reports only
>> platform data
>> which are constant to the host and have nothing to do with QEMU. It
>> would be
>> okay to just say <sev supported="yes|no"/> in qemu capabilities
and
>> report the
>> rest in host capabilities. I haven't added this info into host
>> capabilities
>> when I realized the mistake because I didn't want to duplicate the
>> platform
>> info on 2 places. For the IBM secure guest feature, it makes total
>> sense to
>> report support within host capabilities, I'm just not sure whether
>> we should do
>> the same for SEV and try really hard to "fix" the mess.
>>
>> Regards,
>
>
7:14 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On Mon, May 11, 2020 at 06:41:58PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted
Virtualization) in order to invalidate the qemu capabilities
cache in case the availability of the feature changed.
For AMD SEV the verification consists of:
- checking if /sys/module/kvm_amd/parameters/sev contains the
value '1': meaning SEV is enabled in the host kernel;
- checking if the kernel cmdline contains 'mem_encrypt=on': meaning
SME memory encryption feature on the host is enabled
Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
docs/kbase/launch_security_sev.rst | 2 +-
src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
index 65f258587d..fa602c7432 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -109,7 +109,7 @@ following:
</features>
</domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before
+Note that if libvirt (<6.4.0) was already installed and libvirtd running before
enabling SEV in the kernel followed by the host reboot you need to force
libvirtd to re-probe both the host and QEMU capabilities. First stop
libvirtd:
Docs updates tend to go to a separate patch.
diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
index 2874bb1e7c..6cf926d52d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
}
+/*
+ * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestAMD(void)
+{
+ g_autofree char *cmdline = NULL;
+ g_autofree char *modValue = NULL;
+ static const char *kValues[] = {"on"};
+
+ if (virFileReadValueString(&modValue,
"/sys/module/kvm_amd/parameters/sev") < 0)
+ return false;
+ if (modValue[0] != '1')
+ return false;
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return false;
+ if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
+ G_N_ELEMENTS(kValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
As discussed already, the last 2 checks will not be necessary.
+ return true;
+ return false;
+}
+
+
/*
* Check whether the secure guest functionality is enabled.
* See the specific architecture function for details on the verifications made.
@@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch))
return virQEMUCapsKVMSupportsSecureGuestS390();
+ if (ARCH_IS_X86(arch))
+ return virQEMUCapsKVMSupportsSecureGuestAMD();
For s390 the check will most likely suffice, but for x86, there isn't just SEV,
there's also MKTME from Intel. However, given there's no support for that in
libvirt, we can figure out later how to propagate host caps in here so that
CPU vendor can be extracted. I just wanted to make it clear, that we'll need
to differentiate the x86 callbacks according to the CPU vendor at some point.
--
Erik Skultety
10:08 a.m.
New subject: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On 5/18/20 2:14 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:58PM +0200, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
>
> Implement secure guest check for AMD SEV (Secure Encrypted
> Virtualization) in order to invalidate the qemu capabilities
> cache in case the availability of the feature changed.
>
> For AMD SEV the verification consists of:
> - checking if /sys/module/kvm_amd/parameters/sev contains the
> value '1': meaning SEV is enabled in the host kernel;
> - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
> SME memory encryption feature on the host is enabled
>
> Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> docs/kbase/launch_security_sev.rst | 2 +-
> src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
> index 65f258587d..fa602c7432 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -109,7 +109,7 @@ following:
> </features>
> </domainCapabilities>
>
> -Note that if libvirt was already installed and libvirtd running before
> +Note that if libvirt (<6.4.0) was already installed and libvirtd running before
> enabling SEV in the kernel followed by the host reboot you need to force
> libvirtd to re-probe both the host and QEMU capabilities. First stop
> libvirtd:
Docs updates tend to go to a separate patch.
And I also have been told when I had the docs updates separate to put
doc updates into the patches that cause them.
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2874bb1e7c..6cf926d52d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
> }
>
>
> +/*
> + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
> + */
> +static bool
> +virQEMUCapsKVMSupportsSecureGuestAMD(void)
> +{
> + g_autofree char *cmdline = NULL;
> + g_autofree char *modValue = NULL;
> + static const char *kValues[] = {"on"};
> +
> + if (virFileReadValueString(&modValue,
"/sys/module/kvm_amd/parameters/sev") < 0)
> + return false;
> + if (modValue[0] != '1')
> + return false;
> + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
> + return false;
> + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
> + G_N_ELEMENTS(kValues),
> + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
> + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
As discussed already, the last 2 checks will not be necessary.
Yes, I have made
some updates for a follow up version.
> + return true;
> + return false;
> +}
> +
> +
> /*
> * Check whether the secure guest functionality is enabled.
> * See the specific architecture function for details on the verifications made.
> @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
>
> if (ARCH_IS_S390(arch))
> return virQEMUCapsKVMSupportsSecureGuestS390();
> + if (ARCH_IS_X86(arch))
> + return virQEMUCapsKVMSupportsSecureGuestAMD();
For s390 the check will most likely suffice, but for x86, there isn't just SEV,
there's also MKTME from Intel. However, given there's no support for that in
libvirt, we can figure out later how to propagate host caps in here so that
CPU vendor can be extracted. I just wanted to make it clear, that we'll need
to differentiate the x86 callbacks according to the CPU vendor at some point.
I agree that once Intels MKTME will be supported by libvirt some
massaging needs to be done for the AMD/Intel split.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:41 a.m.
New subject: [PATCH 4/6] tools: secure guest check on s390 in virt-host-validate
Add checking in virt-host-validate for secure guest support
on s390 for IBM Secure Execution.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Tested-by: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++--
tools/virt-host-validate-common.h | 4 +++
tools/virt-host-validate-qemu.c | 4 +++
3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index fbefbada96..dd73bd0dea 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag,
VIR_HOST_VALIDATE_CPU_FLAG_LAST,
"vmx",
"svm",
- "sie");
+ "sie",
+ "158");
static bool quiet;
@@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void)
* on the architecture, so check possible prefixes */
if (!STRPREFIX(line, "flags") &&
!STRPREFIX(line, "Features") &&
- !STRPREFIX(line, "features"))
+ !STRPREFIX(line, "features") &&
+ !STRPREFIX(line, "facilities"))
continue;
/* fgets() includes the trailing newline in the output buffer,
@@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module)
return ret;
}
+
+
+int virHostValidateSecureGuests(const char *hvname,
+ virHostValidateLevel level)
+{
+ virBitmapPtr flags;
+ bool hasFac158 = false;
+ virArch arch = virArchFromHost();
+ g_autofree char *cmdline = NULL;
+ static const char *kIBMValues[] = {"y", "Y", "1"};
+
+ flags = virHostValidateGetCPUFlags();
+
+ if (flags && virBitmapIsBitSet(flags,
VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158))
+ hasFac158 = true;
+
+ virBitmapFree(flags);
+
+ virHostMsgCheck(hvname, "%s", _("for secure guest support"));
+ if (ARCH_IS_S390(arch)) {
+ if (hasFac158) {
+ if (!virFileIsDir("/sys/firmware/uv")) {
+ virHostMsgFail(level, "IBM Secure Execution not supported by "
+ "the currently used kernel");
+ return 0;
+ }
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return -1;
+ if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues,
+ G_N_ELEMENTS(kIBMValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) {
+ virHostMsgPass();
+ return 1;
+ } else {
+ virHostMsgFail(level,
+ "IBM Secure Execution appears to be disabled "
+ "in kernel. Add prot_virt=1 to kernel cmdline "
+ "arguments");
+ }
+ } else {
+ virHostMsgFail(level, "Hardware or firmware does not provide "
+ "support for IBM Secure Execution");
+ }
+ } else {
+ virHostMsgFail(level,
+ "Unknown if this platform has Secure Guest support");
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h
index 8ae60a21de..44b5544a12 100644
--- a/tools/virt-host-validate-common.h
+++ b/tools/virt-host-validate-common.h
@@ -37,6 +37,7 @@ typedef enum {
VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0,
VIR_HOST_VALIDATE_CPU_FLAG_SVM,
VIR_HOST_VALIDATE_CPU_FLAG_SIE,
+ VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
VIR_HOST_VALIDATE_CPU_FLAG_LAST,
} virHostValidateCPUFlag;
@@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname,
int virHostValidateIOMMU(const char *hvname,
virHostValidateLevel level);
+int virHostValidateSecureGuests(const char *hvname,
+ virHostValidateLevel level);
+
bool virHostKernelModuleIsLoaded(const char *module);
diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c
index bd717a604e..ea7f172790 100644
--- a/tools/virt-host-validate-qemu.c
+++ b/tools/virt-host-validate-qemu.c
@@ -127,5 +127,9 @@ int virHostValidateQEMU(void)
VIR_HOST_VALIDATE_WARN) < 0)
ret = -1;
+ if (virHostValidateSecureGuests("QEMU",
+ VIR_HOST_VALIDATE_WARN) < 0)
+ ret = -1;
+
return ret;
}
--
2.25.1
7:59 a.m.
New subject: [PATCH 4/6] tools: secure guest check on s390 in virt-host-validate
On Mon, May 11, 2020 at 06:41:59PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support
on s390 for IBM Secure Execution.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Tested-by: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++--
tools/virt-host-validate-common.h | 4 +++
tools/virt-host-validate-qemu.c | 4 +++
3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index fbefbada96..dd73bd0dea 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag,
VIR_HOST_VALIDATE_CPU_FLAG_LAST,
"vmx",
"svm",
- "sie");
+ "sie",
+ "158");
static bool quiet;
@@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void)
* on the architecture, so check possible prefixes */
if (!STRPREFIX(line, "flags") &&
!STRPREFIX(line, "Features") &&
- !STRPREFIX(line, "features"))
+ !STRPREFIX(line, "features") &&
+ !STRPREFIX(line, "facilities"))
I can't comment on the first 2 hunks as I don't have an appropriate s390 at
hand, so I can only trust you it's the correct way.
continue;
/* fgets() includes the trailing newline in the output buffer,
@@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module)
return ret;
}
+
+
+int virHostValidateSecureGuests(const char *hvname,
+ virHostValidateLevel level)
+{
+ virBitmapPtr flags;
+ bool hasFac158 = false;
+ virArch arch = virArchFromHost();
+ g_autofree char *cmdline = NULL;
+ static const char *kIBMValues[] = {"y", "Y", "1"};
+
+ flags = virHostValidateGetCPUFlags();
+
+ if (flags && virBitmapIsBitSet(flags,
VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158))
+ hasFac158 = true;
+
+ virBitmapFree(flags);
+
+ virHostMsgCheck(hvname, "%s", _("for secure guest support"));
+ if (ARCH_IS_S390(arch)) {
We don't need ^this check here, facility 158 won't be available on x86.
+ if (hasFac158) {
btw ^such checks should be adopted in QEMU caps code as well like, but we don't
have an appropriate helper at the moment.
+ if (!virFileIsDir("/sys/firmware/uv")) {
+ virHostMsgFail(level, "IBM Secure Execution not supported by
"
+ "the currently used kernel");
+ return 0;
+ }
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return -1;
+ if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues,
+ G_N_ELEMENTS(kIBMValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) {
+ virHostMsgPass();
+ return 1;
+ } else {
+ virHostMsgFail(level,
+ "IBM Secure Execution appears to be disabled "
+ "in kernel. Add prot_virt=1 to kernel cmdline
"
+ "arguments");
+ }
+ } else {
+ virHostMsgFail(level, "Hardware or firmware does not provide "
+ "support for IBM Secure Execution");
+ }
+ } else {
+ virHostMsgFail(level,
+ "Unknown if this platform has Secure Guest support");
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h
index 8ae60a21de..44b5544a12 100644
--- a/tools/virt-host-validate-common.h
+++ b/tools/virt-host-validate-common.h
@@ -37,6 +37,7 @@ typedef enum {
VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0,
VIR_HOST_VALIDATE_CPU_FLAG_SVM,
VIR_HOST_VALIDATE_CPU_FLAG_SIE,
+ VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
VIR_HOST_VALIDATE_CPU_FLAG_LAST,
} virHostValidateCPUFlag;
@@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname,
int virHostValidateIOMMU(const char *hvname,
virHostValidateLevel level);
+int virHostValidateSecureGuests(const char *hvname,
+ virHostValidateLevel level);
+
bool virHostKernelModuleIsLoaded(const char *module);
diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c
index bd717a604e..ea7f172790 100644
--- a/tools/virt-host-validate-qemu.c
+++ b/tools/virt-host-validate-qemu.c
@@ -127,5 +127,9 @@ int virHostValidateQEMU(void)
VIR_HOST_VALIDATE_WARN) < 0)
ret = -1;
+ if (virHostValidateSecureGuests("QEMU",
+ VIR_HOST_VALIDATE_WARN) < 0)
+ ret = -1;
+
return ret;
}
--
2.25.1
--
Erik Skultety
9:56 a.m.
New subject: [PATCH 4/6] tools: secure guest check on s390 in virt-host-validate
On 5/18/20 2:59 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:59PM +0200, Boris Fiuczynski wrote:
> Add checking in virt-host-validate for secure guest support
> on s390 for IBM Secure Execution.
>
> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> Tested-by: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
> Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> ---
> tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++--
> tools/virt-host-validate-common.h | 4 +++
> tools/virt-host-validate-qemu.c | 4 +++
> 3 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
> index fbefbada96..dd73bd0dea 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag,
> VIR_HOST_VALIDATE_CPU_FLAG_LAST,
> "vmx",
> "svm",
> - "sie");
> + "sie",
> + "158");
>
> static bool quiet;
>
> @@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void)
> * on the architecture, so check possible prefixes */
> if (!STRPREFIX(line, "flags") &&
> !STRPREFIX(line, "Features") &&
> - !STRPREFIX(line, "features"))
> + !STRPREFIX(line, "features") &&
> + !STRPREFIX(line, "facilities"))
I can't comment on the first 2 hunks as I don't have an appropriate s390 at
hand, so I can only trust you it's the correct way.
That is fine. We have given this some testing on s390.
> continue;
>
> /* fgets() includes the trailing newline in the output buffer,
> @@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module)
>
> return ret;
> }
> +
> +
> +int virHostValidateSecureGuests(const char *hvname,
> + virHostValidateLevel level)
> +{
> + virBitmapPtr flags;
> + bool hasFac158 = false;
> + virArch arch = virArchFromHost();
> + g_autofree char *cmdline = NULL;
> + static const char *kIBMValues[] = {"y", "Y",
"1"};
> +
> + flags = virHostValidateGetCPUFlags();
> +
> + if (flags && virBitmapIsBitSet(flags,
VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158))
> + hasFac158 = true;
> +
> + virBitmapFree(flags);
> +
> + virHostMsgCheck(hvname, "%s", _("for secure guest
support"));
> + if (ARCH_IS_S390(arch)) {
We don't need ^this check here, facility 158 won't be available on x86.
Since it is very true that the facility 158 won't be available on x86
you do not want to get the message "Hardware or firmware does not
provide support for IBM Secure Execution" on x86.
On the other hand you can be on s390 and not have the facility available
and want to know see the message.
> + if (hasFac158) {
btw ^such checks should be adopted in QEMU caps code as well like, but we don't
have an appropriate helper at the moment.
> + if (!virFileIsDir("/sys/firmware/uv")) {
> + virHostMsgFail(level, "IBM Secure Execution not supported by
"
> + "the currently used kernel");
> + return 0;
> + }
> + if (virFileReadValueString(&cmdline, "/proc/cmdline") <
0)
> + return -1;
> + if (virKernelCmdlineMatchParam(cmdline, "prot_virt",
kIBMValues,
> + G_N_ELEMENTS(kIBMValues),
> + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY |
> + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) {
> + virHostMsgPass();
> + return 1;
> + } else {
> + virHostMsgFail(level,
> + "IBM Secure Execution appears to be disabled
"
> + "in kernel. Add prot_virt=1 to kernel cmdline
"
> + "arguments");
> + }
> + } else {
> + virHostMsgFail(level, "Hardware or firmware does not provide
"
> + "support for IBM Secure Execution");
> + }
> + } else {
> + virHostMsgFail(level,
> + "Unknown if this platform has Secure Guest
support");
> + return -1;
> + }
> +
> + return 0;
> +}
> diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h
> index 8ae60a21de..44b5544a12 100644
> --- a/tools/virt-host-validate-common.h
> +++ b/tools/virt-host-validate-common.h
> @@ -37,6 +37,7 @@ typedef enum {
> VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0,
> VIR_HOST_VALIDATE_CPU_FLAG_SVM,
> VIR_HOST_VALIDATE_CPU_FLAG_SIE,
> + VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
>
> VIR_HOST_VALIDATE_CPU_FLAG_LAST,
> } virHostValidateCPUFlag;
> @@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname,
> int virHostValidateIOMMU(const char *hvname,
> virHostValidateLevel level);
>
> +int virHostValidateSecureGuests(const char *hvname,
> + virHostValidateLevel level);
> +
> bool virHostKernelModuleIsLoaded(const char *module);
> diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c
> index bd717a604e..ea7f172790 100644
> --- a/tools/virt-host-validate-qemu.c
> +++ b/tools/virt-host-validate-qemu.c
> @@ -127,5 +127,9 @@ int virHostValidateQEMU(void)
> VIR_HOST_VALIDATE_WARN) < 0)
> ret = -1;
>
> + if (virHostValidateSecureGuests("QEMU",
> + VIR_HOST_VALIDATE_WARN) < 0)
> + ret = -1;
> +
> return ret;
> }
> --
> 2.25.1
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
1:17 a.m.
New subject: [PATCH 4/6] tools: secure guest check on s390 in virt-host-validate
...
> > +
> > + virHostMsgCheck(hvname, "%s", _("for secure guest
support"));
> > + if (ARCH_IS_S390(arch)) {
>
> We don't need ^this check here, facility 158 won't be available on x86.
Since it is very true that the facility 158 won't be available on x86 you do
not want to get the message "Hardware or firmware does not provide support
for IBM Secure Execution" on x86.
On the other hand you can be on s390 and not have the facility available and
want to know see the message.
Oops, I missed the last nested else block, please ignore what I said then.
--
Erik Skultety
11:42 a.m.
New subject: [PATCH 5/6] tools: secure guest check for AMD in virt-host-validate
Add checking in virt-host-validate for secure guest support
on x86 for AMD Secure Encrypted Virtualization.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
docs/kbase/launch_security_sev.rst | 7 ++++--
tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++--
tools/virt-host-validate-common.h | 1 +
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
index fa602c7432..45166b3886 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -30,8 +30,11 @@ Enabling SEV on the host
========================
Before VMs can make use of the SEV feature you need to make sure your
-AMD CPU does support SEV. You can check whether SEV is among the CPU
-flags with:
+AMD CPU does support SEV. You can run ``libvirt-host-validate``
+(libvirt >= 6.4.0) to check if your host supports secure guests or you
+can follow the manual checks below.
+
+You can manually check whether SEV is among the CPU flags with:
::
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index dd73bd0dea..67aa420d35 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -41,7 +41,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag,
"vmx",
"svm",
"sie",
- "158");
+ "158",
+ "sev");
static bool quiet;
@@ -447,15 +448,19 @@ int virHostValidateSecureGuests(const char *hvname,
virHostValidateLevel level)
{
virBitmapPtr flags;
- bool hasFac158 = false;
+ bool hasFac158 = false, hasAMDSev = false;
virArch arch = virArchFromHost();
g_autofree char *cmdline = NULL;
static const char *kIBMValues[] = {"y", "Y", "1"};
+ static const char *kAMDValues[] = {"on"};
+ g_autofree char *mod_value = NULL;
flags = virHostValidateGetCPUFlags();
if (flags && virBitmapIsBitSet(flags,
VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158))
hasFac158 = true;
+ else if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SEV))
+ hasAMDSev = true;
virBitmapFree(flags);
@@ -485,6 +490,33 @@ int virHostValidateSecureGuests(const char *hvname,
virHostMsgFail(level, "Hardware or firmware does not provide "
"support for IBM Secure Execution");
}
+ } else if (hasAMDSev) {
+ if (virFileReadValueString(&mod_value,
"/sys/module/kvm_amd/parameters/sev") < 0) {
+ virHostMsgFail(level, "AMD Secure Encrypted Virtualization not "
+ "supported by the currently used kernel");
+ return 0;
+ }
+ if (mod_value[0] != '1') {
+ virHostMsgFail(level,
+ "AMD Secure Encrypted Virtualization appears to be
"
+ "disabled in kernel. Add mem_encrypt=on "
+ "kvm_amd.sev=1 to kernel cmdline arguments");
+ return 0;
+ }
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
+ return -1;
+ if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kAMDValues,
+ G_N_ELEMENTS(kAMDValues),
+ VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
+ VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)) {
+ virHostMsgPass();
+ return 1;
+ } else {
+ virHostMsgFail(level,
+ "AMD Secure Encrypted Virtualization appears to be
"
+ "disabled in kernel. Add mem_encrypt=on "
+ "kvm_amd.sev=1 to kernel cmdline arguments");
+ }
} else {
virHostMsgFail(level,
"Unknown if this platform has Secure Guest support");
diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h
index 44b5544a12..3df5ea0c7e 100644
--- a/tools/virt-host-validate-common.h
+++ b/tools/virt-host-validate-common.h
@@ -38,6 +38,7 @@ typedef enum {
VIR_HOST_VALIDATE_CPU_FLAG_SVM,
VIR_HOST_VALIDATE_CPU_FLAG_SIE,
VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
+ VIR_HOST_VALIDATE_CPU_FLAG_SEV,
VIR_HOST_VALIDATE_CPU_FLAG_LAST,
} virHostValidateCPUFlag;
--
2.25.1
8:01 a.m.
New subject: [PATCH 5/6] tools: secure guest check for AMD in virt-host-validate
On Mon, May 11, 2020 at 06:42:00PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support
on x86 for AMD Secure Encrypted Virtualization.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
docs/kbase/launch_security_sev.rst | 7 ++++--
tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++--
tools/virt-host-validate-common.h | 1 +
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
index fa602c7432..45166b3886 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -30,8 +30,11 @@ Enabling SEV on the host
========================
Before VMs can make use of the SEV feature you need to make sure your
-AMD CPU does support SEV. You can check whether SEV is among the CPU
-flags with:
+AMD CPU does support SEV. You can run ``libvirt-host-validate``
+(libvirt >= 6.4.0) to check if your host supports secure guests or you
+can follow the manual checks below.
+
+You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a
standalone patch.
Otherwise looking good.
--
Erik Skultety
10:16 a.m.
New subject: [PATCH 5/6] tools: secure guest check for AMD in virt-host-validate
On 5/18/20 3:01 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:42:00PM +0200, Boris Fiuczynski wrote:
> Add checking in virt-host-validate for secure guest support
> on x86 for AMD Secure Encrypted Virtualization.
>
> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> ---
> docs/kbase/launch_security_sev.rst | 7 ++++--
> tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++--
> tools/virt-host-validate-common.h | 1 +
> 3 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst
> index fa602c7432..45166b3886 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -30,8 +30,11 @@ Enabling SEV on the host
> ========================
>
> Before VMs can make use of the SEV feature you need to make sure your
> -AMD CPU does support SEV. You can check whether SEV is among the CPU
> -flags with:
> +AMD CPU does support SEV. You can run ``libvirt-host-validate``
> +(libvirt >= 6.4.0) to check if your host supports secure guests or you
> +can follow the manual checks below.
> +
> +You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a
standalone patch.
Actually the earlier patches fix the stale cap cache and this update is
because of a new support in libvirt-host-validate. I am not sure that we
should tie these to into one patch.
I would prefer to keep the two doc changes separate and with the changes
that caused the update.
Otherwise looking good.
Thanks but the changes need also to be adjusted as discussed on patch 3.
I will do so in a followup version.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
1:25 a.m.
New subject: [PATCH 5/6] tools: secure guest check for AMD in virt-host-validate
...
> > diff --git a/docs/kbase/launch_security_sev.rst
b/docs/kbase/launch_security_sev.rst
> > index fa602c7432..45166b3886 100644
> > --- a/docs/kbase/launch_security_sev.rst
> > +++ b/docs/kbase/launch_security_sev.rst
> > @@ -30,8 +30,11 @@ Enabling SEV on the host
> > ========================
> > Before VMs can make use of the SEV feature you need to make sure your
> > -AMD CPU does support SEV. You can check whether SEV is among the CPU
> > -flags with:
> > +AMD CPU does support SEV. You can run ``libvirt-host-validate``
> > +(libvirt >= 6.4.0) to check if your host supports secure guests or you
> > +can follow the manual checks below.
> > +
> > +You can manually check whether SEV is among the CPU flags with:
>
> ^this change should go along the (<6.4.0) in one of the earlier patches into a
> standalone patch.
Actually the earlier patches fix the stale cap cache and this update is
because of a new support in libvirt-host-validate. I am not sure that we
should tie these to into one patch.
I would prefer to keep the two doc changes separate and with the changes
that caused the update.
I won't argue against that logic. However, both patch 3 and this one update the
same knowledge article. What IMO matters here the most is that once all of the
changes you're introducing are applied as a unit, the article needs to
reflect both the changes. From that perspective, at least to me it makes total
sense to group the docs changes from both 3/6 and this patch to a single update
to the SEV article accordingly.
--
Erik Skultety
3:47 a.m.
New subject: [PATCH 5/6] tools: secure guest check for AMD in virt-host-validate
On 5/19/20 8:25 AM, Erik Skultety wrote:
...
>>> diff --git a/docs/kbase/launch_security_sev.rst
b/docs/kbase/launch_security_sev.rst
>>> index fa602c7432..45166b3886 100644
>>> --- a/docs/kbase/launch_security_sev.rst
>>> +++ b/docs/kbase/launch_security_sev.rst
>>> @@ -30,8 +30,11 @@ Enabling SEV on the host
>>> ========================
>>> Before VMs can make use of the SEV feature you need to make sure your
>>> -AMD CPU does support SEV. You can check whether SEV is among the CPU
>>> -flags with:
>>> +AMD CPU does support SEV. You can run ``libvirt-host-validate``
>>> +(libvirt >= 6.4.0) to check if your host supports secure guests or you
>>> +can follow the manual checks below.
>>> +
>>> +You can manually check whether SEV is among the CPU flags with:
>>
>> ^this change should go along the (<6.4.0) in one of the earlier patches into
a
>> standalone patch.
>
> Actually the earlier patches fix the stale cap cache and this update is
> because of a new support in libvirt-host-validate. I am not sure that we
> should tie these to into one patch.
> I would prefer to keep the two doc changes separate and with the changes
> that caused the update.
I won't argue against that logic. However, both patch 3 and this one update the
same knowledge article. What IMO matters here the most is that once all of the
changes you're introducing are applied as a unit, the article needs to
reflect both the changes. From that perspective, at least to me it makes total
sense to group the docs changes from both 3/6 and this patch to a single update
to the SEV article accordingly.
Fine, I will sort out the changes and create a single AMD doc update patch.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:42 a.m.
New subject: [PATCH 6/6] docs: Describe protected virtualization guest setup
From: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
Protected virtualization/IBM Secure Execution for Linux protects
guest memory and state from the host.
Add some basic information about technology and a brief guide
on setting up secure guests with libvirt.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com>
---
docs/kbase.html.in | 3 +
docs/kbase/s390_protected_virt.rst | 189 +++++++++++++++++++++++++++++
2 files changed, 192 insertions(+)
create mode 100644 docs/kbase/s390_protected_virt.rst
diff --git a/docs/kbase.html.in b/docs/kbase.html.in
index c586e0f676..241a212fa9 100644
--- a/docs/kbase.html.in
+++ b/docs/kbase.html.in
@@ -14,6 +14,9 @@
<dt><a href="kbase/secureusage.html">Secure
usage</a></dt>
<dd>Secure usage of the libvirt APIs</dd>
+ <dt><a href="kbase/s390_protected_virt.html">Protected
virtualization on s390</a></dt>
+ <dd>Running secure s390 guests with IBM Secure Execution</dd>
+
<dt><a href="kbase/launch_security_sev.html">Launch
security</a></dt>
<dd>Securely launching VMs with AMD SEV</dd>
diff --git a/docs/kbase/s390_protected_virt.rst b/docs/kbase/s390_protected_virt.rst
new file mode 100644
index 0000000000..3e354455d1
--- /dev/null
+++ b/docs/kbase/s390_protected_virt.rst
@@ -0,0 +1,189 @@
+================================
+Protected Virtualization on s390
+================================
+
+.. contents::
+
+Overview
+========
+
+Protected virtualization, also known as IBM Secure Execution is a
+hardware-based privacy protection technology for s390x (IBM Z).
+It allows to execute virtual machines such that the host system
+has no access to a VM's state and memory contents.
+
+Unlike other similar technologies, the memory of a running guest
+is not encrypted but protected by hardware access controls, which
+may only be manipulated by trusted system firmware, called
+ultravisor.
+
+For the cases where the host needs access to guest memory (e.g. for
+paging), it can request pages to be exported to it. The exported page
+will be encrypted with a unique key for the running guest by the
+ultravisor. The ultravisor also computes an integrity value for
+the page, and stores it in a special table, together with the page
+index and a counter. This way it can verify the integrity of
+the page content upon re-import into the guest.
+
+In other cases it may be necessary for a guest to grant the host access
+to dedicated memory regions (e.g. for I/O). The guest can request
+that the ultravisor removes the memory protection from individual
+pages, so that they can be shared with the host. Likewise, the
+guest can undo the sharing.
+
+A secure guest will initially start in a regular non-protected VM.
+The start-up is controlled by a small bootstrap program loaded
+into memory together with encrypted operating system components and
+a control structure (the PV header).
+The operating system components (e.g. Linux kernel, initial RAM
+file system, kernel parameters) are encrypted and integrity
+protected. The component encryption keys and integrity values are
+stored in the PV header.
+The PV header is wrapped with a public key belonging to a specific
+system (in fact it can be wrapped with multiple such keys). The
+matching private key is only accessible by trusted hardware and
+firmware in that specific system.
+Consequently, such a secure guest boot image can only be run on the
+systems it has been prepared for. Its contents can't be decrypted
+without access to the private key and it can't be modified as
+it is integrity protected.
+
+Host Requirements
+=================
+
+IBM Secure Execution for Linux has some hardware and firmware
+requirements. The system hardware must be an IBM z15 (or newer),
+or an IBM LinuxONE III (or newer).
+
+It is also necessary that the IBM Secure Execution feature is
+enabled for that system. With libvirt >= 6.4.0 you can run
+``libvirt-host--validate`` or otherwise check for facility '158', e.g.
+
+::
+
+ $ grep facilities /proc/cpuinfo | grep 158
+
+The kernel must include the protected virtualization support
+which can be verified by checking for the presence of directory
+``/sys/firmware/uv``. It will only be present when both the
+hardware and the kernel support are available.
+
+Finally, the host operating system must donate some memory to
+the ultravisor needed to store memory security information.
+This is achieved by specifying the following kernel command
+line parameter to the host boot configuration
+
+::
+
+ prot_virt=1
+
+
+Guest Requirements
+==================
+
+Guest Boot
+----------
+
+To start a guest in protected virtualization secure mode, the
+boot image must have been prepared first with the program
+``genprotimg`` using the correct public key for this host.
+``genprotimg`` is part of the package ``s390-tools``, or
+``s390-utils``, depending on the Linux distribution being used.
+It can also be found at
+`<https://github.com/ibm-s390-tools/s390-tools/tree/master/genprotimg>`_
+
+The guests have to be configured to use the host CPU model, which
+must contain the ``unpack`` facility indicating ultravisor guest support.
+
+With the following command it's possible to check whether the host
+CPU model satisfies the requirement
+
+::
+
+ $ virsh domcapabilities | grep unpack
+
+which should return
+
+::
+
+ <feature policy='require' name='unpack'/>
+
+Note that on hosts with libvirt < 6.4.0 if the check fails despite
+the host system actually supporting protected virtualization guests,
+this can be caused by a stale libvirt capabilities cache.
+To recover, run the following commands
+
+::
+
+ $ systemctl stop libvirtd
+ $ rm /var/cache/libvirt/qemu/capabilities/*.xml
+ $ systemctl start libvirtd
+
+
+Guest I/O
+---------
+
+Protected virtualization guests support I/O using virtio devices.
+As the virtio data structures of secure guests are not accessible
+by the host, it is necessary to use shared memory ('bounce buffers').
+
+To enable virtio devices to use shared buffers, it is necessary
+to configure them with platform_iommu enabled. This can done by adding
+``iommu='on'`` to the driver element of a virtio device definition in the
+guest's XML, e.g.
+
+::
+
+ <interface type='network'>
+ <source network='default'/>
+ <model type='virtio'/>
+ <driver name='vhost' iommu='on'/>
+ </interface>
+
+It is mandatory to define all virtio bus devices in this way to
+prevent the host from attempting to access protected memory.
+Ballooning will not work and is fenced by QEMU. It should be
+disabled by specifying
+
+::
+
+ <memballoon model='none'/>
+
+Finally, the guest Linux must be instructed to allocate I/O
+buffers using memory shared between host and guest using SWIOTLB.
+This is done by adding ``swiotlb=nnn`` to the guest's kernel command
+line string, where ``nnn`` stands for the number of statically
+allocated 2K entries. A commonly used value for swiotlb is 262144.
+
+Example guest definition
+========================
+
+Minimal domain XML for a protected virtualization guest, essentially
+it's mostly about the ``iommu`` property
+
+::
+
+ <domain type='kvm'>
+ <name>protected</name>
+ <memory unit='KiB'>2048000</memory>
+ <currentMemory unit='KiB'>2048000</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x'>hvm</type>
+ </os>
+ <cpu mode='host-model'/>
+ <devices>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2' cache='none'
io='native' iommu='on'>
+ <source file='/var/lib/libvirt/images/protected.qcow2'/>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <interface type='network'>
+ <driver iommu='on'/>
+ <source network='default'/>
+ <model type='virtio'/>
+ </interface>
+ <console type='pty'/>
+ <memballoon model='none'/>
+ </devices>
+ </domain>
--
2.25.1
1669
days inactive
1686
days old
28 comments
4 participants
participants (4)
-
Boris Fiuczynski -
Brijesh Singh -
Erik Skultety -
Paulo de Rezende Pinatti