qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
@ 2021-07-09 21:55 Brijesh Singh
  2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
                   ` (7 more replies)
  0 siblings, 8 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
new hardware-based memory protections. SEV-SNP adds strong memory integrity
protection to help prevent malicious hypervisor-based attacks like data
replay, memory re-mapping and more in order to create an isolated memory
encryption environment.

The patches to support the SEV-SNP in Linux kernel and OVMF are available:
https://lore.kernel.org/kvm/20210707181506.30489-1-brijesh.singh@amd.com/
https://lore.kernel.org/kvm/20210707183616.5620-1-brijesh.singh@amd.com/
https://edk2.groups.io/g/devel/message/77335?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891508

The Qemu patches uses the command id added by the SEV-SNP hypervisor
patches to bootstrap the SEV-SNP VMs.

TODO:
 * Add support to filter CPUID values through the PSP.

Additional resources
---------------------
SEV-SNP whitepaper
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)

GHCB spec:
https://developer.amd.com/wp-content/resources/56421.pdf

SEV-SNP firmware specification:
https://www.amd.com/system/files/TechDocs/56860.pdf

Brijesh Singh (6):
  linux-header: add the SNP specific command
  i386/sev: extend sev-guest property to include SEV-SNP
  i386/sev: initialize SNP context
  i386/sev: add the SNP launch start context
  i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  i386/sev: populate secrets and cpuid page and finalize the SNP launch

 docs/amd-memory-encryption.txt |  81 +++++-
 linux-headers/linux/kvm.h      |  47 ++++
 qapi/qom.json                  |   6 +
 target/i386/sev.c              | 498 ++++++++++++++++++++++++++++++++-
 target/i386/sev_i386.h         |   1 +
 target/i386/trace-events       |   4 +
 6 files changed, 628 insertions(+), 9 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 1/6] linux-header: add the SNP specific command
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-10 20:32   ` Michael S. Tsirkin
  2021-07-19 11:35   ` Dov Murik
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

Sync the kvm.h with the kernel to include the SNP specific commands.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 linux-headers/linux/kvm.h | 47 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 20d6a263bb..c17ace1ece 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1679,6 +1679,12 @@ enum sev_cmd_id {
 	/* Guest Migration Extension */
 	KVM_SEV_SEND_CANCEL,
 
+	/* SNP specific commands */
+	KVM_SEV_SNP_INIT = 256,
+	KVM_SEV_SNP_LAUNCH_START,
+	KVM_SEV_SNP_LAUNCH_UPDATE,
+	KVM_SEV_SNP_LAUNCH_FINISH,
+
 	KVM_SEV_NR_MAX,
 };
 
@@ -1775,6 +1781,47 @@ struct kvm_sev_receive_update_data {
 	__u32 trans_len;
 };
 
+struct kvm_snp_init {
+	__u64 flags;
+};
+
+struct kvm_sev_snp_launch_start {
+	__u64 policy;
+	__u64 ma_uaddr;
+	__u8 ma_en;
+	__u8 imi_en;
+	__u8 gosvw[16];
+};
+
+#define KVM_SEV_SNP_PAGE_TYPE_NORMAL		0x1
+#define KVM_SEV_SNP_PAGE_TYPE_VMSA		0x2
+#define KVM_SEV_SNP_PAGE_TYPE_ZERO		0x3
+#define KVM_SEV_SNP_PAGE_TYPE_UNMEASURED	0x4
+#define KVM_SEV_SNP_PAGE_TYPE_SECRETS		0x5
+#define KVM_SEV_SNP_PAGE_TYPE_CPUID		0x6
+
+struct kvm_sev_snp_launch_update {
+	__u64 uaddr;
+	__u32 len;
+	__u8 imi_page;
+	__u8 page_type;
+	__u8 vmpl3_perms;
+	__u8 vmpl2_perms;
+	__u8 vmpl1_perms;
+};
+
+#define KVM_SEV_SNP_ID_BLOCK_SIZE	96
+#define KVM_SEV_SNP_ID_AUTH_SIZE	4096
+#define KVM_SEV_SNP_FINISH_DATA_SIZE	32
+
+struct kvm_sev_snp_launch_finish {
+	__u64 id_block_uaddr;
+	__u64 id_auth_uaddr;
+	__u8 id_block_en;
+	__u8 auth_key_en;
+	__u8 host_data[KVM_SEV_SNP_FINISH_DATA_SIZE];
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
  2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-12  6:09   ` Dov Murik
                     ` (4 more replies)
  2021-07-09 21:55 ` [RFC PATCH 3/6] i386/sev: initialize SNP context Brijesh Singh
                   ` (5 subsequent siblings)
  7 siblings, 5 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

To launch the SEV-SNP guest, a user can specify up to 8 parameters.
Passing all parameters through command line can be difficult. To simplify
the launch parameter passing, introduce a .ini-like config file that can be
used for passing the parameters to the launch flow.

The contents of the config file will look like this:

$ cat snp-launch.init

# SNP launch parameters
[SEV-SNP]
init_flags = 0
policy = 0x1000
id_block = "YWFhYWFhYWFhYWFhYWFhCg=="


Add 'snp' property that can be used to indicate that SEV guest launch
should enable the SNP support.

SEV-SNP guest launch examples:

1) launch without additional parameters

  $(QEMU_CLI) \
    -object sev-guest,id=sev0,snp=on

2) launch with optional parameters
  $(QEMU_CLI) \
    -object sev-guest,id=sev0,snp=on,launch-config=<file>

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 docs/amd-memory-encryption.txt |  81 +++++++++++-
 qapi/qom.json                  |   6 +
 target/i386/sev.c              | 227 +++++++++++++++++++++++++++++++++
 3 files changed, 312 insertions(+), 2 deletions(-)

diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
index ffca382b5f..322bf38f68 100644
--- a/docs/amd-memory-encryption.txt
+++ b/docs/amd-memory-encryption.txt
@@ -22,8 +22,8 @@ support for notifying a guest's operating system when certain types of VMEXITs
 are about to occur. This allows the guest to selectively share information with
 the hypervisor to satisfy the requested function.
 
-Launching
----------
+Launching (SEV and SEV-ES)
+--------------------------
 Boot images (such as bios) must be encrypted before a guest can be booted. The
 MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: LAUNCH_START,
 LAUNCH_UPDATE_DATA, LAUNCH_MEASURE and LAUNCH_FINISH. These four commands
@@ -113,6 +113,83 @@ a SEV-ES guest:
  - Requires in-kernel irqchip - the burden is placed on the hypervisor to
    manage booting APs.
 
+Launching (SEV-SNP)
+-------------------
+Boot images (such as bios) must be encrypted before a guest can be booted. The
+MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images:
+KVM_SNP_INIT, SNP_LAUNCH_START, SNP_LAUNCH_UPDATE, and SNP_LAUNCH_FINISH. These
+four commands together generate a fresh memory encryption key for the VM,
+encrypt the boot images for a successful launch.
+
+KVM_SNP_INIT is called first to initialize the SEV-SNP firmware and SNP
+features in the KVM. The feature flags value can be provided through the
+launch-config file.
+
++------------+-------+----------+---------------------------------+
+| key        | type  | default  | meaning                         |
++------------+-------+----------+---------------------------------+
+| init_flags | hex   | 0        | SNP feature flags               |
++-----------------------------------------------------------------+
+
+Note: currently the init_flags must be zero.
+
+SNP_LAUNCH_START is called first to create a cryptographic launch context
+within the firmware. To create this context, guest owner must provide a guest
+policy and other parameters as described in the SEV-SNP firmware
+specification. The launch parameters should be specified in the launch-config
+ini file and should be treated as a binary blob and must be passed as-is to
+the SEV-SNP firmware.
+
+The SNP_LAUNCH_START uses the following parameters from the launch-config
+file. See the SEV-SNP specification for more details.
+
++--------+-------+----------+----------------------------------------------+
+| key    | type  | default  | meaning                                      |
++--------+-------+----------+----------------------------------------------+
+| policy | hex   | 0x30000  | a 64-bit guest policy                        |
+| imi_en | bool  | 0        | 1 when IMI is enabled                        |
+| ma_end | bool  | 0        | 1 when migration agent is used               |
+| gosvw  | string| 0        | 16-byte base64 encoded string for the guest  |
+|        |       |          | OS visible workaround.                       |
++--------+-------+----------+----------------------------------------------+
+
+SNP_LAUNCH_UPDATE encrypts the memory region using the cryptographic context
+created via the SNP_LAUNCH_START command. If required, this command can be called
+multiple times to encrypt different memory regions. The command also calculates
+the measurement of the memory contents as it encrypts.
+
+SNP_LAUNCH_FINISH finalizes the guest launch flow. Optionally, while finalizing
+the launch the firmware can perform checks on the launch digest computing
+through the SNP_LAUNCH_UPDATE. To perform the check the user must supply
+the id block, authentication blob and host data that should be included in the
+attestation report. See the SEV-SNP spec for further details.
+
+The SNP_LAUNCH_FINISH uses the following parameters from the launch-config file.
+
++------------+-------+----------+----------------------------------------------+
+| key        | type  | default  | meaning                                      |
++------------+-------+----------+----------------------------------------------+
+| id_block   | string| none     | base64 encoded ID block                      |
++------------+-------+----------+----------------------------------------------+
+| id_auth    | string| none     | base64 encoded authentication information    |
++------------+-------+----------+----------------------------------------------+
+| auth_key_en| bool  | 0        | auth block contains author key               |
++------------+-------+----------+----------------------------------------------+
+| host_data  | string| none     | host provided data                           |
++------------+-------+----------+----------------------------------------------+
+
+To launch a SEV-SNP guest
+
+# ${QEMU} \
+    -machine ...,confidential-guest-support=sev0 \
+    -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on
+
+To launch a SEV-SNP guest with launch configuration
+
+# ${QEMU} \
+    -machine ...,confidential-guest-support=sev0 \
+    -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on,launch-config=<config>
+
 Debugging
 -----------
 Since the memory contents of a SEV guest are encrypted, hypervisor access to
diff --git a/qapi/qom.json b/qapi/qom.json
index 652be317b8..bdf89fda27 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -749,6 +749,10 @@
 # @reduced-phys-bits: number of bits in physical addresses that become
 #                     unavailable when SEV is enabled
 #
+# @snp: SEV-SNP is enabled (default: 0)
+#
+# @launch-config: launch config file to use
+#
 # Since: 2.12
 ##
 { 'struct': 'SevGuestProperties',
@@ -758,6 +762,8 @@
             '*policy': 'uint32',
             '*handle': 'uint32',
             '*cbitpos': 'uint32',
+            '*snp': 'bool',
+            '*launch-config': 'str',
             'reduced-phys-bits': 'uint32' } }
 
 ##
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 83df8c09f6..6b238ef969 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -37,6 +37,11 @@
 #define TYPE_SEV_GUEST "sev-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
 
+struct snp_launch_config {
+    struct kvm_snp_init init;
+    struct kvm_sev_snp_launch_start start;
+    struct kvm_sev_snp_launch_finish finish;
+};
 
 /**
  * SevGuestState:
@@ -58,6 +63,8 @@ struct SevGuestState {
     char *session_file;
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
+    char *launch_config_file;
+    bool snp;
 
     /* runtime state */
     uint32_t handle;
@@ -72,10 +79,13 @@ struct SevGuestState {
     uint32_t reset_cs;
     uint32_t reset_ip;
     bool reset_data_valid;
+
+    struct snp_launch_config snp_config;
 };
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
+#define DEFAULT_SEV_SNP_POLICY  0x30000
 
 #define SEV_INFO_BLOCK_GUID     "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
 typedef struct __attribute__((__packed__)) SevInfoBlock {
@@ -298,6 +308,212 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
     sev->sev_device = g_strdup(value);
 }
 
+static void
+sev_guest_set_snp(Object *obj, bool value, Error **errp)
+{
+    SevGuestState *sev = SEV_GUEST(obj);
+
+    sev->snp = value;
+}
+
+static bool
+sev_guest_get_snp(Object *obj, Error **errp)
+{
+    SevGuestState *sev = SEV_GUEST(obj);
+
+    return sev->snp;
+}
+
+
+static char *
+sev_guest_get_launch_config_file(Object *obj, Error **errp)
+{
+    SevGuestState *s = SEV_GUEST(obj);
+
+    return g_strdup(s->launch_config_file);
+}
+
+static int
+config_read_uint64(GKeyFile *f, const char *key, uint64_t *value, Error **errp)
+{
+    g_autoptr(GError) error = NULL;
+    g_autofree gchar *str = NULL;
+    uint64_t res;
+
+    str = g_key_file_get_string(f, "SEV-SNP", key, &error);
+    if (!str) {
+        /* key not found */
+        return 0;
+    }
+
+    res = g_ascii_strtoull(str, NULL, 16);
+    if (res == G_MAXUINT64) {
+        error_setg(errp, "Failed to convert %s", str);
+        return 1;
+    }
+
+    *value = res;
+    return 0;
+}
+
+static int
+config_read_bool(GKeyFile *f, const char *key, bool *value, Error **errp)
+{
+    g_autoptr(GError) error = NULL;
+    gboolean val;
+
+    val = g_key_file_get_boolean(f, "SEV-SNP", key, &error);
+    if (!val && g_error_matches(error, G_KEY_FILE_ERROR,
+                                 G_KEY_FILE_ERROR_INVALID_VALUE)) {
+        error_setg(errp, "%s", error->message);
+        return 1;
+    }
+
+    *value = val;
+    return 0;
+}
+
+static int
+config_read_blob(GKeyFile *f, const char *key, uint8_t *blob, uint32_t len,
+                 Error **errp)
+{
+    g_autoptr(GError) error = NULL;
+    g_autofree guchar *data = NULL;
+    g_autofree gchar *base64 = NULL;
+    gsize size;
+
+    base64 = g_key_file_get_string(f, "SEV-SNP", key, &error);
+    if (!base64) {
+        /* key not found */
+        return 0;
+    }
+
+    /* lets decode the value string */
+    data = g_base64_decode(base64, &size);
+    if (!data) {
+        error_setg(errp, "failed to decode '%s'", key);
+        return 1;
+    }
+
+    /* verify the length */
+    if (len != size) {
+        error_setg(errp, "invalid length for key '%s' (expected %d got %ld)",
+                   key, len, size);
+        return 1;
+    }
+
+    memcpy(blob, data, size);
+    return 0;
+}
+
+static int
+snp_parse_launch_config(SevGuestState *sev, const char *file, Error **errp)
+{
+    g_autoptr(GError) error = NULL;
+    g_autoptr(GKeyFile) key_file = g_key_file_new();
+    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
+    struct kvm_snp_init *init = &sev->snp_config.init;
+    struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
+    uint8_t *id_block = NULL, *id_auth = NULL;
+
+    if (!g_key_file_load_from_file(key_file, file, G_KEY_FILE_NONE, &error)) {
+        error_setg(errp, "Error loading config file: %s", error->message);
+        return 1;
+    }
+
+    /* Check the group first */
+    if (!g_key_file_has_group(key_file, "SEV-SNP")) {
+        error_setg(errp, "Error parsing config file, group SEV-SNP not found");
+        return 1;
+    }
+
+    /* Get the init_flags used in KVM_SNP_INIT */
+    if (config_read_uint64(key_file, "init_flags",
+                           (uint64_t *)&init->flags, errp)) {
+        goto err;
+    }
+
+    /* Get the policy used in LAUNCH_START */
+    if (config_read_uint64(key_file, "policy",
+                           (uint64_t *)&start->policy, errp)) {
+        goto err;
+    }
+
+    /* Get IMI_EN used in LAUNCH_START */
+    if (config_read_bool(key_file, "imi_en", (bool *)&start->imi_en, errp)) {
+        goto err;
+    }
+
+    /* Get MA_EN used in LAUNCH_START */
+    if (config_read_bool(key_file, "imi_en", (bool *)&start->ma_en, errp)) {
+        goto err;
+    }
+
+    /* Get GOSVW used in LAUNCH_START */
+    if (config_read_blob(key_file, "gosvw", (uint8_t *)&start->gosvw,
+                         sizeof(start->gosvw), errp)) {
+        goto err;
+    }
+
+    /* Get ID block used in LAUNCH_FINISH */
+    if (g_key_file_has_key(key_file, "SEV-SNP", "id_block", &error)) {
+
+        id_block = g_malloc(KVM_SEV_SNP_ID_BLOCK_SIZE);
+
+        if (config_read_blob(key_file, "id_block", id_block,
+                             KVM_SEV_SNP_ID_BLOCK_SIZE, errp)) {
+            goto err;
+        }
+
+        finish->id_block_uaddr = (unsigned long)id_block;
+        finish->id_block_en = 1;
+    }
+
+    /* Get authentication block used in LAUNCH_FINISH */
+    if (g_key_file_has_key(key_file, "SEV-SNP", "id_auth", &error)) {
+
+        id_auth = g_malloc(KVM_SEV_SNP_ID_AUTH_SIZE);
+
+        if (config_read_blob(key_file, "auth_block", id_auth,
+                             KVM_SEV_SNP_ID_AUTH_SIZE, errp)) {
+            goto err;
+        }
+
+        finish->id_auth_uaddr = (unsigned long)id_auth;
+
+        /* Get AUTH_KEY_EN used in LAUNCH_FINISH */
+        if (config_read_bool(key_file, "auth_key_en",
+                             (bool *)&finish->auth_key_en, errp)) {
+            goto err;
+        }
+    }
+
+    /* Get host_data used in LAUNCH_FINISH */
+    if (config_read_blob(key_file, "host_data", (uint8_t *)&finish->host_data,
+                         sizeof(finish->host_data), errp)) {
+        goto err;
+    }
+
+    return 0;
+
+err:
+    g_free(id_block);
+    g_free(id_auth);
+    return 1;
+}
+
+static void
+sev_guest_set_launch_config_file(Object *obj, const char *value, Error **errp)
+{
+    SevGuestState *s = SEV_GUEST(obj);
+
+    if (snp_parse_launch_config(s, value, errp)) {
+        return;
+    }
+
+    s->launch_config_file = g_strdup(value);
+}
+
 static void
 sev_guest_class_init(ObjectClass *oc, void *data)
 {
@@ -316,6 +532,16 @@ sev_guest_class_init(ObjectClass *oc, void *data)
                                   sev_guest_set_session_file);
     object_class_property_set_description(oc, "session-file",
             "guest owners session parameters (encoded with base64)");
+    object_class_property_add_bool(oc, "snp",
+                                   sev_guest_get_snp,
+                                   sev_guest_set_snp);
+    object_class_property_set_description(oc, "snp",
+            "enable SEV-SNP support");
+    object_class_property_add_str(oc, "launch-config",
+                                  sev_guest_get_launch_config_file,
+                                  sev_guest_set_launch_config_file);
+    object_class_property_set_description(oc, "launch-config",
+            "the file provides the SEV-SNP guest launch parameters");
 }
 
 static void
@@ -325,6 +551,7 @@ sev_guest_instance_init(Object *obj)
 
     sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
     sev->policy = DEFAULT_GUEST_POLICY;
+    sev->snp_config.start.policy = DEFAULT_SEV_SNP_POLICY;
     object_property_add_uint32_ptr(obj, "policy", &sev->policy,
                                    OBJ_PROP_FLAG_READWRITE);
     object_property_add_uint32_ptr(obj, "handle", &sev->handle,
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 3/6] i386/sev: initialize SNP context
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
  2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-15  9:32   ` Dov Murik
  2021-07-09 21:55 ` [RFC PATCH 4/6] i386/sev: add the SNP launch start context Brijesh Singh
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
the platform. The command checks whether SNP is enabled in the KVM, if
enabled then it allocate a new ASID from the SNP pool and calls the
firmware to initialize the all the resources.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c      | 24 +++++++++++++++++++++---
 target/i386/sev_i386.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6b238ef969..84ae244af0 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -583,10 +583,17 @@ sev_enabled(void)
     return !!sev_guest;
 }
 
+bool
+sev_snp_enabled(void)
+{
+    return sev_guest->snp;
+}
+
 bool
 sev_es_enabled(void)
 {
-    return sev_enabled() && (sev_guest->policy & SEV_POLICY_ES);
+    return sev_snp_enabled() ||
+           (sev_enabled() && (sev_guest->policy & SEV_POLICY_ES));
 }
 
 uint64_t
@@ -1008,6 +1015,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
+    void *init_args = NULL;
 
     if (!sev) {
         return 0;
@@ -1061,7 +1069,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     sev->api_major = status.api_major;
     sev->api_minor = status.api_minor;
 
-    if (sev_es_enabled()) {
+    if (sev_snp_enabled()) {
+        if (!kvm_kernel_irqchip_allowed()) {
+            error_report("%s: SEV-SNP guests require in-kernel irqchip support",
+                         __func__);
+            goto err;
+        }
+
+        cmd = KVM_SEV_SNP_INIT;
+        init_args = (void *)&sev->snp_config.init;
+
+    } else if (sev_es_enabled()) {
         if (!kvm_kernel_irqchip_allowed()) {
             error_report("%s: SEV-ES guests require in-kernel irqchip support",
                          __func__);
@@ -1080,7 +1098,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     }
 
     trace_kvm_sev_init();
-    ret = sev_ioctl(sev->sev_fd, cmd, NULL, &fw_error);
+    ret = sev_ioctl(sev->sev_fd, cmd, init_args, &fw_error);
     if (ret) {
         error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
                    __func__, ret, fw_error, fw_error_to_str(fw_error));
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d840478..e0e1a599be 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -29,6 +29,7 @@
 #define SEV_POLICY_SEV          0x20
 
 extern bool sev_es_enabled(void);
+extern bool sev_snp_enabled(void);
 extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
 extern uint32_t sev_get_cbit_position(void);
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 4/6] i386/sev: add the SNP launch start context
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
                   ` (2 preceding siblings ...)
  2021-07-09 21:55 ` [RFC PATCH 3/6] i386/sev: initialize SNP context Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-19 12:34   ` Dov Murik
  2021-07-09 21:55 ` [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled Brijesh Singh
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

The SNP_LAUNCH_START is called first to create a cryptographic launch
context within the firmware.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c        | 30 +++++++++++++++++++++++++++++-
 target/i386/trace-events |  1 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 84ae244af0..259408a8f1 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
     return 0;
 }
 
+static int
+sev_snp_launch_start(SevGuestState *sev)
+{
+    int ret = 1;
+    int fw_error, rc;
+    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
+
+    trace_kvm_sev_snp_launch_start(start->policy);
+
+    rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error);
+    if (rc < 0) {
+        error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'",
+                __func__, ret, fw_error, fw_error_to_str(fw_error));
+        goto out;
+    }
+
+    sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
+    ret = 0;
+
+out:
+    return ret;
+}
+
 static int
 sev_launch_start(SevGuestState *sev)
 {
@@ -1105,7 +1128,12 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         goto err;
     }
 
-    ret = sev_launch_start(sev);
+    if (sev_snp_enabled()) {
+        ret = sev_snp_launch_start(sev);
+    } else {
+        ret = sev_launch_start(sev);
+    }
+
     if (ret) {
         error_setg(errp, "%s: failed to create encryption context", __func__);
         goto err;
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 2cd8726eeb..18cc14b956 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -11,3 +11,4 @@ kvm_sev_launch_measurement(const char *value) "data %s"
 kvm_sev_launch_finish(void) ""
 kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
 kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
+kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
                   ` (3 preceding siblings ...)
  2021-07-09 21:55 ` [RFC PATCH 4/6] i386/sev: add the SNP launch start context Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-14 17:08   ` Connor Kuehl
  2021-07-19 13:00   ` Dov Murik
  2021-07-09 21:55 ` [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch Brijesh Singh
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
image used for booting the SEV-SNP guest.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
 target/i386/trace-events |  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 259408a8f1..41dcb084d1 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -883,6 +883,30 @@ out:
     return ret;
 }
 
+static int
+sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type)
+{
+    int ret, fw_error;
+    struct kvm_sev_snp_launch_update update = {};
+
+    if (!addr || !len) {
+        return 1;
+    }
+
+    update.uaddr = (__u64)(unsigned long)addr;
+    update.len = len;
+    update.page_type = type;
+    trace_kvm_sev_snp_launch_update(addr, len, type);
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE,
+                    &update, &fw_error);
+    if (ret) {
+        error_report("%s: SNP_LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
+                __func__, ret, fw_error, fw_error_to_str(fw_error));
+    }
+
+    return ret;
+}
+
 static int
 sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
 {
@@ -1161,7 +1185,14 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
 
     /* if SEV is in update state then encrypt the data else do nothing */
     if (sev_check_state(sev_guest, SEV_STATE_LAUNCH_UPDATE)) {
-        int ret = sev_launch_update_data(sev_guest, ptr, len);
+        int ret;
+
+        if (sev_snp_enabled()) {
+            ret = sev_snp_launch_update(sev_guest, ptr, len,
+                                        KVM_SEV_SNP_PAGE_TYPE_NORMAL);
+        } else {
+            ret = sev_launch_update_data(sev_guest, ptr, len);
+        }
         if (ret < 0) {
             error_setg(errp, "failed to encrypt pflash rom");
             return ret;
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 18cc14b956..0c2d250206 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -12,3 +12,4 @@ kvm_sev_launch_finish(void) ""
 kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
 kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
 kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
+kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
                   ` (4 preceding siblings ...)
  2021-07-09 21:55 ` [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled Brijesh Singh
@ 2021-07-09 21:55 ` Brijesh Singh
  2021-07-14 17:29   ` Dr. David Alan Gilbert
  2021-07-19 11:24   ` Dov Murik
  2021-07-12 17:00 ` [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Tom Lendacky
  2021-07-13  8:05 ` Dov Murik
  7 siblings, 2 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-09 21:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost, Brijesh Singh

During the SNP guest launch sequence, a special secrets and cpuid page
needs to be populated by the SEV-SNP firmware. The secrets page contains
the VM Platform Communication Key (VMPCKs) used by the guest to send and
receive secure messages to the PSP. And CPUID page will contain the CPUID
value filtered through the PSP.

The guest BIOS (OVMF) reserves these pages in MEMFD and location of it
is available through the SNP boot block GUID. While finalizing the guest
boot flow, lookup for the boot block and call the SNP_LAUNCH_UPDATE
command to populate secrets and cpuid pages.

In order to support early boot code, the OVMF may ask hypervisor to
request the pre-validation of certain memory range. If such range is
present the call LAUNCH_UPDATE command to validate those address range
without affecting the measurement. See the SEV-SNP specification for
further details.

Finally, call the SNP_LAUNCH_FINISH to finalize the guest boot.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c        | 184 ++++++++++++++++++++++++++++++++++++++-
 target/i386/trace-events |   2 +
 2 files changed, 184 insertions(+), 2 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 41dcb084d1..f438e09d33 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -93,6 +93,19 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
     uint32_t reset_addr;
 } SevInfoBlock;
 
+#define SEV_SNP_BOOT_BLOCK_GUID "bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9"
+typedef struct __attribute__((__packed__)) SevSnpBootInfoBlock {
+    /* Prevalidate range address */
+    uint32_t pre_validated_start;
+    uint32_t pre_validated_end;
+    /* Secrets page address */
+    uint32_t secrets_addr;
+    uint32_t secrets_len;
+    /* CPUID page address */
+    uint32_t cpuid_addr;
+    uint32_t cpuid_len;
+} SevSnpBootInfoBlock;
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1014,6 +1027,158 @@ static Notifier sev_machine_done_notify = {
     .notify = sev_launch_get_measure,
 };
 
+static int
+sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
+{
+    void *hva;
+    MemoryRegion *mr = NULL;
+
+    hva = gpa2hva(&mr, hwaddr, size, NULL);
+    if (!hva) {
+        error_report("SEV-SNP failed to get HVA for GPA 0x%x", hwaddr);
+        return 1;
+    }
+
+    return sev_snp_launch_update(sev_guest, hva, size, type);
+}
+
+struct snp_pre_validated_range {
+    uint32_t start;
+    uint32_t end;
+};
+
+static struct snp_pre_validated_range pre_validated[2];
+
+static bool
+detectoverlap(uint32_t start, uint32_t end,
+              struct snp_pre_validated_range *overlap)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
+        if (pre_validated[i].start < end && start < pre_validated[i].end) {
+            memcpy(overlap, &pre_validated[i], sizeof(*overlap));
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static void snp_ovmf_boot_block_setup(void)
+{
+    struct snp_pre_validated_range overlap;
+    SevSnpBootInfoBlock *info;
+    uint32_t start, end, sz;
+    int ret;
+
+    /*
+     * Extract the SNP boot block for the SEV-SNP guests by locating the
+     * SNP_BOOT GUID. The boot block contains the information such as location
+     * of secrets and CPUID page, additionaly it may contain the range of
+     * memory that need to be pre-validated for the boot.
+     */
+    if (!pc_system_ovmf_table_find(SEV_SNP_BOOT_BLOCK_GUID,
+        (uint8_t **)&info, NULL)) {
+        error_report("SEV-SNP: failed to find the SNP boot block");
+        exit(1);
+    }
+
+    trace_kvm_sev_snp_ovmf_boot_block_info(info->secrets_addr,
+                                           info->secrets_len, info->cpuid_addr,
+                                           info->cpuid_len,
+                                           info->pre_validated_start,
+                                           info->pre_validated_end);
+
+    /* Populate the secrets page */
+    ret = sev_snp_launch_update_gpa(info->secrets_addr, info->secrets_len,
+                                    KVM_SEV_SNP_PAGE_TYPE_SECRETS);
+    if (ret) {
+        error_report("SEV-SNP: failed to insert secret page GPA 0x%x",
+                     info->secrets_addr);
+        exit(1);
+    }
+
+    /* Populate the cpuid page */
+    ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len,
+                                    KVM_SEV_SNP_PAGE_TYPE_CPUID);
+    if (ret) {
+        error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x",
+                     info->cpuid_addr);
+        exit(1);
+    }
+
+    /*
+     * Pre-validate the range using the LAUNCH_UPDATE_DATA, if the
+     * pre-validation range contains the CPUID and Secret page GPA then skip
+     * it. This is because SEV-SNP firmware pre-validates those pages as part
+     * of adding secrets and cpuid LAUNCH_UPDATE type.
+     */
+    pre_validated[0].start = info->secrets_addr;
+    pre_validated[0].end = info->secrets_addr + info->secrets_len;
+    pre_validated[1].start = info->cpuid_addr;
+    pre_validated[1].end = info->cpuid_addr + info->cpuid_len;
+    start = info->pre_validated_start;
+    end = info->pre_validated_end;
+
+    while (start < end) {
+        /* Check if the requested range overlaps with Secrets and CPUID page */
+        if (detectoverlap(start, end, &overlap)) {
+            if (start < overlap.start) {
+                sz = overlap.start - start;
+                if (sev_snp_launch_update_gpa(start, sz,
+                    KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
+                    error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
+                                 start, sz);
+                    exit(1);
+                }
+            }
+
+            start = overlap.end;
+            continue;
+        }
+
+        /* Validate the remaining range */
+        if (sev_snp_launch_update_gpa(start, end - start,
+            KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
+            error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
+                         start, end - start);
+            exit(1);
+        }
+
+        start = end;
+    }
+}
+
+static void
+sev_snp_launch_finish(SevGuestState *sev)
+{
+    int ret, error;
+    Error *local_err = NULL;
+    struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
+
+    trace_kvm_sev_snp_launch_finish();
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, finish, &error);
+    if (ret) {
+        error_report("%s: SNP_LAUNCH_FINISH ret=%d fw_error=%d '%s'",
+                     __func__, ret, error, fw_error_to_str(error));
+        exit(1);
+    }
+
+    sev_set_guest_state(sev, SEV_STATE_RUNNING);
+
+    /* add migration blocker */
+    error_setg(&sev_mig_blocker,
+               "SEV: Migration is not implemented");
+    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        error_free(sev_mig_blocker);
+        exit(1);
+    }
+}
+
+
 static void
 sev_launch_finish(SevGuestState *sev)
 {
@@ -1048,7 +1213,12 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
 
     if (running) {
         if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
-            sev_launch_finish(sev);
+            if (sev_snp_enabled()) {
+                snp_ovmf_boot_block_setup();
+                sev_snp_launch_finish(sev);
+            } else {
+                sev_launch_finish(sev);
+            }
         }
     }
 }
@@ -1164,7 +1334,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     }
 
     ram_block_notifier_add(&sev_ram_notifier);
-    qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
+
+    /*
+     * The machine done notify event is used by the SEV guest to get the
+     * measurement of the encrypted images. When SEV-SNP is enabled then
+     * measurement is part of the attestation report and the measurement
+     * command does not exist. So skip registering the notifier.
+     */
+    if (!sev_snp_enabled()) {
+        qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
+    }
+
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
     cgs->ready = true;
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 0c2d250206..db91287439 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -13,3 +13,5 @@ kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa
 kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
 kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
 kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
+kvm_sev_snp_launch_finish(void) ""
+kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"
-- 
2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 1/6] linux-header: add the SNP specific command
  2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
@ 2021-07-10 20:32   ` Michael S. Tsirkin
  2021-07-12 15:48     ` Brijesh Singh
  2021-07-19 11:35   ` Dov Murik
  1 sibling, 1 reply; 45+ messages in thread
From: Michael S. Tsirkin @ 2021-07-10 20:32 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Connor Kuehl, Michael Roth,
	James Bottomley, qemu-devel, Dr . David Alan Gilbert, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, Jul 09, 2021 at 04:55:45PM -0500, Brijesh Singh wrote:
> Sync the kvm.h with the kernel to include the SNP specific commands.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

Pls specify which kernel version you used for the sync.

> ---
>  linux-headers/linux/kvm.h | 47 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 20d6a263bb..c17ace1ece 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1679,6 +1679,12 @@ enum sev_cmd_id {
>  	/* Guest Migration Extension */
>  	KVM_SEV_SEND_CANCEL,
>  
> +	/* SNP specific commands */
> +	KVM_SEV_SNP_INIT = 256,
> +	KVM_SEV_SNP_LAUNCH_START,
> +	KVM_SEV_SNP_LAUNCH_UPDATE,
> +	KVM_SEV_SNP_LAUNCH_FINISH,
> +
>  	KVM_SEV_NR_MAX,
>  };
>  
> @@ -1775,6 +1781,47 @@ struct kvm_sev_receive_update_data {
>  	__u32 trans_len;
>  };
>  
> +struct kvm_snp_init {
> +	__u64 flags;
> +};
> +
> +struct kvm_sev_snp_launch_start {
> +	__u64 policy;
> +	__u64 ma_uaddr;
> +	__u8 ma_en;
> +	__u8 imi_en;
> +	__u8 gosvw[16];
> +};
> +
> +#define KVM_SEV_SNP_PAGE_TYPE_NORMAL		0x1
> +#define KVM_SEV_SNP_PAGE_TYPE_VMSA		0x2
> +#define KVM_SEV_SNP_PAGE_TYPE_ZERO		0x3
> +#define KVM_SEV_SNP_PAGE_TYPE_UNMEASURED	0x4
> +#define KVM_SEV_SNP_PAGE_TYPE_SECRETS		0x5
> +#define KVM_SEV_SNP_PAGE_TYPE_CPUID		0x6
> +
> +struct kvm_sev_snp_launch_update {
> +	__u64 uaddr;
> +	__u32 len;
> +	__u8 imi_page;
> +	__u8 page_type;
> +	__u8 vmpl3_perms;
> +	__u8 vmpl2_perms;
> +	__u8 vmpl1_perms;
> +};
> +
> +#define KVM_SEV_SNP_ID_BLOCK_SIZE	96
> +#define KVM_SEV_SNP_ID_AUTH_SIZE	4096
> +#define KVM_SEV_SNP_FINISH_DATA_SIZE	32
> +
> +struct kvm_sev_snp_launch_finish {
> +	__u64 id_block_uaddr;
> +	__u64 id_auth_uaddr;
> +	__u8 id_block_en;
> +	__u8 auth_key_en;
> +	__u8 host_data[KVM_SEV_SNP_FINISH_DATA_SIZE];
> +};
> +
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
>  #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
> -- 
> 2.17.1



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
@ 2021-07-12  6:09   ` Dov Murik
  2021-07-12 14:34   ` Dr. David Alan Gilbert
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 45+ messages in thread
From: Dov Murik @ 2021-07-12  6:09 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel, Connor Kuehl
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Michael Roth,
	James Bottomley, Dr . David Alan Gilbert, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson



On 10/07/2021 0:55, Brijesh Singh wrote:
> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> Passing all parameters through command line can be difficult. To simplify
> the launch parameter passing, introduce a .ini-like config file that can be
> used for passing the parameters to the launch flow.
> 
> The contents of the config file will look like this:
> 
> $ cat snp-launch.init
> 
> # SNP launch parameters
> [SEV-SNP]
> init_flags = 0
> policy = 0x1000
> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> 
> 
> Add 'snp' property that can be used to indicate that SEV guest launch
> should enable the SNP support.
> 
> SEV-SNP guest launch examples:
> 
> 1) launch without additional parameters
> 
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on
> 
> 2) launch with optional parameters
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on,launch-config=<file>
> 

Not directly SNP-related, but in an internal communication Connor told
me he wants to allow the SEV configuration (like dh-cert-file etc.) to
be set using QMP commands when the machine launches instead (or in
addition to) setting them via QEMU command-line parameters.

Whatever the configuration solution decided for the SEV parameters
should also apply to these new SNP settings (policy, id_block, etc.).

-Dov


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
  2021-07-12  6:09   ` Dov Murik
@ 2021-07-12 14:34   ` Dr. David Alan Gilbert
  2021-07-12 15:59     ` Brijesh Singh
  2021-07-12 14:43   ` Daniel P. Berrangé
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-12 14:34 UTC (permalink / raw)
  To: Brijesh Singh, armbru
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, qemu-devel, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

cc'ing in armbru, since he knows about our command line - have we got a
neater way of doing this, or something else that reads config file?
Could the existing -readconfig work?

Although this is a fairly large chunk of data, I don't think it's any
larger than our block device configs on a bad day.

Dave

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> Passing all parameters through command line can be difficult. To simplify
> the launch parameter passing, introduce a .ini-like config file that can be
> used for passing the parameters to the launch flow.
> 
> The contents of the config file will look like this:
> 
> $ cat snp-launch.init
> 
> # SNP launch parameters
> [SEV-SNP]
> init_flags = 0
> policy = 0x1000
> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="

Wouldn't the 'gosvw' and 'hostdata' also be in there?

Dave

> 
> Add 'snp' property that can be used to indicate that SEV guest launch
> should enable the SNP support.
> 
> SEV-SNP guest launch examples:
> 
> 1) launch without additional parameters
> 
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on
> 
> 2) launch with optional parameters
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on,launch-config=<file>
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/amd-memory-encryption.txt |  81 +++++++++++-
>  qapi/qom.json                  |   6 +
>  target/i386/sev.c              | 227 +++++++++++++++++++++++++++++++++
>  3 files changed, 312 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index ffca382b5f..322bf38f68 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -22,8 +22,8 @@ support for notifying a guest's operating system when certain types of VMEXITs
>  are about to occur. This allows the guest to selectively share information with
>  the hypervisor to satisfy the requested function.
>  
> -Launching
> ----------
> +Launching (SEV and SEV-ES)
> +--------------------------
>  Boot images (such as bios) must be encrypted before a guest can be booted. The
>  MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: LAUNCH_START,
>  LAUNCH_UPDATE_DATA, LAUNCH_MEASURE and LAUNCH_FINISH. These four commands
> @@ -113,6 +113,83 @@ a SEV-ES guest:
>   - Requires in-kernel irqchip - the burden is placed on the hypervisor to
>     manage booting APs.
>  
> +Launching (SEV-SNP)
> +-------------------
> +Boot images (such as bios) must be encrypted before a guest can be booted. The
> +MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images:
> +KVM_SNP_INIT, SNP_LAUNCH_START, SNP_LAUNCH_UPDATE, and SNP_LAUNCH_FINISH. These
> +four commands together generate a fresh memory encryption key for the VM,
> +encrypt the boot images for a successful launch.
> +
> +KVM_SNP_INIT is called first to initialize the SEV-SNP firmware and SNP
> +features in the KVM. The feature flags value can be provided through the
> +launch-config file.
> +
> ++------------+-------+----------+---------------------------------+
> +| key        | type  | default  | meaning                         |
> ++------------+-------+----------+---------------------------------+
> +| init_flags | hex   | 0        | SNP feature flags               |
> ++-----------------------------------------------------------------+
> +
> +Note: currently the init_flags must be zero.
> +
> +SNP_LAUNCH_START is called first to create a cryptographic launch context
> +within the firmware. To create this context, guest owner must provide a guest
> +policy and other parameters as described in the SEV-SNP firmware
> +specification. The launch parameters should be specified in the launch-config
> +ini file and should be treated as a binary blob and must be passed as-is to
> +the SEV-SNP firmware.
> +
> +The SNP_LAUNCH_START uses the following parameters from the launch-config
> +file. See the SEV-SNP specification for more details.
> +
> ++--------+-------+----------+----------------------------------------------+
> +| key    | type  | default  | meaning                                      |
> ++--------+-------+----------+----------------------------------------------+
> +| policy | hex   | 0x30000  | a 64-bit guest policy                        |
> +| imi_en | bool  | 0        | 1 when IMI is enabled                        |
> +| ma_end | bool  | 0        | 1 when migration agent is used               |
> +| gosvw  | string| 0        | 16-byte base64 encoded string for the guest  |
> +|        |       |          | OS visible workaround.                       |
> ++--------+-------+----------+----------------------------------------------+
> +
> +SNP_LAUNCH_UPDATE encrypts the memory region using the cryptographic context
> +created via the SNP_LAUNCH_START command. If required, this command can be called
> +multiple times to encrypt different memory regions. The command also calculates
> +the measurement of the memory contents as it encrypts.
> +
> +SNP_LAUNCH_FINISH finalizes the guest launch flow. Optionally, while finalizing
> +the launch the firmware can perform checks on the launch digest computing
> +through the SNP_LAUNCH_UPDATE. To perform the check the user must supply
> +the id block, authentication blob and host data that should be included in the
> +attestation report. See the SEV-SNP spec for further details.
> +
> +The SNP_LAUNCH_FINISH uses the following parameters from the launch-config file.
> +
> ++------------+-------+----------+----------------------------------------------+
> +| key        | type  | default  | meaning                                      |
> ++------------+-------+----------+----------------------------------------------+
> +| id_block   | string| none     | base64 encoded ID block                      |
> ++------------+-------+----------+----------------------------------------------+
> +| id_auth    | string| none     | base64 encoded authentication information    |
> ++------------+-------+----------+----------------------------------------------+
> +| auth_key_en| bool  | 0        | auth block contains author key               |
> ++------------+-------+----------+----------------------------------------------+
> +| host_data  | string| none     | host provided data                           |
> ++------------+-------+----------+----------------------------------------------+
> +
> +To launch a SEV-SNP guest
> +
> +# ${QEMU} \
> +    -machine ...,confidential-guest-support=sev0 \
> +    -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on
> +
> +To launch a SEV-SNP guest with launch configuration
> +
> +# ${QEMU} \
> +    -machine ...,confidential-guest-support=sev0 \
> +    -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on,launch-config=<config>
> +
>  Debugging
>  -----------
>  Since the memory contents of a SEV guest are encrypted, hypervisor access to
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 652be317b8..bdf89fda27 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -749,6 +749,10 @@
>  # @reduced-phys-bits: number of bits in physical addresses that become
>  #                     unavailable when SEV is enabled
>  #
> +# @snp: SEV-SNP is enabled (default: 0)
> +#
> +# @launch-config: launch config file to use
> +#
>  # Since: 2.12
>  ##
>  { 'struct': 'SevGuestProperties',
> @@ -758,6 +762,8 @@
>              '*policy': 'uint32',
>              '*handle': 'uint32',
>              '*cbitpos': 'uint32',
> +            '*snp': 'bool',
> +            '*launch-config': 'str',
>              'reduced-phys-bits': 'uint32' } }
>  
>  ##
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6..6b238ef969 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -37,6 +37,11 @@
>  #define TYPE_SEV_GUEST "sev-guest"
>  OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
>  
> +struct snp_launch_config {
> +    struct kvm_snp_init init;
> +    struct kvm_sev_snp_launch_start start;
> +    struct kvm_sev_snp_launch_finish finish;
> +};
>  
>  /**
>   * SevGuestState:
> @@ -58,6 +63,8 @@ struct SevGuestState {
>      char *session_file;
>      uint32_t cbitpos;
>      uint32_t reduced_phys_bits;
> +    char *launch_config_file;
> +    bool snp;
>  
>      /* runtime state */
>      uint32_t handle;
> @@ -72,10 +79,13 @@ struct SevGuestState {
>      uint32_t reset_cs;
>      uint32_t reset_ip;
>      bool reset_data_valid;
> +
> +    struct snp_launch_config snp_config;
>  };
>  
>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
> +#define DEFAULT_SEV_SNP_POLICY  0x30000
>  
>  #define SEV_INFO_BLOCK_GUID     "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
>  typedef struct __attribute__((__packed__)) SevInfoBlock {
> @@ -298,6 +308,212 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
>      sev->sev_device = g_strdup(value);
>  }
>  
> +static void
> +sev_guest_set_snp(Object *obj, bool value, Error **errp)
> +{
> +    SevGuestState *sev = SEV_GUEST(obj);
> +
> +    sev->snp = value;
> +}
> +
> +static bool
> +sev_guest_get_snp(Object *obj, Error **errp)
> +{
> +    SevGuestState *sev = SEV_GUEST(obj);
> +
> +    return sev->snp;
> +}
> +
> +
> +static char *
> +sev_guest_get_launch_config_file(Object *obj, Error **errp)
> +{
> +    SevGuestState *s = SEV_GUEST(obj);
> +
> +    return g_strdup(s->launch_config_file);
> +}
> +
> +static int
> +config_read_uint64(GKeyFile *f, const char *key, uint64_t *value, Error **errp)
> +{
> +    g_autoptr(GError) error = NULL;
> +    g_autofree gchar *str = NULL;
> +    uint64_t res;
> +
> +    str = g_key_file_get_string(f, "SEV-SNP", key, &error);
> +    if (!str) {
> +        /* key not found */
> +        return 0;
> +    }
> +
> +    res = g_ascii_strtoull(str, NULL, 16);
> +    if (res == G_MAXUINT64) {
> +        error_setg(errp, "Failed to convert %s", str);
> +        return 1;
> +    }
> +
> +    *value = res;
> +    return 0;
> +}
> +
> +static int
> +config_read_bool(GKeyFile *f, const char *key, bool *value, Error **errp)
> +{
> +    g_autoptr(GError) error = NULL;
> +    gboolean val;
> +
> +    val = g_key_file_get_boolean(f, "SEV-SNP", key, &error);
> +    if (!val && g_error_matches(error, G_KEY_FILE_ERROR,
> +                                 G_KEY_FILE_ERROR_INVALID_VALUE)) {
> +        error_setg(errp, "%s", error->message);
> +        return 1;
> +    }
> +
> +    *value = val;
> +    return 0;
> +}
> +
> +static int
> +config_read_blob(GKeyFile *f, const char *key, uint8_t *blob, uint32_t len,
> +                 Error **errp)
> +{
> +    g_autoptr(GError) error = NULL;
> +    g_autofree guchar *data = NULL;
> +    g_autofree gchar *base64 = NULL;
> +    gsize size;
> +
> +    base64 = g_key_file_get_string(f, "SEV-SNP", key, &error);
> +    if (!base64) {
> +        /* key not found */
> +        return 0;
> +    }
> +
> +    /* lets decode the value string */
> +    data = g_base64_decode(base64, &size);
> +    if (!data) {
> +        error_setg(errp, "failed to decode '%s'", key);
> +        return 1;
> +    }
> +
> +    /* verify the length */
> +    if (len != size) {
> +        error_setg(errp, "invalid length for key '%s' (expected %d got %ld)",
> +                   key, len, size);
> +        return 1;
> +    }
> +
> +    memcpy(blob, data, size);
> +    return 0;
> +}
> +
> +static int
> +snp_parse_launch_config(SevGuestState *sev, const char *file, Error **errp)
> +{
> +    g_autoptr(GError) error = NULL;
> +    g_autoptr(GKeyFile) key_file = g_key_file_new();
> +    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
> +    struct kvm_snp_init *init = &sev->snp_config.init;
> +    struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
> +    uint8_t *id_block = NULL, *id_auth = NULL;
> +
> +    if (!g_key_file_load_from_file(key_file, file, G_KEY_FILE_NONE, &error)) {
> +        error_setg(errp, "Error loading config file: %s", error->message);
> +        return 1;
> +    }
> +
> +    /* Check the group first */
> +    if (!g_key_file_has_group(key_file, "SEV-SNP")) {
> +        error_setg(errp, "Error parsing config file, group SEV-SNP not found");
> +        return 1;
> +    }
> +
> +    /* Get the init_flags used in KVM_SNP_INIT */
> +    if (config_read_uint64(key_file, "init_flags",
> +                           (uint64_t *)&init->flags, errp)) {
> +        goto err;
> +    }
> +
> +    /* Get the policy used in LAUNCH_START */
> +    if (config_read_uint64(key_file, "policy",
> +                           (uint64_t *)&start->policy, errp)) {
> +        goto err;
> +    }
> +
> +    /* Get IMI_EN used in LAUNCH_START */
> +    if (config_read_bool(key_file, "imi_en", (bool *)&start->imi_en, errp)) {
> +        goto err;
> +    }
> +
> +    /* Get MA_EN used in LAUNCH_START */
> +    if (config_read_bool(key_file, "imi_en", (bool *)&start->ma_en, errp)) {
> +        goto err;
> +    }
> +
> +    /* Get GOSVW used in LAUNCH_START */
> +    if (config_read_blob(key_file, "gosvw", (uint8_t *)&start->gosvw,
> +                         sizeof(start->gosvw), errp)) {
> +        goto err;
> +    }
> +
> +    /* Get ID block used in LAUNCH_FINISH */
> +    if (g_key_file_has_key(key_file, "SEV-SNP", "id_block", &error)) {
> +
> +        id_block = g_malloc(KVM_SEV_SNP_ID_BLOCK_SIZE);
> +
> +        if (config_read_blob(key_file, "id_block", id_block,
> +                             KVM_SEV_SNP_ID_BLOCK_SIZE, errp)) {
> +            goto err;
> +        }
> +
> +        finish->id_block_uaddr = (unsigned long)id_block;
> +        finish->id_block_en = 1;
> +    }
> +
> +    /* Get authentication block used in LAUNCH_FINISH */
> +    if (g_key_file_has_key(key_file, "SEV-SNP", "id_auth", &error)) {
> +
> +        id_auth = g_malloc(KVM_SEV_SNP_ID_AUTH_SIZE);
> +
> +        if (config_read_blob(key_file, "auth_block", id_auth,
> +                             KVM_SEV_SNP_ID_AUTH_SIZE, errp)) {
> +            goto err;
> +        }
> +
> +        finish->id_auth_uaddr = (unsigned long)id_auth;
> +
> +        /* Get AUTH_KEY_EN used in LAUNCH_FINISH */
> +        if (config_read_bool(key_file, "auth_key_en",
> +                             (bool *)&finish->auth_key_en, errp)) {
> +            goto err;
> +        }
> +    }
> +
> +    /* Get host_data used in LAUNCH_FINISH */
> +    if (config_read_blob(key_file, "host_data", (uint8_t *)&finish->host_data,
> +                         sizeof(finish->host_data), errp)) {
> +        goto err;
> +    }
> +
> +    return 0;
> +
> +err:
> +    g_free(id_block);
> +    g_free(id_auth);
> +    return 1;
> +}
> +
> +static void
> +sev_guest_set_launch_config_file(Object *obj, const char *value, Error **errp)
> +{
> +    SevGuestState *s = SEV_GUEST(obj);
> +
> +    if (snp_parse_launch_config(s, value, errp)) {
> +        return;
> +    }
> +
> +    s->launch_config_file = g_strdup(value);
> +}
> +
>  static void
>  sev_guest_class_init(ObjectClass *oc, void *data)
>  {
> @@ -316,6 +532,16 @@ sev_guest_class_init(ObjectClass *oc, void *data)
>                                    sev_guest_set_session_file);
>      object_class_property_set_description(oc, "session-file",
>              "guest owners session parameters (encoded with base64)");
> +    object_class_property_add_bool(oc, "snp",
> +                                   sev_guest_get_snp,
> +                                   sev_guest_set_snp);
> +    object_class_property_set_description(oc, "snp",
> +            "enable SEV-SNP support");
> +    object_class_property_add_str(oc, "launch-config",
> +                                  sev_guest_get_launch_config_file,
> +                                  sev_guest_set_launch_config_file);
> +    object_class_property_set_description(oc, "launch-config",
> +            "the file provides the SEV-SNP guest launch parameters");
>  }
>  
>  static void
> @@ -325,6 +551,7 @@ sev_guest_instance_init(Object *obj)
>  
>      sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>      sev->policy = DEFAULT_GUEST_POLICY;
> +    sev->snp_config.start.policy = DEFAULT_SEV_SNP_POLICY;
>      object_property_add_uint32_ptr(obj, "policy", &sev->policy,
>                                     OBJ_PROP_FLAG_READWRITE);
>      object_property_add_uint32_ptr(obj, "handle", &sev->handle,
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
  2021-07-12  6:09   ` Dov Murik
  2021-07-12 14:34   ` Dr. David Alan Gilbert
@ 2021-07-12 14:43   ` Daniel P. Berrangé
  2021-07-12 15:56     ` Brijesh Singh
  2021-07-13 13:46   ` Markus Armbruster
  2021-07-13 18:21   ` Eric Blake
  4 siblings, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2021-07-12 14:43 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Eduardo Habkost, kvm, Michael S . Tsirkin,
	Connor Kuehl, Michael Roth, James Bottomley, qemu-devel,
	Dr . David Alan Gilbert, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote:
> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> Passing all parameters through command line can be difficult.

This sentence applies to pretty much everything in QEMU and the
SEV-SNP example is nowhere near an extreme example IMHO.

>                                                              To simplify
> the launch parameter passing, introduce a .ini-like config file that can be
> used for passing the parameters to the launch flow.

Inventing a new config file format for usage by just one specific
niche feature in QEMU is something I'd say we do not want.

Our long term goal in QEMU is to move to a world where 100% of
QEMU configuration is provided in JSON format, using the QAPI
schema to define the accepted input set.  

> 
> The contents of the config file will look like this:
> 
> $ cat snp-launch.init
> 
> # SNP launch parameters
> [SEV-SNP]
> init_flags = 0
> policy = 0x1000
> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="

These parameters are really tiny and trivial to provide on the command
line, so I'm not finding this config file compelling.

> 
> 
> Add 'snp' property that can be used to indicate that SEV guest launch
> should enable the SNP support.
> 
> SEV-SNP guest launch examples:
> 
> 1) launch without additional parameters
> 
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on
> 
> 2) launch with optional parameters
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on,launch-config=<file>
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/amd-memory-encryption.txt |  81 +++++++++++-
>  qapi/qom.json                  |   6 +
>  target/i386/sev.c              | 227 +++++++++++++++++++++++++++++++++
>  3 files changed, 312 insertions(+), 2 deletions(-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 1/6] linux-header: add the SNP specific command
  2021-07-10 20:32   ` Michael S. Tsirkin
@ 2021-07-12 15:48     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-12 15:48 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	James Bottomley, Dr . David Alan Gilbert, Tom Lendacky,
	Paolo Bonzini, Dov Murik, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/10/21 3:32 PM, Michael S. Tsirkin wrote:
> On Fri, Jul 09, 2021 at 04:55:45PM -0500, Brijesh Singh wrote:
>> Sync the kvm.h with the kernel to include the SNP specific commands.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> 
> Pls specify which kernel version you used for the sync.
> 

This sync is based on the my guest kernel rfc patches (5.13-rc6). After 
the guest patches are accepted then will include the exact linux kernel 
version.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-12 14:43   ` Daniel P. Berrangé
@ 2021-07-12 15:56     ` Brijesh Singh
  2021-07-12 16:24       ` Daniel P. Berrangé
  0 siblings, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-12 15:56 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson, kvm,
	Michael Roth, Eduardo Habkost



On 7/12/21 9:43 AM, Daniel P. Berrangé wrote:
> On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote:
>> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
>> Passing all parameters through command line can be difficult.
> 
> This sentence applies to pretty much everything in QEMU and the
> SEV-SNP example is nowhere near an extreme example IMHO.
> 
>>                                                               To simplify
>> the launch parameter passing, introduce a .ini-like config file that can be
>> used for passing the parameters to the launch flow.
> 
> Inventing a new config file format for usage by just one specific
> niche feature in QEMU is something I'd say we do not want.
> 
> Our long term goal in QEMU is to move to a world where 100% of
> QEMU configuration is provided in JSON format, using the QAPI
> schema to define the accepted input set.
> 

I am open to all suggestions. I was trying to avoid passing all these 
parameters through the command line because some of them can be huge (up 
to a page size)


>>
>> The contents of the config file will look like this:
>>
>> $ cat snp-launch.init
>>
>> # SNP launch parameters
>> [SEV-SNP]
>> init_flags = 0
>> policy = 0x1000
>> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> 
> These parameters are really tiny and trivial to provide on the command
> line, so I'm not finding this config file compelling.
> 

I have only included 3 small parameters. Other parameters can be up to a 
page size. The breakdown looks like this:

policy: 8 bytes
flags: 8 bytes
id_block: 96 bytes
id_auth: 4096 bytes
host_data: 32 bytes
gosvw: 16 bytes



>>
>>
>> Add 'snp' property that can be used to indicate that SEV guest launch
>> should enable the SNP support.
>>
>> SEV-SNP guest launch examples:
>>
>> 1) launch without additional parameters
>>
>>    $(QEMU_CLI) \
>>      -object sev-guest,id=sev0,snp=on
>>
>> 2) launch with optional parameters
>>    $(QEMU_CLI) \
>>      -object sev-guest,id=sev0,snp=on,launch-config=<file>
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   docs/amd-memory-encryption.txt |  81 +++++++++++-
>>   qapi/qom.json                  |   6 +
>>   target/i386/sev.c              | 227 +++++++++++++++++++++++++++++++++
>>   3 files changed, 312 insertions(+), 2 deletions(-)
> 
> Regards,
> Daniel
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-12 14:34   ` Dr. David Alan Gilbert
@ 2021-07-12 15:59     ` Brijesh Singh
  2021-07-12 16:16       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-12 15:59 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, armbru
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Tom Lendacky,
	Paolo Bonzini, Dov Murik, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/12/21 9:34 AM, Dr. David Alan Gilbert wrote:
>>
>> $ cat snp-launch.init
>>
>> # SNP launch parameters
>> [SEV-SNP]
>> init_flags = 0
>> policy = 0x1000
>> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> 
> Wouldn't the 'gosvw' and 'hostdata' also be in there?
> 

I did not included all the 8 parameters in the commit messages, mainly 
because some of them are big. I just picked 3 smaller ones.

-Brijesh


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-12 15:59     ` Brijesh Singh
@ 2021-07-12 16:16       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-12 16:16 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, armbru, qemu-devel, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> 
> 
> On 7/12/21 9:34 AM, Dr. David Alan Gilbert wrote:
> > > 
> > > $ cat snp-launch.init
> > > 
> > > # SNP launch parameters
> > > [SEV-SNP]
> > > init_flags = 0
> > > policy = 0x1000
> > > id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> > 
> > Wouldn't the 'gosvw' and 'hostdata' also be in there?
> > 
> 
> I did not included all the 8 parameters in the commit messages, mainly
> because some of them are big. I just picked 3 smaller ones.

It would be good to have a full example, even if one of them was
something like:

  stuff = ".....<upto 4096 bytes>...."

so we'd just be able to get more of an idea.

Dave

> -Brijesh
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-12 15:56     ` Brijesh Singh
@ 2021-07-12 16:24       ` Daniel P. Berrangé
  2021-07-13 13:54         ` Brijesh Singh
  0 siblings, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2021-07-12 16:24 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Eduardo Habkost, kvm, Michael S . Tsirkin,
	Connor Kuehl, Michael Roth, James Bottomley, qemu-devel,
	Dr . David Alan Gilbert, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

On Mon, Jul 12, 2021 at 10:56:40AM -0500, Brijesh Singh wrote:
> 
> 
> On 7/12/21 9:43 AM, Daniel P. Berrangé wrote:
> > On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote:
> > > To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> > > Passing all parameters through command line can be difficult.
> > 
> > This sentence applies to pretty much everything in QEMU and the
> > SEV-SNP example is nowhere near an extreme example IMHO.
> > 
> > >                                                               To simplify
> > > the launch parameter passing, introduce a .ini-like config file that can be
> > > used for passing the parameters to the launch flow.
> > 
> > Inventing a new config file format for usage by just one specific
> > niche feature in QEMU is something I'd say we do not want.
> > 
> > Our long term goal in QEMU is to move to a world where 100% of
> > QEMU configuration is provided in JSON format, using the QAPI
> > schema to define the accepted input set.
> > 
> 
> I am open to all suggestions. I was trying to avoid passing all these
> parameters through the command line because some of them can be huge (up to
> a page size)
> 
> 
> > > 
> > > The contents of the config file will look like this:
> > > 
> > > $ cat snp-launch.init
> > > 
> > > # SNP launch parameters
> > > [SEV-SNP]
> > > init_flags = 0
> > > policy = 0x1000
> > > id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> > 
> > These parameters are really tiny and trivial to provide on the command
> > line, so I'm not finding this config file compelling.
> > 
> 
> I have only included 3 small parameters. Other parameters can be up to a
> page size. The breakdown looks like this:
> 
> policy: 8 bytes
> flags: 8 bytes
> id_block: 96 bytes
> id_auth: 4096 bytes
> host_data: 32 bytes
> gosvw: 16 bytes

Only the id_auth parameter is really considered large here.

When you say "up to a page size", that implies that the size is
actually variable.  Is that correct, and if so, what is a real
world common size going to be ? Is the common size much smaller
than this upper limit ? If so I'd just ignore the issue entirely.

If not, then, 4k on the command line is certainly ugly, but isn't
technically impossible. It would be similarly ugly to have this
value stuffed into a libvirt XML configuration for that matter.

One option is to supply only that one parameter via an external
file, with the file being an opaque blob whose context is the
parameter value thus avoiding inventing a custom file format
parser.

When "id_auth" is described as "authentication data", are there
any secrecy requirements around this ?

QEMU does have the '-object secret' framework for passing blobs
of sensitive data to QEMU and can allow passing via a file:

  https://qemu-project.gitlab.io/qemu/system/secrets.html

Even if this doesn't actually need to be kept private, we
could decide to simply (ab)use the 'secret' object anyway
as a way to let it be passed in out of band via a file.

Libvirt also has a 'secret' concept for passing blobs of
sensitive data out of band from the main XML config, which
could again be leveraged.

It does feel a little dirty to be using 'secrets' in libvirt
and QEMU for data that doesn't actually need to be secret,
just as a way to avoid huge config files. So we could just
ignore the secrets and directly have 'id_auth_file' as a
parameter and directly reference a file.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
                   ` (5 preceding siblings ...)
  2021-07-09 21:55 ` [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch Brijesh Singh
@ 2021-07-12 17:00 ` Tom Lendacky
  2021-07-13  8:05 ` Dov Murik
  7 siblings, 0 replies; 45+ messages in thread
From: Tom Lendacky @ 2021-07-12 17:00 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Paolo Bonzini, Dov Murik, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost

On 7/9/21 4:55 PM, Brijesh Singh wrote:
> SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
> new hardware-based memory protections. SEV-SNP adds strong memory integrity
> protection to help prevent malicious hypervisor-based attacks like data
> replay, memory re-mapping and more in order to create an isolated memory
> encryption environment.
> 
> The patches to support the SEV-SNP in Linux kernel and OVMF are available:
> https://lore.kernel.org/kvm/20210707181506.30489-1-brijesh.singh@amd.com/
> https://lore.kernel.org/kvm/20210707183616.5620-1-brijesh.singh@amd.com/
> https://edk2.groups.io/g/devel/message/77335?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891508
> 
> The Qemu patches uses the command id added by the SEV-SNP hypervisor
> patches to bootstrap the SEV-SNP VMs.
> 
> TODO:
>  * Add support to filter CPUID values through the PSP.
> 
> Additional resources
> ---------------------
> SEV-SNP whitepaper
> https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
> 
> APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
> 
> GHCB spec:
> https://developer.amd.com/wp-content/resources/56421.pdf
> 
> SEV-SNP firmware specification:
> https://www.amd.com/system/files/TechDocs/56860.pdf
> 
> Brijesh Singh (6):
>   linux-header: add the SNP specific command
>   i386/sev: extend sev-guest property to include SEV-SNP
>   i386/sev: initialize SNP context
>   i386/sev: add the SNP launch start context
>   i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
>   i386/sev: populate secrets and cpuid page and finalize the SNP launch
> 
>  docs/amd-memory-encryption.txt |  81 +++++-
>  linux-headers/linux/kvm.h      |  47 ++++
>  qapi/qom.json                  |   6 +
>  target/i386/sev.c              | 498 ++++++++++++++++++++++++++++++++-
>  target/i386/sev_i386.h         |   1 +
>  target/i386/trace-events       |   4 +
>  6 files changed, 628 insertions(+), 9 deletions(-)

Don't forget to update target/i386/sev-stub.c as appropriate, too.

Thanks,
Tom

> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
                   ` (6 preceding siblings ...)
  2021-07-12 17:00 ` [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Tom Lendacky
@ 2021-07-13  8:05 ` Dov Murik
  2021-07-13  8:31   ` Dr. David Alan Gilbert
  2021-07-13 14:01   ` Brijesh Singh
  7 siblings, 2 replies; 45+ messages in thread
From: Dov Murik @ 2021-07-13  8:05 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
> new hardware-based memory protections. SEV-SNP adds strong memory integrity
> protection to help prevent malicious hypervisor-based attacks like data
> replay, memory re-mapping and more in order to create an isolated memory
> encryption environment.
> 
> The patches to support the SEV-SNP in Linux kernel and OVMF are available:
> https://lore.kernel.org/kvm/20210707181506.30489-1-brijesh.singh@amd.com/
> https://lore.kernel.org/kvm/20210707183616.5620-1-brijesh.singh@amd.com/
> https://edk2.groups.io/g/devel/message/77335?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891508
> 
> The Qemu patches uses the command id added by the SEV-SNP hypervisor
> patches to bootstrap the SEV-SNP VMs.
> 
> TODO:
>  * Add support to filter CPUID values through the PSP.
> 
> Additional resources
> ---------------------
> SEV-SNP whitepaper
> https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
> 
> APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
> 
> GHCB spec:
> https://developer.amd.com/wp-content/resources/56421.pdf
> 
> SEV-SNP firmware specification:
> https://www.amd.com/system/files/TechDocs/56860.pdf
> 
> Brijesh Singh (6):
>   linux-header: add the SNP specific command
>   i386/sev: extend sev-guest property to include SEV-SNP
>   i386/sev: initialize SNP context
>   i386/sev: add the SNP launch start context
>   i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
>   i386/sev: populate secrets and cpuid page and finalize the SNP launch
> 
>  docs/amd-memory-encryption.txt |  81 +++++-
>  linux-headers/linux/kvm.h      |  47 ++++
>  qapi/qom.json                  |   6 +
>  target/i386/sev.c              | 498 ++++++++++++++++++++++++++++++++-
>  target/i386/sev_i386.h         |   1 +
>  target/i386/trace-events       |   4 +
>  6 files changed, 628 insertions(+), 9 deletions(-)
> 

It might be useful to allow the user to view SNP-related status/settings
in HMP's `info sev` and QMP's qom-list/qom-get under
/machine/confidential-guest-support .

(Not sure whether HMP is deprecated and new stuff should not be added
there.)

Particularly confusing is the `policy` attribute which is only relevant
for SEV / SEV-ES, while there's a new `snp.policy` attribute for SNP...
Maybe the irrelevant attributes should not be added to the tree when not
in SNP.

-Dov


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-13  8:05 ` Dov Murik
@ 2021-07-13  8:31   ` Dr. David Alan Gilbert
  2021-07-13 13:57     ` Brijesh Singh
  2021-07-13 14:01   ` Brijesh Singh
  1 sibling, 1 reply; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-13  8:31 UTC (permalink / raw)
  To: Dov Murik
  Cc: Tom Lendacky, Brijesh Singh, Eduardo Habkost, kvm,
	Michael S . Tsirkin, Connor Kuehl, Michael Roth, James Bottomley,
	qemu-devel, Daniel P. Berrangé,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

* Dov Murik (dovmurik@linux.ibm.com) wrote:
> Brijesh,
> 
> On 10/07/2021 0:55, Brijesh Singh wrote:
> > SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
> > new hardware-based memory protections. SEV-SNP adds strong memory integrity
> > protection to help prevent malicious hypervisor-based attacks like data
> > replay, memory re-mapping and more in order to create an isolated memory
> > encryption environment.
> > 
> > The patches to support the SEV-SNP in Linux kernel and OVMF are available:
> > https://lore.kernel.org/kvm/20210707181506.30489-1-brijesh.singh@amd.com/
> > https://lore.kernel.org/kvm/20210707183616.5620-1-brijesh.singh@amd.com/
> > https://edk2.groups.io/g/devel/message/77335?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891508
> > 
> > The Qemu patches uses the command id added by the SEV-SNP hypervisor
> > patches to bootstrap the SEV-SNP VMs.
> > 
> > TODO:
> >  * Add support to filter CPUID values through the PSP.
> > 
> > Additional resources
> > ---------------------
> > SEV-SNP whitepaper
> > https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
> > 
> > APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
> > 
> > GHCB spec:
> > https://developer.amd.com/wp-content/resources/56421.pdf
> > 
> > SEV-SNP firmware specification:
> > https://www.amd.com/system/files/TechDocs/56860.pdf
> > 
> > Brijesh Singh (6):
> >   linux-header: add the SNP specific command
> >   i386/sev: extend sev-guest property to include SEV-SNP
> >   i386/sev: initialize SNP context
> >   i386/sev: add the SNP launch start context
> >   i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
> >   i386/sev: populate secrets and cpuid page and finalize the SNP launch
> > 
> >  docs/amd-memory-encryption.txt |  81 +++++-
> >  linux-headers/linux/kvm.h      |  47 ++++
> >  qapi/qom.json                  |   6 +
> >  target/i386/sev.c              | 498 ++++++++++++++++++++++++++++++++-
> >  target/i386/sev_i386.h         |   1 +
> >  target/i386/trace-events       |   4 +
> >  6 files changed, 628 insertions(+), 9 deletions(-)
> > 
> 
> It might be useful to allow the user to view SNP-related status/settings
> in HMP's `info sev` and QMP's qom-list/qom-get under
> /machine/confidential-guest-support .
> 
> (Not sure whether HMP is deprecated and new stuff should not be added
> there.)

It's still fine to add stuff to HMP, although generally you should be
adding it to QMP as well (unles sit's purely for debug and may change).

Dave

> Particularly confusing is the `policy` attribute which is only relevant
> for SEV / SEV-ES, while there's a new `snp.policy` attribute for SNP...
> Maybe the irrelevant attributes should not be added to the tree when not
> in SNP.
> 
> -Dov
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
                     ` (2 preceding siblings ...)
  2021-07-12 14:43   ` Daniel P. Berrangé
@ 2021-07-13 13:46   ` Markus Armbruster
  2021-07-14 14:18     ` Brijesh Singh
  2021-07-20 19:42     ` Michael Roth
  2021-07-13 18:21   ` Eric Blake
  4 siblings, 2 replies; 45+ messages in thread
From: Markus Armbruster @ 2021-07-13 13:46 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, qemu-devel,
	Dr . David Alan Gilbert, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

Brijesh Singh <brijesh.singh@amd.com> writes:

> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> Passing all parameters through command line can be difficult. To simplify
> the launch parameter passing, introduce a .ini-like config file that can be
> used for passing the parameters to the launch flow.
>
> The contents of the config file will look like this:
>
> $ cat snp-launch.init
>
> # SNP launch parameters
> [SEV-SNP]
> init_flags = 0
> policy = 0x1000
> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
>
>
> Add 'snp' property that can be used to indicate that SEV guest launch
> should enable the SNP support.
>
> SEV-SNP guest launch examples:
>
> 1) launch without additional parameters
>
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on
>
> 2) launch with optional parameters
>   $(QEMU_CLI) \
>     -object sev-guest,id=sev0,snp=on,launch-config=<file>
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

I acknowledge doing complex configuration on the command line can be
awkward.  But if we added a separate configuration file for every
configurable thing where that's the case, we'd have too many already,
and we'd constantly grow more.  I don't think this is a viable solution.

In my opinion, much of what we do on the command line should be done in
configuration files instead.  Not in several different configuration
languages, mind, but using one common language for all our configuration
needs.

Some of us argue this language already exists: QMP.  It can't do
everything the command line can do, but that's a matter of putting in
the work.  However, JSON isn't a good configuration language[1].  To get
a decent one, we'd have to to extend JSON[2], or wrap another concrete
syntax around QMP's abstract syntax.

But this doesn't help you at all *now*.

I recommend to do exactly what we've done before for complex
configuration: define it in the QAPI schema, so we can use both dotted
keys and JSON on the command line, and can have QMP, too.  Examples:
-blockdev, -display, -compat.

Questions?


[1] https://www.lucidchart.com/techblog/2018/07/16/why-json-isnt-a-good-configuration-language/

[2] Thanks, but no thanks.  Let's make new and interesting mistakes
instead of repeating old and tired ones.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-12 16:24       ` Daniel P. Berrangé
@ 2021-07-13 13:54         ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-13 13:54 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson, kvm,
	Michael Roth, Eduardo Habkost



On 7/12/21 11:24 AM, Daniel P. Berrangé wrote:>>
>> policy: 8 bytes
>> flags: 8 bytes
>> id_block: 96 bytes
>> id_auth: 4096 bytes
>> host_data: 32 bytes
>> gosvw: 16 bytes
> 
> Only the id_auth parameter is really considered large here.
> 
> When you say "up to a page size", that implies that the size is
> actually variable.  Is that correct, and if so, what is a real
> world common size going to be ? Is the common size much smaller
> than this upper limit ? If so I'd just ignore the issue entirely.

Looking at the recent spec, it appears that id_auth is fixed to 4K.

> 
> If not, then, 4k on the command line is certainly ugly, but isn't
> technically impossible. It would be similarly ugly to have this
> value stuffed into a libvirt XML configuration for that matter.
> 
> One option is to supply only that one parameter via an external
> file, with the file being an opaque blob whose context is the
> parameter value thus avoiding inventing a custom file format
> parser.
> 
> When "id_auth" is described as "authentication data", are there
> any secrecy requirements around this ?
> 

Yes this sounds much better, we have been using the similar approach for 
the SEV in which we pass the PDH and session blob through the file.


> QEMU does have the '-object secret' framework for passing blobs
> of sensitive data to QEMU and can allow passing via a file:
> 
>    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fqemu-project.gitlab.io%2Fqemu%2Fsystem%2Fsecrets.html&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C891fdc1ab0d8483aecb808d945519054%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637617038899405482%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8AHUC3DeyauxT4Pd2ZkUJkDyu9XHtexybM0BgdRlego%3D&amp;reserved=0
> 
> Even if this doesn't actually need to be kept private, we
> could decide to simply (ab)use the 'secret' object anyway
> as a way to let it be passed in out of band via a file.
> 

The content of the field does not need to be protected. It's a public 
information, so I am not sure the secrets object fits here.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-13  8:31   ` Dr. David Alan Gilbert
@ 2021-07-13 13:57     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-13 13:57 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, Dov Murik
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Tom Lendacky,
	Paolo Bonzini, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/13/21 3:31 AM, Dr. David Alan Gilbert wrote:
> adding it to QMP as well (unles sit's purely for debug and may change).

We have query-sev QMP, I will extend to add a new 'snp: bool' field.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-13  8:05 ` Dov Murik
  2021-07-13  8:31   ` Dr. David Alan Gilbert
@ 2021-07-13 14:01   ` Brijesh Singh
  2021-07-14  9:52     ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-13 14:01 UTC (permalink / raw)
  To: Dov Murik, qemu-devel
  Cc: brijesh.singh, Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/13/21 3:05 AM, Dov Murik wrote:>
> Particularly confusing is the `policy` attribute which is only relevant
> for SEV / SEV-ES, while there's a new `snp.policy` attribute for SNP...
> Maybe the irrelevant attributes should not be added to the tree when not
> in SNP.

The policy fields are also applicable to the SNP. The main difference are:

- in SEV/SEV-ES the policy is 32-bit compare to 64-bit value in SEV-SNP. 
However, for SEV-SNP spec uses lower 32-bit value and higher bits are 
marked reserved.

- the bit field meaning are different

Based on this, we can introduce a new filed 'snp-policy'.

-Brijesh


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
                     ` (3 preceding siblings ...)
  2021-07-13 13:46   ` Markus Armbruster
@ 2021-07-13 18:21   ` Eric Blake
  4 siblings, 0 replies; 45+ messages in thread
From: Eric Blake @ 2021-07-13 18:21 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, qemu-devel,
	Dr . David Alan Gilbert, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote:
> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> Passing all parameters through command line can be difficult. To simplify
> the launch parameter passing, introduce a .ini-like config file that can be
> used for passing the parameters to the launch flow.

I agree with Markus' assessment that we are probably going to be
better off reusing what we already have for other complex options
rather than inventing yet another ini file.

Additional things I noted:

> +++ b/qapi/qom.json
> @@ -749,6 +749,10 @@
>  # @reduced-phys-bits: number of bits in physical addresses that become
>  #                     unavailable when SEV is enabled
>  #
> +# @snp: SEV-SNP is enabled (default: 0)

Here you list 0...

> +#
> +# @launch-config: launch config file to use

Both additions (if we keep the launch-config addition) are missing
'(since 6.1)' notations.

> +#
>  # Since: 2.12
>  ##
>  { 'struct': 'SevGuestProperties',
> @@ -758,6 +762,8 @@
>              '*policy': 'uint32',
>              '*handle': 'uint32',
>              '*cbitpos': 'uint32',
> +            '*snp': 'bool',

...but here you state snp is bool. That means the default is 'false', not '0'.

> +            '*launch-config': 'str',
>              'reduced-phys-bits': 'uint32' } }
>  

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-13 14:01   ` Brijesh Singh
@ 2021-07-14  9:52     ` Dr. David Alan Gilbert
  2021-07-14 14:23       ` Brijesh Singh
  0 siblings, 1 reply; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-14  9:52 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, qemu-devel, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> 
> 
> On 7/13/21 3:05 AM, Dov Murik wrote:>
> > Particularly confusing is the `policy` attribute which is only relevant
> > for SEV / SEV-ES, while there's a new `snp.policy` attribute for SNP...
> > Maybe the irrelevant attributes should not be added to the tree when not
> > in SNP.
> 
> The policy fields are also applicable to the SNP. The main difference are:
> 
> - in SEV/SEV-ES the policy is 32-bit compare to 64-bit value in SEV-SNP.
> However, for SEV-SNP spec uses lower 32-bit value and higher bits are marked
> reserved.
> 
> - the bit field meaning are different

Ah, I see that from the SNP ABI spec (section 4.3).

That's a bit subtle; in that at the moment we select SEV or SEV-ES based
on the existing guest policy flags; I think you're saying that SEV-SNP
is enabled by the user explicitly.

> Based on this, we can introduce a new filed 'snp-policy'.

Yes, people are bound to confuse them if they're not clearly separated;
although I guess whatever comes after SNP will probably share that
longer field?

Dave

> -Brijesh
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-13 13:46   ` Markus Armbruster
@ 2021-07-14 14:18     ` Brijesh Singh
  2021-07-20 19:42     ` Michael Roth
  1 sibling, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-14 14:18 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost


On 7/13/21 8:46 AM, Markus Armbruster wrote:
> Brijesh Singh <brijesh.singh@amd.com> writes:
>
>> To launch the SEV-SNP guest, a user can specify up to 8 parameters.
>> Passing all parameters through command line can be difficult. To simplify
>> the launch parameter passing, introduce a .ini-like config file that can be
>> used for passing the parameters to the launch flow.
>>
>> The contents of the config file will look like this:
>>
>> $ cat snp-launch.init
>>
>> # SNP launch parameters
>> [SEV-SNP]
>> init_flags = 0
>> policy = 0x1000
>> id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
>>
>>
>> Add 'snp' property that can be used to indicate that SEV guest launch
>> should enable the SNP support.
>>
>> SEV-SNP guest launch examples:
>>
>> 1) launch without additional parameters
>>
>>   $(QEMU_CLI) \
>>     -object sev-guest,id=sev0,snp=on
>>
>> 2) launch with optional parameters
>>   $(QEMU_CLI) \
>>     -object sev-guest,id=sev0,snp=on,launch-config=<file>
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> I acknowledge doing complex configuration on the command line can be
> awkward.  But if we added a separate configuration file for every
> configurable thing where that's the case, we'd have too many already,
> and we'd constantly grow more.  I don't think this is a viable solution.
>
> In my opinion, much of what we do on the command line should be done in
> configuration files instead.  Not in several different configuration
> languages, mind, but using one common language for all our configuration
> needs.
>
> Some of us argue this language already exists: QMP.  It can't do
> everything the command line can do, but that's a matter of putting in
> the work.  However, JSON isn't a good configuration language[1].  To get
> a decent one, we'd have to to extend JSON[2], or wrap another concrete
> syntax around QMP's abstract syntax.
>
> But this doesn't help you at all *now*.
>
> I recommend to do exactly what we've done before for complex
> configuration: define it in the QAPI schema, so we can use both dotted
> keys and JSON on the command line, and can have QMP, too.  Examples:
> -blockdev, -display, -compat.
>
> Questions?


I will take a look at the blockdev and try modeling after that. if I run
into any questions then I will ask. thanks for the pointer Markus.

-Brijesh


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  2021-07-14  9:52     ` Dr. David Alan Gilbert
@ 2021-07-14 14:23       ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-14 14:23 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: brijesh.singh, Dov Murik, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Tom Lendacky,
	Paolo Bonzini, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost


On 7/14/21 4:52 AM, Dr. David Alan Gilbert wrote:
> * Brijesh Singh (brijesh.singh@amd.com) wrote:
>>
>> On 7/13/21 3:05 AM, Dov Murik wrote:>
>>> Particularly confusing is the `policy` attribute which is only relevant
>>> for SEV / SEV-ES, while there's a new `snp.policy` attribute for SNP...
>>> Maybe the irrelevant attributes should not be added to the tree when not
>>> in SNP.
>> The policy fields are also applicable to the SNP. The main difference are:
>>
>> - in SEV/SEV-ES the policy is 32-bit compare to 64-bit value in SEV-SNP.
>> However, for SEV-SNP spec uses lower 32-bit value and higher bits are marked
>> reserved.
>>
>> - the bit field meaning are different
> Ah, I see that from the SNP ABI spec (section 4.3).
>
> That's a bit subtle; in that at the moment we select SEV or SEV-ES based
> on the existing guest policy flags; I think you're saying that SEV-SNP
> is enabled by the user explicitly.

Correct. This is one of the reason that I added the "snp" property.


>
>> Based on this, we can introduce a new filed 'snp-policy'.
> Yes, people are bound to confuse them if they're not clearly separated;
> although I guess whatever comes after SNP will probably share that
> longer field?


I am keeping my finger crossed on it. I hope that in future they will
share it.

-Brijesh



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  2021-07-09 21:55 ` [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled Brijesh Singh
@ 2021-07-14 17:08   ` Connor Kuehl
  2021-07-14 18:52     ` Brijesh Singh
  2021-07-19 13:00   ` Dov Murik
  1 sibling, 1 reply; 45+ messages in thread
From: Connor Kuehl @ 2021-07-14 17:08 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Michael Roth,
	James Bottomley, Dr . David Alan Gilbert, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On 7/9/21 3:55 PM, Brijesh Singh wrote:
> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
> image used for booting the SEV-SNP guest.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 259408a8f1..41dcb084d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -883,6 +883,30 @@ out:
>      return ret;
>  }
>  
> +static int
> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type)
> +{
> +    int ret, fw_error;
> +    struct kvm_sev_snp_launch_update update = {};
> +
> +    if (!addr || !len) {
> +        return 1;

Should this be a -1? It looks like the caller checks if this function
returns < 0, but doesn't check for res == 1.

Alternatively, invoking error_report might provide more useful
information that the preconditions to this function were violated.

Connor



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch
  2021-07-09 21:55 ` [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch Brijesh Singh
@ 2021-07-14 17:29   ` Dr. David Alan Gilbert
  2021-07-14 18:53     ` Brijesh Singh
  2021-07-19 11:24   ` Dov Murik
  1 sibling, 1 reply; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-14 17:29 UTC (permalink / raw)
  To: Brijesh Singh
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, qemu-devel, Dov Murik,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> During the SNP guest launch sequence, a special secrets and cpuid page
> needs to be populated by the SEV-SNP firmware. The secrets page contains
> the VM Platform Communication Key (VMPCKs) used by the guest to send and
> receive secure messages to the PSP. And CPUID page will contain the CPUID
> value filtered through the PSP.
> 
> The guest BIOS (OVMF) reserves these pages in MEMFD and location of it
> is available through the SNP boot block GUID. While finalizing the guest
> boot flow, lookup for the boot block and call the SNP_LAUNCH_UPDATE
> command to populate secrets and cpuid pages.
> 
> In order to support early boot code, the OVMF may ask hypervisor to
> request the pre-validation of certain memory range. If such range is
> present the call LAUNCH_UPDATE command to validate those address range
> without affecting the measurement. See the SEV-SNP specification for
> further details.
> 
> Finally, call the SNP_LAUNCH_FINISH to finalize the guest boot.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 184 ++++++++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |   2 +
>  2 files changed, 184 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 41dcb084d1..f438e09d33 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -93,6 +93,19 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_SNP_BOOT_BLOCK_GUID "bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9"
> +typedef struct __attribute__((__packed__)) SevSnpBootInfoBlock {
> +    /* Prevalidate range address */
> +    uint32_t pre_validated_start;
> +    uint32_t pre_validated_end;
> +    /* Secrets page address */
> +    uint32_t secrets_addr;
> +    uint32_t secrets_len;
> +    /* CPUID page address */
> +    uint32_t cpuid_addr;
> +    uint32_t cpuid_len;
> +} SevSnpBootInfoBlock;
> +
>  static SevGuestState *sev_guest;
>  static Error *sev_mig_blocker;
>  
> @@ -1014,6 +1027,158 @@ static Notifier sev_machine_done_notify = {
>      .notify = sev_launch_get_measure,
>  };
>  
> +static int
> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
> +{
> +    void *hva;
> +    MemoryRegion *mr = NULL;
> +
> +    hva = gpa2hva(&mr, hwaddr, size, NULL);
> +    if (!hva) {
> +        error_report("SEV-SNP failed to get HVA for GPA 0x%x", hwaddr);
> +        return 1;
> +    }
> +
> +    return sev_snp_launch_update(sev_guest, hva, size, type);
> +}
> +
> +struct snp_pre_validated_range {
> +    uint32_t start;
> +    uint32_t end;
> +};

Just a thought, but maybe use a 'Range' from include/qemu/range.h ?

Dave

> +static struct snp_pre_validated_range pre_validated[2];
> +
> +static bool
> +detectoverlap(uint32_t start, uint32_t end,
> +              struct snp_pre_validated_range *overlap)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
> +        if (pre_validated[i].start < end && start < pre_validated[i].end) {
> +            memcpy(overlap, &pre_validated[i], sizeof(*overlap));
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static void snp_ovmf_boot_block_setup(void)
> +{
> +    struct snp_pre_validated_range overlap;
> +    SevSnpBootInfoBlock *info;
> +    uint32_t start, end, sz;
> +    int ret;
> +
> +    /*
> +     * Extract the SNP boot block for the SEV-SNP guests by locating the
> +     * SNP_BOOT GUID. The boot block contains the information such as location
> +     * of secrets and CPUID page, additionaly it may contain the range of
> +     * memory that need to be pre-validated for the boot.
> +     */
> +    if (!pc_system_ovmf_table_find(SEV_SNP_BOOT_BLOCK_GUID,
> +        (uint8_t **)&info, NULL)) {
> +        error_report("SEV-SNP: failed to find the SNP boot block");
> +        exit(1);
> +    }
> +
> +    trace_kvm_sev_snp_ovmf_boot_block_info(info->secrets_addr,
> +                                           info->secrets_len, info->cpuid_addr,
> +                                           info->cpuid_len,
> +                                           info->pre_validated_start,
> +                                           info->pre_validated_end);
> +
> +    /* Populate the secrets page */
> +    ret = sev_snp_launch_update_gpa(info->secrets_addr, info->secrets_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_SECRETS);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert secret page GPA 0x%x",
> +                     info->secrets_addr);
> +        exit(1);
> +    }
> +
> +    /* Populate the cpuid page */
> +    ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_CPUID);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x",
> +                     info->cpuid_addr);
> +        exit(1);
> +    }
> +
> +    /*
> +     * Pre-validate the range using the LAUNCH_UPDATE_DATA, if the
> +     * pre-validation range contains the CPUID and Secret page GPA then skip
> +     * it. This is because SEV-SNP firmware pre-validates those pages as part
> +     * of adding secrets and cpuid LAUNCH_UPDATE type.
> +     */
> +    pre_validated[0].start = info->secrets_addr;
> +    pre_validated[0].end = info->secrets_addr + info->secrets_len;
> +    pre_validated[1].start = info->cpuid_addr;
> +    pre_validated[1].end = info->cpuid_addr + info->cpuid_len;
> +    start = info->pre_validated_start;
> +    end = info->pre_validated_end;
> +
> +    while (start < end) {
> +        /* Check if the requested range overlaps with Secrets and CPUID page */
> +        if (detectoverlap(start, end, &overlap)) {
> +            if (start < overlap.start) {
> +                sz = overlap.start - start;
> +                if (sev_snp_launch_update_gpa(start, sz,
> +                    KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> +                    error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> +                                 start, sz);
> +                    exit(1);
> +                }
> +            }
> +
> +            start = overlap.end;
> +            continue;
> +        }
> +
> +        /* Validate the remaining range */
> +        if (sev_snp_launch_update_gpa(start, end - start,
> +            KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> +            error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> +                         start, end - start);
> +            exit(1);
> +        }
> +
> +        start = end;
> +    }
> +}
> +
> +static void
> +sev_snp_launch_finish(SevGuestState *sev)
> +{
> +    int ret, error;
> +    Error *local_err = NULL;
> +    struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
> +
> +    trace_kvm_sev_snp_launch_finish();
> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, finish, &error);
> +    if (ret) {
> +        error_report("%s: SNP_LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> +                     __func__, ret, error, fw_error_to_str(error));
> +        exit(1);
> +    }
> +
> +    sev_set_guest_state(sev, SEV_STATE_RUNNING);
> +
> +    /* add migration blocker */
> +    error_setg(&sev_mig_blocker,
> +               "SEV: Migration is not implemented");
> +    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        error_free(sev_mig_blocker);
> +        exit(1);
> +    }
> +}
> +
> +
>  static void
>  sev_launch_finish(SevGuestState *sev)
>  {
> @@ -1048,7 +1213,12 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>  
>      if (running) {
>          if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
> -            sev_launch_finish(sev);
> +            if (sev_snp_enabled()) {
> +                snp_ovmf_boot_block_setup();
> +                sev_snp_launch_finish(sev);
> +            } else {
> +                sev_launch_finish(sev);
> +            }
>          }
>      }
>  }
> @@ -1164,7 +1334,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      }
>  
>      ram_block_notifier_add(&sev_ram_notifier);
> -    qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +
> +    /*
> +     * The machine done notify event is used by the SEV guest to get the
> +     * measurement of the encrypted images. When SEV-SNP is enabled then
> +     * measurement is part of the attestation report and the measurement
> +     * command does not exist. So skip registering the notifier.
> +     */
> +    if (!sev_snp_enabled()) {
> +        qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +    }
> +
>      qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
>  
>      cgs->ready = true;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 0c2d250206..db91287439 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -13,3 +13,5 @@ kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
>  kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
>  kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
> +kvm_sev_snp_launch_finish(void) ""
> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  2021-07-14 17:08   ` Connor Kuehl
@ 2021-07-14 18:52     ` Brijesh Singh
  2021-07-15  5:54       ` Dov Murik
  0 siblings, 1 reply; 45+ messages in thread
From: Brijesh Singh @ 2021-07-14 18:52 UTC (permalink / raw)
  To: Connor Kuehl, qemu-devel
  Cc: brijesh.singh, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/14/21 12:08 PM, Connor Kuehl wrote:
> On 7/9/21 3:55 PM, Brijesh Singh wrote:
>> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
>> image used for booting the SEV-SNP guest.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
>>   target/i386/trace-events |  1 +
>>   2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 259408a8f1..41dcb084d1 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -883,6 +883,30 @@ out:
>>       return ret;
>>   }
>>   
>> +static int
>> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type)
>> +{
>> +    int ret, fw_error;
>> +    struct kvm_sev_snp_launch_update update = {};
>> +
>> +    if (!addr || !len) {
>> +        return 1;
> 
> Should this be a -1? It looks like the caller checks if this function
> returns < 0, but doesn't check for res == 1.

Ah, it should be -1.

> 
> Alternatively, invoking error_report might provide more useful
> information that the preconditions to this function were violated.
> 

Sure, I will add error_report.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch
  2021-07-14 17:29   ` Dr. David Alan Gilbert
@ 2021-07-14 18:53     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-14 18:53 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: brijesh.singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Tom Lendacky,
	Paolo Bonzini, Dov Murik, David Gibson, Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/14/21 12:29 PM, Dr. David Alan Gilbert wrote:>> +struct 
snp_pre_validated_range {
>> +    uint32_t start;
>> +    uint32_t end;
>> +};
> 
> Just a thought, but maybe use a 'Range' from include/qemu/range.h ?
> 

I will look into it.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  2021-07-14 18:52     ` Brijesh Singh
@ 2021-07-15  5:54       ` Dov Murik
  0 siblings, 0 replies; 45+ messages in thread
From: Dov Murik @ 2021-07-15  5:54 UTC (permalink / raw)
  To: Brijesh Singh, Connor Kuehl, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Michael Roth,
	James Bottomley, Dr . David Alan Gilbert, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson



On 14/07/2021 21:52, Brijesh Singh wrote:
> 
> 
> On 7/14/21 12:08 PM, Connor Kuehl wrote:
>> On 7/9/21 3:55 PM, Brijesh Singh wrote:
>>> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
>>> image used for booting the SEV-SNP guest.
>>>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>>   target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
>>>   target/i386/trace-events |  1 +
>>>   2 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>>> index 259408a8f1..41dcb084d1 100644
>>> --- a/target/i386/sev.c
>>> +++ b/target/i386/sev.c
>>> @@ -883,6 +883,30 @@ out:
>>>       return ret;
>>>   }
>>>   +static int
>>> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t
>>> len, int type)
>>> +{
>>> +    int ret, fw_error;
>>> +    struct kvm_sev_snp_launch_update update = {};
>>> +
>>> +    if (!addr || !len) {
>>> +        return 1;
>>
>> Should this be a -1? It looks like the caller checks if this function
>> returns < 0, but doesn't check for res == 1.
> 
> Ah, it should be -1.
> 
>>
>> Alternatively, invoking error_report might provide more useful
>> information that the preconditions to this function were violated.
>>
> 
> Sure, I will add error_report.

Maybe even simpler:

  assert(addr);
  assert(len > 0);

The assertion failure will show the developer what is wrong. This should
not happen for the end-user (unless I'm missing something).

-Dov


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 3/6] i386/sev: initialize SNP context
  2021-07-09 21:55 ` [RFC PATCH 3/6] i386/sev: initialize SNP context Brijesh Singh
@ 2021-07-15  9:32   ` Dov Murik
  2021-07-15 13:24     ` Brijesh Singh
  0 siblings, 1 reply; 45+ messages in thread
From: Dov Murik @ 2021-07-15  9:32 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

Hi Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocate a new ASID from the SNP pool and calls the

s/allocate/allocates/

> firmware to initialize the all the resources.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c      | 24 +++++++++++++++++++++---
>  target/i386/sev_i386.h |  1 +
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6b238ef969..84ae244af0 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -583,10 +583,17 @@ sev_enabled(void)
>      return !!sev_guest;
>  }
>  
> +bool
> +sev_snp_enabled(void)
> +{
> +    return sev_guest->snp;
> +}
> +
>  bool
>  sev_es_enabled(void)
>  {
> -    return sev_enabled() && (sev_guest->policy & SEV_POLICY_ES);
> +    return sev_snp_enabled() ||
> +           (sev_enabled() && (sev_guest->policy & SEV_POLICY_ES));
>  }
>  

Just making sure I understand:

* sev_enabled() returns true for SEV or newer (SEV or SEV-ES or
  SEV-SNP).
* sev_es_enabled() returns true for SEV-ES or newer (SEV-ES or SEV-SNP).
* sev_snp_enabled() returns true for SEV-SNP or newer (currently only
  SEV-SNP).

Is that indeed the intention?


-Dov


>  uint64_t
> @@ -1008,6 +1015,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      uint32_t ebx;
>      uint32_t host_cbitpos;
>      struct sev_user_data_status status = {};
> +    void *init_args = NULL;
>  
>      if (!sev) {
>          return 0;
> @@ -1061,7 +1069,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      sev->api_major = status.api_major;
>      sev->api_minor = status.api_minor;
>  
> -    if (sev_es_enabled()) {
> +    if (sev_snp_enabled()) {
> +        if (!kvm_kernel_irqchip_allowed()) {
> +            error_report("%s: SEV-SNP guests require in-kernel irqchip support",
> +                         __func__);
> +            goto err;
> +        }
> +
> +        cmd = KVM_SEV_SNP_INIT;
> +        init_args = (void *)&sev->snp_config.init;
> +
> +    } else if (sev_es_enabled()) {
>          if (!kvm_kernel_irqchip_allowed()) {
>              error_report("%s: SEV-ES guests require in-kernel irqchip support",
>                           __func__);
> @@ -1080,7 +1098,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      }
>  
>      trace_kvm_sev_init();
> -    ret = sev_ioctl(sev->sev_fd, cmd, NULL, &fw_error);
> +    ret = sev_ioctl(sev->sev_fd, cmd, init_args, &fw_error);
>      if (ret) {
>          error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
>                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..e0e1a599be 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -29,6 +29,7 @@
>  #define SEV_POLICY_SEV          0x20
>  
>  extern bool sev_es_enabled(void);
> +extern bool sev_snp_enabled(void);
>  extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
>  extern uint32_t sev_get_cbit_position(void);
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 3/6] i386/sev: initialize SNP context
  2021-07-15  9:32   ` Dov Murik
@ 2021-07-15 13:24     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-15 13:24 UTC (permalink / raw)
  To: Dov Murik, qemu-devel
  Cc: brijesh.singh, Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/15/21 4:32 AM, Dov Murik wrote:
> 
> Just making sure I understand:
> 
> * sev_enabled() returns true for SEV or newer (SEV or SEV-ES or
>    SEV-SNP).
> * sev_es_enabled() returns true for SEV-ES or newer (SEV-ES or SEV-SNP).
> * sev_snp_enabled() returns true for SEV-SNP or newer (currently only
>    SEV-SNP).
> 
> Is that indeed the intention?
> 

Yes. The SEV-SNP support requires the SEV and SEV-ES to be enabled. See 
the text from the APM vol2 section 15.36.

	The SEV-SNP features enable additional protection for encrypted
	VMs designed to achieve stronger isolation from the hypervisor.
	SEV-SNP is used with the SEV and SEV-ES features described in
	Section 15.34 and Section 15.35 respectively and requires the
	enablement and use of these features.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch
  2021-07-09 21:55 ` [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch Brijesh Singh
  2021-07-14 17:29   ` Dr. David Alan Gilbert
@ 2021-07-19 11:24   ` Dov Murik
  2021-07-19 14:45     ` Brijesh Singh
  1 sibling, 1 reply; 45+ messages in thread
From: Dov Murik @ 2021-07-19 11:24 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

Hi Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> During the SNP guest launch sequence, a special secrets and cpuid page
> needs to be populated by the SEV-SNP firmware. The secrets page contains
> the VM Platform Communication Key (VMPCKs) used by the guest to send and
> receive secure messages to the PSP. And CPUID page will contain the CPUID
> value filtered through the PSP.
> 
> The guest BIOS (OVMF) reserves these pages in MEMFD and location of it
> is available through the SNP boot block GUID. While finalizing the guest
> boot flow, lookup for the boot block and call the SNP_LAUNCH_UPDATE
> command to populate secrets and cpuid pages.
> 
> In order to support early boot code, the OVMF may ask hypervisor to
> request the pre-validation of certain memory range. If such range is
> present the call LAUNCH_UPDATE command to validate those address range

s/LAUNCH_UPDATE/SNP_LAUNCH_UPDATE/
(to show it's the same command you refer to above)

> without affecting the measurement. See the SEV-SNP specification for
> further details.
> 
> Finally, call the SNP_LAUNCH_FINISH to finalize the guest boot.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 184 ++++++++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |   2 +
>  2 files changed, 184 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 41dcb084d1..f438e09d33 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -93,6 +93,19 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_SNP_BOOT_BLOCK_GUID "bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9"
> +typedef struct __attribute__((__packed__)) SevSnpBootInfoBlock {
> +    /* Prevalidate range address */
> +    uint32_t pre_validated_start;
> +    uint32_t pre_validated_end;
> +    /* Secrets page address */
> +    uint32_t secrets_addr;
> +    uint32_t secrets_len;
> +    /* CPUID page address */
> +    uint32_t cpuid_addr;
> +    uint32_t cpuid_len;
> +} SevSnpBootInfoBlock;
> +
>  static SevGuestState *sev_guest;
>  static Error *sev_mig_blocker;
>  
> @@ -1014,6 +1027,158 @@ static Notifier sev_machine_done_notify = {
>      .notify = sev_launch_get_measure,
>  };
>  
> +static int
> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)

hwaddr is a confusing name here because it is also a typedef (which is
most likely uint64_t...).  Maybe call this argument `gpa` ?


> +{
> +    void *hva;
> +    MemoryRegion *mr = NULL;
> +
> +    hva = gpa2hva(&mr, hwaddr, size, NULL);
> +    if (!hva) {
> +        error_report("SEV-SNP failed to get HVA for GPA 0x%x", hwaddr);
> +        return 1;
> +    }
> +
> +    return sev_snp_launch_update(sev_guest, hva, size, type);
> +}
> +
> +struct snp_pre_validated_range {
> +    uint32_t start;
> +    uint32_t end;
> +};
> +
> +static struct snp_pre_validated_range pre_validated[2];
> +
> +static bool
> +detectoverlap(uint32_t start, uint32_t end,
> +              struct snp_pre_validated_range *overlap)

naming conventions dictate: detect_overlap

> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
> +        if (pre_validated[i].start < end && start < pre_validated[i].end) {
> +            memcpy(overlap, &pre_validated[i], sizeof(*overlap));

Maybe simpler than memcpy:

    *overlap = pre_validated[i];


> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static void snp_ovmf_boot_block_setup(void)
> +{
> +    struct snp_pre_validated_range overlap;
> +    SevSnpBootInfoBlock *info;
> +    uint32_t start, end, sz;
> +    int ret;
> +
> +    /*
> +     * Extract the SNP boot block for the SEV-SNP guests by locating the
> +     * SNP_BOOT GUID. The boot block contains the information such as location
> +     * of secrets and CPUID page, additionaly it may contain the range of
> +     * memory that need to be pre-validated for the boot.
> +     */
> +    if (!pc_system_ovmf_table_find(SEV_SNP_BOOT_BLOCK_GUID,
> +        (uint8_t **)&info, NULL)) {
> +        error_report("SEV-SNP: failed to find the SNP boot block");
> +        exit(1);
> +    }
> +
> +    trace_kvm_sev_snp_ovmf_boot_block_info(info->secrets_addr,
> +                                           info->secrets_len, info->cpuid_addr,
> +                                           info->cpuid_len,
> +                                           info->pre_validated_start,
> +                                           info->pre_validated_end);
> +
> +    /* Populate the secrets page */
> +    ret = sev_snp_launch_update_gpa(info->secrets_addr, info->secrets_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_SECRETS);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert secret page GPA 0x%x",
> +                     info->secrets_addr);
> +        exit(1);
> +    }
> +
> +    /* Populate the cpuid page */
> +    ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_CPUID);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x",
> +                     info->cpuid_addr);
> +        exit(1);
> +    }
> +
> +    /*
> +     * Pre-validate the range using the LAUNCH_UPDATE_DATA, if the
> +     * pre-validation range contains the CPUID and Secret page GPA then skip
> +     * it. This is because SEV-SNP firmware pre-validates those pages as part
> +     * of adding secrets and cpuid LAUNCH_UPDATE type.
> +     */
> +    pre_validated[0].start = info->secrets_addr;
> +    pre_validated[0].end = info->secrets_addr + info->secrets_len;
> +    pre_validated[1].start = info->cpuid_addr;
> +    pre_validated[1].end = info->cpuid_addr + info->cpuid_len;
> +    start = info->pre_validated_start;
> +    end = info->pre_validated_end;
> +
> +    while (start < end) {
> +        /* Check if the requested range overlaps with Secrets and CPUID page */
> +        if (detectoverlap(start, end, &overlap)) {
> +            if (start < overlap.start) {
> +                sz = overlap.start - start;
> +                if (sev_snp_launch_update_gpa(start, sz,
> +                    KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> +                    error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> +                                 start, sz);
> +                    exit(1);
> +                }
> +            }
> +
> +            start = overlap.end;
> +            continue;
> +        }
> +
> +        /* Validate the remaining range */
> +        if (sev_snp_launch_update_gpa(start, end - start,
> +            KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> +            error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> +                         start, end - start);
> +            exit(1);
> +        }
> +
> +        start = end;
> +    }
> +}
> +
> +static void
> +sev_snp_launch_finish(SevGuestState *sev)
> +{
> +    int ret, error;
> +    Error *local_err = NULL;
> +    struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
> +
> +    trace_kvm_sev_snp_launch_finish();

Maybe the trace should show some info about the snp_config.finish fields?


> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, finish, &error);
> +    if (ret) {
> +        error_report("%s: SNP_LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> +                     __func__, ret, error, fw_error_to_str(error));
> +        exit(1);
> +    }
> +
> +    sev_set_guest_state(sev, SEV_STATE_RUNNING);
> +
> +    /* add migration blocker */
> +    error_setg(&sev_mig_blocker,
> +               "SEV: Migration is not implemented");
> +    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        error_free(sev_mig_blocker);
> +        exit(1);
> +    }
> +}
> +
> +
>  static void
>  sev_launch_finish(SevGuestState *sev)
>  {
> @@ -1048,7 +1213,12 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>  
>      if (running) {
>          if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
> -            sev_launch_finish(sev);
> +            if (sev_snp_enabled()) {
> +                snp_ovmf_boot_block_setup();
> +                sev_snp_launch_finish(sev);
> +            } else {
> +                sev_launch_finish(sev);
> +            }
>          }
>      }
>  }
> @@ -1164,7 +1334,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      }
>  
>      ram_block_notifier_add(&sev_ram_notifier);
> -    qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +
> +    /*
> +     * The machine done notify event is used by the SEV guest to get the
> +     * measurement of the encrypted images. When SEV-SNP is enabled then
> +     * measurement is part of the attestation report and the measurement
> +     * command does not exist. So skip registering the notifier.
> +     */
> +    if (!sev_snp_enabled()) {
> +        qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +    }
> +
>      qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
>  
>      cgs->ready = true;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 0c2d250206..db91287439 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -13,3 +13,5 @@ kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
>  kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
>  kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
> +kvm_sev_snp_launch_finish(void) ""
> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"

The last argument is an end-addr (not a length), so maybe the format
string should end with:

   ".... pre-validate 0x%x - 0x%x"

Also I'd prefer to log the SevSnpBootInfoBlock fields in the order they
appear in the struct.


-Dov


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 1/6] linux-header: add the SNP specific command
  2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
  2021-07-10 20:32   ` Michael S. Tsirkin
@ 2021-07-19 11:35   ` Dov Murik
  2021-07-19 14:40     ` Brijesh Singh
  1 sibling, 1 reply; 45+ messages in thread
From: Dov Murik @ 2021-07-19 11:35 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

Hi Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> Sync the kvm.h with the kernel to include the SNP specific commands.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  linux-headers/linux/kvm.h | 47 +++++++++++++++++++++++++++++++++++++++


What about psp-sev.h ? I see that kernel patch "[PATCH Part2 RFC v4
11/40] crypto:ccp: Define the SEV-SNP commands" adds some new PSP return
codes.

The QEMU user-friendly string list sev_fw_errlist (in sev.c) should be
updated accordingly.

-Dov



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 4/6] i386/sev: add the SNP launch start context
  2021-07-09 21:55 ` [RFC PATCH 4/6] i386/sev: add the SNP launch start context Brijesh Singh
@ 2021-07-19 12:34   ` Dov Murik
  2021-07-19 15:27     ` Brijesh Singh
  0 siblings, 1 reply; 45+ messages in thread
From: Dov Murik @ 2021-07-19 12:34 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

Hi Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> The SNP_LAUNCH_START is called first to create a cryptographic launch
> context within the firmware.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 30 +++++++++++++++++++++++++++++-
>  target/i386/trace-events |  1 +
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 84ae244af0..259408a8f1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
>      return 0;
>  }
>  
> +static int
> +sev_snp_launch_start(SevGuestState *sev)
> +{
> +    int ret = 1;
> +    int fw_error, rc;
> +    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
> +
> +    trace_kvm_sev_snp_launch_start(start->policy);
> +
> +    rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error);
> +    if (rc < 0) {
> +        error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));

Did you mean to report the value of ret or rc?


> +        goto out;

Suggestion:

Remove the `ret` variable.
Here: simply `return 1`.
At the end: remove the `out:` label; simply `return 0`.


> +    }
> +
> +    sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
> +    ret = 0;
> +
> +out:
> +    return ret;
> +}
> +
>  static int
>  sev_launch_start(SevGuestState *sev)
>  {
> @@ -1105,7 +1128,12 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>          goto err;
>      }
>  
> -    ret = sev_launch_start(sev);
> +    if (sev_snp_enabled()) {
> +        ret = sev_snp_launch_start(sev);
> +    } else {
> +        ret = sev_launch_start(sev);
> +    }
> +
>      if (ret) {
>          error_setg(errp, "%s: failed to create encryption context", __func__);
>          goto err;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 2cd8726eeb..18cc14b956 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -11,3 +11,4 @@ kvm_sev_launch_measurement(const char *value) "data %s"
>  kvm_sev_launch_finish(void) ""
>  kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
> +kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
  2021-07-09 21:55 ` [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled Brijesh Singh
  2021-07-14 17:08   ` Connor Kuehl
@ 2021-07-19 13:00   ` Dov Murik
  1 sibling, 0 replies; 45+ messages in thread
From: Dov Murik @ 2021-07-19 13:00 UTC (permalink / raw)
  To: Brijesh Singh, qemu-devel
  Cc: Tom Lendacky, Daniel P. Berrangé,
	Eduardo Habkost, kvm, Michael S . Tsirkin, Connor Kuehl,
	Michael Roth, James Bottomley, Dr . David Alan Gilbert,
	Dov Murik, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 10/07/2021 0:55, Brijesh Singh wrote:
> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
> image used for booting the SEV-SNP guest.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 259408a8f1..41dcb084d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -883,6 +883,30 @@ out:
>      return ret;
>  }
>  
> +static int
> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type)

This seems similar to the SEV LAUNCH_UPDATE_DATA API (with the added
`type` argument).  In SEV API these are the limitations (from the SEV
API document):

* PADDR - System physical address of the data to be encrypted.
          Must be 16 B aligned.
* LENGTH - Length of the data to be encrypted.
           Must be a multiple of 16 B.

But in SNP_LAUNCH_UPDATE it is my understanding that addr must be page
aligned (4KB) and length must be in whole pages (because the underlying
types are PAGE_TYPE_NORMAL, PAGE_TYPE_ZERO, ...).

So what happens if we call sev_encrypt_flash with a non-page-aligned
addr / length?

Or maybe I misunderstood the SNP ABI document?

-Dov



> +{
> +    int ret, fw_error;
> +    struct kvm_sev_snp_launch_update update = {};
> +
> +    if (!addr || !len) {
> +        return 1;
> +    }
> +
> +    update.uaddr = (__u64)(unsigned long)addr;
> +    update.len = len;
> +    update.page_type = type;
> +    trace_kvm_sev_snp_launch_update(addr, len, type);
> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE,
> +                    &update, &fw_error);
> +    if (ret) {
> +        error_report("%s: SNP_LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));
> +    }
> +
> +    return ret;
> +}
> +
>  static int
>  sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
>  {
> @@ -1161,7 +1185,14 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
>  
>      /* if SEV is in update state then encrypt the data else do nothing */
>      if (sev_check_state(sev_guest, SEV_STATE_LAUNCH_UPDATE)) {
> -        int ret = sev_launch_update_data(sev_guest, ptr, len);
> +        int ret;
> +
> +        if (sev_snp_enabled()) {
> +            ret = sev_snp_launch_update(sev_guest, ptr, len,
> +                                        KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +        } else {
> +            ret = sev_launch_update_data(sev_guest, ptr, len);
> +        }
>          if (ret < 0) {
>              error_setg(errp, "failed to encrypt pflash rom");
>              return ret;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 18cc14b956..0c2d250206 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -12,3 +12,4 @@ kvm_sev_launch_finish(void) ""
>  kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
>  kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
> +kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 1/6] linux-header: add the SNP specific command
  2021-07-19 11:35   ` Dov Murik
@ 2021-07-19 14:40     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-19 14:40 UTC (permalink / raw)
  To: Dov Murik, qemu-devel
  Cc: brijesh.singh, Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost

Hi Dov,


On 7/19/21 6:35 AM, Dov Murik wrote:
> Hi Brijesh,
> 
> On 10/07/2021 0:55, Brijesh Singh wrote:
>> Sync the kvm.h with the kernel to include the SNP specific commands.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   linux-headers/linux/kvm.h | 47 +++++++++++++++++++++++++++++++++++++++
> 
> 
> What about psp-sev.h ? I see that kernel patch "[PATCH Part2 RFC v4
> 11/40] crypto:ccp: Define the SEV-SNP commands" adds some new PSP return
> codes.
> 
> The QEMU user-friendly string list sev_fw_errlist (in sev.c) should be
> updated accordingly.
> 

thanks for reminding me, I will sync the psp-sev.h and include the new 
error code as well in the sev.c.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch
  2021-07-19 11:24   ` Dov Murik
@ 2021-07-19 14:45     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-19 14:45 UTC (permalink / raw)
  To: Dov Murik, qemu-devel
  Cc: brijesh.singh, Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost

Hi Dov,

On 7/19/21 6:24 AM, Dov Murik wrote:
> 
> s/LAUNCH_UPDATE/SNP_LAUNCH_UPDATE/
> (to show it's the same command you refer to above)
> 

Noted.

>>   
>> +static int
>> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
> 
> hwaddr is a confusing name here because it is also a typedef (which is
> most likely uint64_t...).  Maybe call this argument `gpa` ?
> 

Noted, 'gpa' sounds much better.

>> +static bool
>> +detectoverlap(uint32_t start, uint32_t end,
>> +              struct snp_pre_validated_range *overlap)
> 
> naming conventions dictate: detect_overlap
>

Noted.

>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
>> +        if (pre_validated[i].start < end && start < pre_validated[i].end) {
>> +            memcpy(overlap, &pre_validated[i], sizeof(*overlap));
> 
> Maybe simpler than memcpy:
> 
>      *overlap = pre_validated[i];
> 

Noted.

>> +
>> +    trace_kvm_sev_snp_launch_finish();
> 
> Maybe the trace should show some info about the snp_config.finish fields?
>

I did thought about it, but one of the field in the snp_config.finish is 
4K in size and may fill the trace buffer quickly.

>> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"
> 
> The last argument is an end-addr (not a length), so maybe the format
> string should end with:
> 
>     ".... pre-validate 0x%x - 0x%x"
> 
> Also I'd prefer to log the SevSnpBootInfoBlock fields in the order they
> appear in the struct.
> 
>

Noted.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 4/6] i386/sev: add the SNP launch start context
  2021-07-19 12:34   ` Dov Murik
@ 2021-07-19 15:27     ` Brijesh Singh
  0 siblings, 0 replies; 45+ messages in thread
From: Brijesh Singh @ 2021-07-19 15:27 UTC (permalink / raw)
  To: Dov Murik, qemu-devel
  Cc: brijesh.singh, Connor Kuehl, Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, David Gibson,
	Daniel P. Berrangé,
	kvm, Michael Roth, Eduardo Habkost



On 7/19/21 7:34 AM, Dov Murik wrote:
> Hi Brijesh,
> 
> On 10/07/2021 0:55, Brijesh Singh wrote:
>> The SNP_LAUNCH_START is called first to create a cryptographic launch
>> context within the firmware.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   target/i386/sev.c        | 30 +++++++++++++++++++++++++++++-
>>   target/i386/trace-events |  1 +
>>   2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 84ae244af0..259408a8f1 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
>>       return 0;
>>   }
>>   
>> +static int
>> +sev_snp_launch_start(SevGuestState *sev)
>> +{
>> +    int ret = 1;
>> +    int fw_error, rc;
>> +    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
>> +
>> +    trace_kvm_sev_snp_launch_start(start->policy);
>> +
>> +    rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error);
>> +    if (rc < 0) {
>> +        error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'",
>> +                __func__, ret, fw_error, fw_error_to_str(fw_error));
> 
> Did you mean to report the value of ret or rc?

Ah, I was meaning the rc.


> 
> 
>> +        goto out;
> 
> Suggestion:
> 
> Remove the `ret` variable.
> Here: simply `return 1`.
> At the end: remove the `out:` label; simply `return 0`.
> 

Noted.

thanks


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-13 13:46   ` Markus Armbruster
  2021-07-14 14:18     ` Brijesh Singh
@ 2021-07-20 19:42     ` Michael Roth
  2021-07-20 21:54       ` Daniel P. Berrangé
  1 sibling, 1 reply; 45+ messages in thread
From: Michael Roth @ 2021-07-20 19:42 UTC (permalink / raw)
  To: Markus Armbruster, Daniel P. Berrangé
  Cc: Brijesh Singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson, kvm,
	Eduardo Habkost

On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:
> Brijesh Singh <brijesh.singh@amd.com> writes:
> 
> > To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> > Passing all parameters through command line can be difficult. To simplify
> > the launch parameter passing, introduce a .ini-like config file that can be
> > used for passing the parameters to the launch flow.
> >
> > The contents of the config file will look like this:
> >
> > $ cat snp-launch.init
> >
> > # SNP launch parameters
> > [SEV-SNP]
> > init_flags = 0
> > policy = 0x1000
> > id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> >
> >
> > Add 'snp' property that can be used to indicate that SEV guest launch
> > should enable the SNP support.
> >
> > SEV-SNP guest launch examples:
> >
> > 1) launch without additional parameters
> >
> >   $(QEMU_CLI) \
> >     -object sev-guest,id=sev0,snp=on
> >
> > 2) launch with optional parameters
> >   $(QEMU_CLI) \
> >     -object sev-guest,id=sev0,snp=on,launch-config=<file>
> >
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> 
> I acknowledge doing complex configuration on the command line can be
> awkward.  But if we added a separate configuration file for every
> configurable thing where that's the case, we'd have too many already,
> and we'd constantly grow more.  I don't think this is a viable solution.
> 
> In my opinion, much of what we do on the command line should be done in
> configuration files instead.  Not in several different configuration
> languages, mind, but using one common language for all our configuration
> needs.
> 
> Some of us argue this language already exists: QMP.  It can't do
> everything the command line can do, but that's a matter of putting in
> the work.  However, JSON isn't a good configuration language[1].  To get
> a decent one, we'd have to to extend JSON[2], or wrap another concrete
> syntax around QMP's abstract syntax.
> 
> But this doesn't help you at all *now*.
> 
> I recommend to do exactly what we've done before for complex
> configuration: define it in the QAPI schema, so we can use both dotted
> keys and JSON on the command line, and can have QMP, too.  Examples:
> -blockdev, -display, -compat.
> 
> Questions?

Hi Markus, Daniel,

I'm dealing with similar considerations with some SNP config options
relating to CPUID enforcement, so I've started looking into this as
well, but am still a little confused on the best way to proceed.

I see that -blockdev supports both JSON command-line arguments (via
qobject_input_visitor_new) and dotted keys
(via qobject_input_vistior_new_keyval).

We could introduce a new config group do the same, maybe something specific
to ConfidentialGuestSupport objects, e.g.:

  -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...

and use the same mechanisms to parse the options, but this seems to
either involve adding a layer of option translations between command-line
and the underlying object properties, or, if we keep the 1:1 mapping
between QAPI-defined keys and object properties, it basically becomes a
way to pass a different Visitor implementation into object_property_set(),
in this case one created by object_input_visitor_new_keyval() instead of
opts_visitor_new().

In either case, genericizing it beyond CGS/SEV would basically be
introducing:

  -object2 sev-guest,id=sev0,key_a.subkey_b=...

Which one seems preferable? Or is the answer neither?

I've also been looking at whether this could all be handled via -object,
and it seems -object already supports JSON command-line arguments, and that
switching it from using OptsVisitor to QObjectVisitor for non-JSON case
would be enough to have it handle dotted keys, but I'm not sure what the
fall-out would be compatibility-wise.

All lot of that falls under making sure the QObject/keyval parser is
compatible with existing command-lines parsed via OptsVisitor. One example
where there still seems to be a difference is lack of support for ranges
such as "cpus=1-4" in keyval parser. Daniel had a series that addressed
this:

  https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html

but it doesn't seem to have made it into the tree, which is why I feel like
maybe there are complications with this approach I haven't considered?

Thanks!

-Mike

> 
> 
> [1] https://www.lucidchart.com/techblog/2018/07/16/why-json-isnt-a-good-configuration-language/
> 
> [2] Thanks, but no thanks.  Let's make new and interesting mistakes
> instead of repeating old and tired ones.
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-20 19:42     ` Michael Roth
@ 2021-07-20 21:54       ` Daniel P. Berrangé
  2021-07-21 13:08         ` Markus Armbruster
  0 siblings, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2021-07-20 21:54 UTC (permalink / raw)
  To: Michael Roth
  Cc: Tom Lendacky, Brijesh Singh, Eduardo Habkost, kvm,
	Michael S . Tsirkin, Connor Kuehl, James Bottomley,
	Markus Armbruster, qemu-devel, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	Dr . David Alan Gilbert, David Gibson

On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote:
> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:
> > Brijesh Singh <brijesh.singh@amd.com> writes:
> > 
> > > To launch the SEV-SNP guest, a user can specify up to 8 parameters.
> > > Passing all parameters through command line can be difficult. To simplify
> > > the launch parameter passing, introduce a .ini-like config file that can be
> > > used for passing the parameters to the launch flow.
> > >
> > > The contents of the config file will look like this:
> > >
> > > $ cat snp-launch.init
> > >
> > > # SNP launch parameters
> > > [SEV-SNP]
> > > init_flags = 0
> > > policy = 0x1000
> > > id_block = "YWFhYWFhYWFhYWFhYWFhCg=="
> > >
> > >
> > > Add 'snp' property that can be used to indicate that SEV guest launch
> > > should enable the SNP support.
> > >
> > > SEV-SNP guest launch examples:
> > >
> > > 1) launch without additional parameters
> > >
> > >   $(QEMU_CLI) \
> > >     -object sev-guest,id=sev0,snp=on
> > >
> > > 2) launch with optional parameters
> > >   $(QEMU_CLI) \
> > >     -object sev-guest,id=sev0,snp=on,launch-config=<file>
> > >
> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > I acknowledge doing complex configuration on the command line can be
> > awkward.  But if we added a separate configuration file for every
> > configurable thing where that's the case, we'd have too many already,
> > and we'd constantly grow more.  I don't think this is a viable solution.
> > 
> > In my opinion, much of what we do on the command line should be done in
> > configuration files instead.  Not in several different configuration
> > languages, mind, but using one common language for all our configuration
> > needs.
> > 
> > Some of us argue this language already exists: QMP.  It can't do
> > everything the command line can do, but that's a matter of putting in
> > the work.  However, JSON isn't a good configuration language[1].  To get
> > a decent one, we'd have to to extend JSON[2], or wrap another concrete
> > syntax around QMP's abstract syntax.
> > 
> > But this doesn't help you at all *now*.
> > 
> > I recommend to do exactly what we've done before for complex
> > configuration: define it in the QAPI schema, so we can use both dotted
> > keys and JSON on the command line, and can have QMP, too.  Examples:
> > -blockdev, -display, -compat.
> > 
> > Questions?
> 
> Hi Markus, Daniel,
> 
> I'm dealing with similar considerations with some SNP config options
> relating to CPUID enforcement, so I've started looking into this as
> well, but am still a little confused on the best way to proceed.
> 
> I see that -blockdev supports both JSON command-line arguments (via
> qobject_input_visitor_new) and dotted keys
> (via qobject_input_vistior_new_keyval).
> 
> We could introduce a new config group do the same, maybe something specific
> to ConfidentialGuestSupport objects, e.g.:
> 
>   -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...

We don't wnt to be adding new CLI options anymore. -object with json
syntx should ultimately be able to cover everything we'll ever need
to do.

> 
> and use the same mechanisms to parse the options, but this seems to
> either involve adding a layer of option translations between command-line
> and the underlying object properties, or, if we keep the 1:1 mapping
> between QAPI-defined keys and object properties, it basically becomes a
> way to pass a different Visitor implementation into object_property_set(),
> in this case one created by object_input_visitor_new_keyval() instead of
> opts_visitor_new().
> 
> In either case, genericizing it beyond CGS/SEV would basically be
> introducing:
> 
>   -object2 sev-guest,id=sev0,key_a.subkey_b=...
>
> Which one seems preferable? Or is the answer neither?

Yep, neither is the answer.

> 
> I've also been looking at whether this could all be handled via -object,
> and it seems -object already supports JSON command-line arguments, and that
> switching it from using OptsVisitor to QObjectVisitor for non-JSON case
> would be enough to have it handle dotted keys, but I'm not sure what the
> fall-out would be compatibility-wise.
> 
> All lot of that falls under making sure the QObject/keyval parser is
> compatible with existing command-lines parsed via OptsVisitor. One example
> where there still seems to be a difference is lack of support for ranges
> such as "cpus=1-4" in keyval parser. Daniel had a series that addressed
> this:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html
> 
> but it doesn't seem to have made it into the tree, which is why I feel like
> maybe there are complications with this approach I haven't considered?

The core problem with QemuOpts is that it has hacked various hacks
grafted onto it to cope with non-scalar data. My patch was adding
yet another hack. It very hard to introduce new hacks without risk
of causing incompatibility with other existing hacks, or introducing
unexpected edge cases that we don't anticipate.

Ultimately I think we've come to the conclusion that QemuOpts is an
unfixable dead end that should be left alone. Our future is trending
towards being entirely JSON, configured via the QMP monitor not the
CLI. As such the json support for -object is a step towards the pure
JSON world.

IOW, if you have things that work today with QemuOpts that's fine.

If, however, you're coming across situations that need non-scalar
data and it doesn't work with QemuOpts, then just declare that
-object json syntax is mandatory for that scenario. Don't invest
time trying to improve QemuOpts for non-scalar data, nor inventing
new CLI args.


FWIW, specificallly in the case of parameters that take an integer
range, like cores=1-4, in JSON we'd end up representing that as a
array of integers and having to specify all values explicitly.
This is quite verbose, but functionally works.

I think it would have been nice if we defined an explicit 'bitmap'
scalar data type in QAPI for these kind of things, but at this point
in time I think that ship has sailed, and trying to add that now
would create an inconsistent representation across different places.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-20 21:54       ` Daniel P. Berrangé
@ 2021-07-21 13:08         ` Markus Armbruster
  2021-07-22  0:02           ` Michael Roth via
  0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2021-07-21 13:08 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Tom Lendacky, Brijesh Singh, Eduardo Habkost, kvm,
	Michael S . Tsirkin, Connor Kuehl, Michael Roth, James Bottomley,
	qemu-devel, Dr . David Alan Gilbert, Dov Murik, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote:
>> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:

[...]

>> > I recommend to do exactly what we've done before for complex
>> > configuration: define it in the QAPI schema, so we can use both dotted
>> > keys and JSON on the command line, and can have QMP, too.  Examples:
>> > -blockdev, -display, -compat.
>> > 
>> > Questions?
>> 
>> Hi Markus, Daniel,
>> 
>> I'm dealing with similar considerations with some SNP config options
>> relating to CPUID enforcement, so I've started looking into this as
>> well, but am still a little confused on the best way to proceed.
>> 
>> I see that -blockdev supports both JSON command-line arguments (via
>> qobject_input_visitor_new) and dotted keys
>> (via qobject_input_vistior_new_keyval).

Yes.  Convenience function qobject_input_visitor_new_str() provides
this.

>> We could introduce a new config group do the same, maybe something specific
>> to ConfidentialGuestSupport objects, e.g.:
>> 
>>   -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...
>
> We don't wnt to be adding new CLI options anymore. -object with json
> syntx should ultimately be able to cover everything we'll ever need
> to do.

Depends.  When you want a CLI option to create a single QOM object, then
-object is commonly the way to go.

>> and use the same mechanisms to parse the options, but this seems to
>> either involve adding a layer of option translations between command-line
>> and the underlying object properties, or, if we keep the 1:1 mapping
>> between QAPI-defined keys and object properties, it basically becomes a
>> way to pass a different Visitor implementation into object_property_set(),
>> in this case one created by object_input_visitor_new_keyval() instead of
>> opts_visitor_new().

qobject_input_visitor_new_str() provides 1:1, i.e. common abstract
syntax, and concrete syntax either JSON or dotted keys.  Note that the
latter is slightly less expressive (can't do empty arrays and objects,
may fall apart for type 'any').  If you run into these limitations, use
JSON.  Machines should always use JSON.

qobject_input_visitor_new_str() works by wrapping the "right" visitor
around the option argument.  Another way to provide a human-friendly
interface in addition to a machine-friendly one is to translate from
human to the machine interface.  HMP works that way: HMP commands wrap
around QMP commands.  The QMP commands are generated from the QAPI
schema.  The HMP commands are written by hand, which is maximally
flexible, but also laborious.

>> In either case, genericizing it beyond CGS/SEV would basically be
>> introducing:
>> 
>>   -object2 sev-guest,id=sev0,key_a.subkey_b=...

That's because existing -object doesn't use keyval_parse() + the keyval
QObjct input visitor, it uses QemuOpts and the options visitor, for
backward compatibility with all their (barely understood) features and
warts.

Unfortunate, because even new user-creatable objects can't escape
QemuOpts.

QemuOpts needs to go.  But replacing it is difficult and scary in
places.  -object is such a place.

>> Which one seems preferable? Or is the answer neither?
>
> Yep, neither is the answer.

Welcome to the QemuOpts swamp, here's your waders and a leaky bucket.

>> I've also been looking at whether this could all be handled via -object,
>> and it seems -object already supports JSON command-line arguments, and that
>> switching it from using OptsVisitor to QObjectVisitor for non-JSON case
>> would be enough to have it handle dotted keys, but I'm not sure what the
>> fall-out would be compatibility-wise.

It's complicated, and nobody knows for sure.  That's why we didn't dare
to take the jump, and instead grafted on JSON syntax without changing
the old syntax.  Excuse me while I sacrifice another rubber chicken to
the backward compatibility idol.

>> All lot of that falls under making sure the QObject/keyval parser is
>> compatible with existing command-lines parsed via OptsVisitor. One example
>> where there still seems to be a difference is lack of support for ranges
>> such as "cpus=1-4" in keyval parser. Daniel had a series that addressed
>> this:
>> 
>>   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html
>> 
>> but it doesn't seem to have made it into the tree, which is why I feel like
>> maybe there are complications with this approach I haven't considered?
>
> The core problem with QemuOpts is that it has hacked various hacks
> grafted onto it to cope with non-scalar data. My patch was adding
> yet another hack. It very hard to introduce new hacks without risk
> of causing incompatibility with other existing hacks, or introducing
> unexpected edge cases that we don't anticipate.

Some of the thornier compatibility issues with QemuOpts are due to
unforeseen edge cases of, and interactions between features.

> Ultimately I think we've come to the conclusion that QemuOpts is an
> unfixable dead end that should be left alone. Our future is trending
> towards being entirely JSON, configured via the QMP monitor not the
> CLI. As such the json support for -object is a step towards the pure
> JSON world.

QemuOpts served us well for a while, but we've long grown out of its
limitations.  It needs to go.

QemuOpts not providing an adequate CLI language does not imply we can't
have an adequate CLI language.  The "everything QMP" movement is due to
other factors.  I have serious reservations about the idea, actually.

> IOW, if you have things that work today with QemuOpts that's fine.
>
> If, however, you're coming across situations that need non-scalar
> data and it doesn't work with QemuOpts, then just declare that
> -object json syntax is mandatory for that scenario. Don't invest
> time trying to improve QemuOpts for non-scalar data, nor inventing
> new CLI args.

I agree 100% with "don't mess with QemuOpts".  That mess is complete.

Whether a new CLI option makes sense should be decided case by case.

"Must use JSON" feels acceptable for things basically only management
applications use.

> FWIW, specificallly in the case of parameters that take an integer
> range, like cores=1-4, in JSON we'd end up representing that as a
> array of integers and having to specify all values explicitly.
> This is quite verbose, but functionally works.
>
> I think it would have been nice if we defined an explicit 'bitmap'
> scalar data type in QAPI for these kind of things, but at this point
> in time I think that ship has sailed, and trying to add that now
> would create an inconsistent representation across different places.

External representation (i.e. JSON) should be as consistent as we can
make it.  Multiple different internal representations can be okay.  So
far, we represent JSON arrays as linked lists.  Optionally representing
certain arrays of small integers as bit vectors feels feasible.  Whether
it's worth the effort is another question.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP
  2021-07-21 13:08         ` Markus Armbruster
@ 2021-07-22  0:02           ` Michael Roth via
  0 siblings, 0 replies; 45+ messages in thread
From: Michael Roth via @ 2021-07-22  0:02 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé,
	Brijesh Singh, qemu-devel, Connor Kuehl,
	Philippe Mathieu-Daudé,
	Michael S . Tsirkin, James Bottomley, Dr . David Alan Gilbert,
	Tom Lendacky, Paolo Bonzini, Dov Murik, David Gibson, kvm,
	Eduardo Habkost

On Wed, Jul 21, 2021 at 03:08:37PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote:
> >> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:
> 
> [...]
> 
> >> > I recommend to do exactly what we've done before for complex
> >> > configuration: define it in the QAPI schema, so we can use both dotted
> >> > keys and JSON on the command line, and can have QMP, too.  Examples:
> >> > -blockdev, -display, -compat.
> >> > 
> >> > Questions?
> >> 
> >> Hi Markus, Daniel,
> >> 
> >> I'm dealing with similar considerations with some SNP config options
> >> relating to CPUID enforcement, so I've started looking into this as
> >> well, but am still a little confused on the best way to proceed.
> >> 
> >> I see that -blockdev supports both JSON command-line arguments (via
> >> qobject_input_visitor_new) and dotted keys
> >> (via qobject_input_vistior_new_keyval).
> 
> Yes.  Convenience function qobject_input_visitor_new_str() provides
> this.
> 
> >> We could introduce a new config group do the same, maybe something specific
> >> to ConfidentialGuestSupport objects, e.g.:
> >> 
> >>   -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...
> >
> > We don't wnt to be adding new CLI options anymore. -object with json
> > syntx should ultimately be able to cover everything we'll ever need
> > to do.
> 
> Depends.  When you want a CLI option to create a single QOM object, then
> -object is commonly the way to go.

So if I've read things correctly the fact that this is a question of how to
define properties of a single QOM object lends itself to using -object rather
than attempting a new -blockdev-like option group, and as such if we
want to allow structured parameters we should use pure JSON instead of
attempting to layer anything on top of the current keyval parser.

> 
> >> and use the same mechanisms to parse the options, but this seems to
> >> either involve adding a layer of option translations between command-line
> >> and the underlying object properties, or, if we keep the 1:1 mapping
> >> between QAPI-defined keys and object properties, it basically becomes a
> >> way to pass a different Visitor implementation into object_property_set(),
> >> in this case one created by object_input_visitor_new_keyval() instead of
> >> opts_visitor_new().
> 
> qobject_input_visitor_new_str() provides 1:1, i.e. common abstract
> syntax, and concrete syntax either JSON or dotted keys.  Note that the
> latter is slightly less expressive (can't do empty arrays and objects,
> may fall apart for type 'any').  If you run into these limitations, use
> JSON.  Machines should always use JSON.
> 
> qobject_input_visitor_new_str() works by wrapping the "right" visitor
> around the option argument.  Another way to provide a human-friendly
> interface in addition to a machine-friendly one is to translate from
> human to the machine interface.  HMP works that way: HMP commands wrap
> around QMP commands.  The QMP commands are generated from the QAPI
> schema.  The HMP commands are written by hand, which is maximally
> flexible, but also laborious.
> 
> >> In either case, genericizing it beyond CGS/SEV would basically be
> >> introducing:
> >> 
> >>   -object2 sev-guest,id=sev0,key_a.subkey_b=...
> 
> That's because existing -object doesn't use keyval_parse() + the keyval
> QObjct input visitor, it uses QemuOpts and the options visitor, for
> backward compatibility with all their (barely understood) features and
> warts.
> 
> Unfortunate, because even new user-creatable objects can't escape
> QemuOpts.
> 
> QemuOpts needs to go.  But replacing it is difficult and scary in
> places.  -object is such a place.
> 
> >> Which one seems preferable? Or is the answer neither?
> >
> > Yep, neither is the answer.
> 
> Welcome to the QemuOpts swamp, here's your waders and a leaky bucket.

*backs slowly away from swamp* :)

So back to the question of whether we need structured parameters. The
main driver for this seems to be that the options are currently defined
via a config file, which was originally introduced to cope with:

a) lots of parameters (8)

   - not really that significant compared to some other objects/options.

b) large page-size parameters

   - mainly applies to 'id_auth', which could be broken out as individual
     files/blobs and passed in via normal file path string arguments
   - already how we handle dh-cert-file and session-file

c) separating SNP-specific parameters from the base sev-guest object
   properties

   - could possibly be done with a new 'sev-snp-guest' object, which
     would also help disambiguate stuff like the 32-bit sev/sev-es
     'policy' arguments from the 64-bit version in SNP, and can still
     re-use common properties like 'cbitpos' via some base object
   - maybe some other benefits, need to look into it more.

If they aren't any objections I'll take a stab at this early next week.
Will be on PTO until then, but will follow-up soon as I'm back in office.

> > Ultimately I think we've come to the conclusion that QemuOpts is an
> > unfixable dead end that should be left alone. Our future is trending
> > towards being entirely JSON, configured via the QMP monitor not the
> > CLI. As such the json support for -object is a step towards the pure
> > JSON world.
> 
> QemuOpts served us well for a while, but we've long grown out of its
> limitations.  It needs to go.
> 
> QemuOpts not providing an adequate CLI language does not imply we can't
> have an adequate CLI language.  The "everything QMP" movement is due to
> other factors.  I have serious reservations about the idea, actually.
> 
> > IOW, if you have things that work today with QemuOpts that's fine.
> >
> > If, however, you're coming across situations that need non-scalar
> > data and it doesn't work with QemuOpts, then just declare that
> > -object json syntax is mandatory for that scenario. Don't invest
> > time trying to improve QemuOpts for non-scalar data, nor inventing
> > new CLI args.
> 
> I agree 100% with "don't mess with QemuOpts".  That mess is complete.
> 
> Whether a new CLI option makes sense should be decided case by case.
> 
> "Must use JSON" feels acceptable for things basically only management
> applications use.

Yah, it's great that QAPI/object_add already takes care of the management
side of things, but giving up the option of using human-readable/non-JSON
command-line args is still a tough pill to swallow, at least as a developer
where I know some significant portion of my life will be spent debugging
parser errors from bash.

*backs up too far, walks into adjacent swamp with no waders*

If more cases where structured arguments for Objects really make sense and
thus necessitate JSON-only, it would be great if there was some developer
tool, e.g. scripts/qemu-cmdline-helper-devs-only that could handle this
translation to JSON or QMP and maybe serve as a test-bed for this awesome
new cmdline syntax that provides all the expressiveness of QAPI and could
abstract away the QemuOpts-specific option formats while still allowing
for periodic reworks.

Or maybe -readconfig is the better starting point? Or is it too late for
that already? -x-readconfig2 ? :)

*disappears into swamp*


^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2021-07-22  0:13 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 21:55 [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
2021-07-09 21:55 ` [RFC PATCH 1/6] linux-header: add the SNP specific command Brijesh Singh
2021-07-10 20:32   ` Michael S. Tsirkin
2021-07-12 15:48     ` Brijesh Singh
2021-07-19 11:35   ` Dov Murik
2021-07-19 14:40     ` Brijesh Singh
2021-07-09 21:55 ` [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP Brijesh Singh
2021-07-12  6:09   ` Dov Murik
2021-07-12 14:34   ` Dr. David Alan Gilbert
2021-07-12 15:59     ` Brijesh Singh
2021-07-12 16:16       ` Dr. David Alan Gilbert
2021-07-12 14:43   ` Daniel P. Berrangé
2021-07-12 15:56     ` Brijesh Singh
2021-07-12 16:24       ` Daniel P. Berrangé
2021-07-13 13:54         ` Brijesh Singh
2021-07-13 13:46   ` Markus Armbruster
2021-07-14 14:18     ` Brijesh Singh
2021-07-20 19:42     ` Michael Roth
2021-07-20 21:54       ` Daniel P. Berrangé
2021-07-21 13:08         ` Markus Armbruster
2021-07-22  0:02           ` Michael Roth via
2021-07-13 18:21   ` Eric Blake
2021-07-09 21:55 ` [RFC PATCH 3/6] i386/sev: initialize SNP context Brijesh Singh
2021-07-15  9:32   ` Dov Murik
2021-07-15 13:24     ` Brijesh Singh
2021-07-09 21:55 ` [RFC PATCH 4/6] i386/sev: add the SNP launch start context Brijesh Singh
2021-07-19 12:34   ` Dov Murik
2021-07-19 15:27     ` Brijesh Singh
2021-07-09 21:55 ` [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled Brijesh Singh
2021-07-14 17:08   ` Connor Kuehl
2021-07-14 18:52     ` Brijesh Singh
2021-07-15  5:54       ` Dov Murik
2021-07-19 13:00   ` Dov Murik
2021-07-09 21:55 ` [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch Brijesh Singh
2021-07-14 17:29   ` Dr. David Alan Gilbert
2021-07-14 18:53     ` Brijesh Singh
2021-07-19 11:24   ` Dov Murik
2021-07-19 14:45     ` Brijesh Singh
2021-07-12 17:00 ` [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support Tom Lendacky
2021-07-13  8:05 ` Dov Murik
2021-07-13  8:31   ` Dr. David Alan Gilbert
2021-07-13 13:57     ` Brijesh Singh
2021-07-13 14:01   ` Brijesh Singh
2021-07-14  9:52     ` Dr. David Alan Gilbert
2021-07-14 14:23       ` Brijesh Singh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).