[RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Tyler Fanelli]

These patches are submitted as an RFC mainly because I'm a relative
newcomer to QEMU with no knowledge of the community's views on
including Rust code, nor it's preference of using library APIs for
ioctls that were previously implemented in QEMU directly.

Recently, the Rust sev library [0] has introduced a C API to take
advantage of the library outside of Rust.

Should the inclusion of the library as a dependency be desired, it can
be extended further to include the firmware/platform ioctls, the
attestation report fetching, and more. This would result in much of
the AMD-SEV portion of QEMU being offloaded to the library.

This series looks to explore the possibility of using the library and
show a bit of what it would look like. I'm looking for comments
regarding if this feature is desired.

[0] https://github.com/virtee/sev

Tyler Fanelli (8):
  Add SEV Rust library as dependency with CONFIG_SEV
  i386/sev: Replace INIT and ES_INIT ioctls with sev library equivalents
  i386/sev: Replace LAUNCH_START ioctl with sev library equivalent
  i386/sev: Replace UPDATE_DATA ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_UPDATE_VMSA ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_MEASURE ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_SECRET ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_FINISH ioctl with sev library equivalent

 meson.build                   |   7 +
 meson_options.txt             |   2 +
 scripts/meson-buildoptions.sh |   3 +
 target/i386/meson.build       |   2 +-
 target/i386/sev.c             | 311 ++++++++++++----------------------
 target/i386/sev.h             |   4 +-
 target/i386/trace-events      |   1 +
 7 files changed, 123 insertions(+), 207 deletions(-)

-- 
2.40.1

[RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Tyler Fanelli]

These patches are submitted as an RFC mainly because I'm a relative
newcomer to QEMU with no knowledge of the community's views on
including Rust code, nor it's preference of using library APIs for
ioctls that were previously implemented in QEMU directly.

Recently, the Rust sev library [0] has introduced a C API to take
advantage of the library outside of Rust.

Should the inclusion of the library as a dependency be desired, it can
be extended further to include the firmware/platform ioctls, the
attestation report fetching, and more. This would result in much of
the AMD-SEV portion of QEMU being offloaded to the library.

This series looks to explore the possibility of using the library and
show a bit of what it would look like. I'm looking for comments
regarding if this feature is desired.

[0] https://github.com/virtee/sev

Tyler Fanelli (8):
  Add SEV Rust library as dependency with CONFIG_SEV
  i386/sev: Replace INIT and ES_INIT ioctls with sev library equivalents
  i386/sev: Replace LAUNCH_START ioctl with sev library equivalent
  i386/sev: Replace UPDATE_DATA ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_UPDATE_VMSA ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_MEASURE ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_SECRET ioctl with sev library equivalent
  i386/sev: Replace LAUNCH_FINISH ioctl with sev library equivalent

 meson.build                   |   7 +
 meson_options.txt             |   2 +
 scripts/meson-buildoptions.sh |   3 +
 target/i386/meson.build       |   2 +-
 target/i386/sev.c             | 311 ++++++++++++----------------------
 target/i386/sev.h             |   4 +-
 target/i386/trace-events      |   1 +
 7 files changed, 123 insertions(+), 207 deletions(-)

-- 
2.40.1

[RFC PATCH 1/8] Add SEV Rust library as dependency with CONFIG_SEV

[Tyler Fanelli]

The Rust sev library provides a type-safe implementation of the AMD
Secure Encrypted Virtualization (SEV) APIs.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 meson.build                   | 7 +++++++
 meson_options.txt             | 2 ++
 scripts/meson-buildoptions.sh | 3 +++
 target/i386/meson.build       | 2 +-
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 5150a74831..7114a4a2b9 100644
--- a/meson.build
+++ b/meson.build
@@ -1079,6 +1079,12 @@ if targetos == 'linux' and (have_system or have_tools)
                        method: 'pkg-config',
                        required: get_option('libudev'))
 endif
+sev = not_found
+if not get_option('sev').auto()
+  sev = dependency('sev', version: '1.2.1',
+                      method: 'pkg-config',
+                      required: get_option('sev'))
+endif
 
 mpathlibs = [libudev]
 mpathpersist = not_found
@@ -4283,6 +4289,7 @@ summary_info += {'PAM':               pam}
 summary_info += {'iconv support':     iconv}
 summary_info += {'virgl support':     virgl}
 summary_info += {'blkio support':     blkio}
+summary_info += {'sev support':       sev}
 summary_info += {'curl support':      curl}
 summary_info += {'Multipath support': mpathpersist}
 summary_info += {'Linux AIO support': libaio}
diff --git a/meson_options.txt b/meson_options.txt
index f82d88b7c6..c57d542c0b 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -134,6 +134,8 @@ option('cap_ng', type : 'feature', value : 'auto',
        description: 'cap_ng support')
 option('blkio', type : 'feature', value : 'auto',
        description: 'libblkio block device driver')
+option('sev', type : 'feature', value : 'auto',
+        description: 'SEV Rust library')
 option('bpf', type : 'feature', value : 'auto',
         description: 'eBPF support')
 option('cocoa', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index e1d178370c..d7deb50bda 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -83,6 +83,7 @@ meson_options_help() {
   printf "%s\n" '  avx512bw        AVX512BW optimizations'
   printf "%s\n" '  avx512f         AVX512F optimizations'
   printf "%s\n" '  blkio           libblkio block device driver'
+  printf "%s\n" '  sev             SEV Rust library'
   printf "%s\n" '  bochs           bochs image format support'
   printf "%s\n" '  bpf             eBPF support'
   printf "%s\n" '  brlapi          brlapi character device driver'
@@ -227,6 +228,8 @@ _meson_option_parse() {
     --disable-lto) printf "%s" -Db_lto=false ;;
     --enable-blkio) printf "%s" -Dblkio=enabled ;;
     --disable-blkio) printf "%s" -Dblkio=disabled ;;
+    --enable-sev) printf "%s" -Dsev=enabled ;;
+    --disable-sev) printf "%s" -Dsev=disabled ;;
     --block-drv-ro-whitelist=*) quote_sh "-Dblock_drv_ro_whitelist=$2" ;;
     --block-drv-rw-whitelist=*) quote_sh "-Dblock_drv_rw_whitelist=$2" ;;
     --enable-block-drv-whitelist-in-tools) printf "%s" -Dblock_drv_whitelist_in_tools=true ;;
diff --git a/target/i386/meson.build b/target/i386/meson.build
index 6f1036d469..18450dc134 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,7 @@ i386_ss.add(files(
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: [sev, files('host-cpu.c')])
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
-- 
2.40.1

[RFC PATCH 2/8] i386/sev: Replace INIT and ES_INIT ioctls with sev library equivalents

[Tyler Fanelli]

The sev library offers APIs for SEV_INIT and SEV_ES_INIT, both taking
the file descriptors of the encrypting VM and /dev/sev as input.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c        | 14 +++++++++-----
 target/i386/trace-events |  1 +
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index fe2144c038..f0fd291e68 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -18,6 +18,8 @@
 
 #include <sys/ioctl.h>
 
+#include <sev/sev.h>
+
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
 #include "qemu/base64.h"
@@ -27,6 +29,7 @@
 #include "crypto/hash.h"
 #include "sysemu/kvm.h"
 #include "sev.h"
+#include "sysemu/kvm_int.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 #include "trace.h"
@@ -911,10 +914,11 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     SevGuestState *sev
         = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
     char *devname;
-    int ret, fw_error, cmd;
+    int ret, fw_error;
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
+    KVMState *s = kvm_state;
 
     if (!sev) {
         return 0;
@@ -990,13 +994,13 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
                          __func__);
             goto err;
         }
-        cmd = KVM_SEV_ES_INIT;
+        trace_kvm_sev_es_init();
+        ret = sev_es_init(s->vmfd, sev->sev_fd, &fw_error);
     } else {
-        cmd = KVM_SEV_INIT;
+        trace_kvm_sev_init();
+        ret = sev_init(s->vmfd, sev->sev_fd, &fw_error);
     }
 
-    trace_kvm_sev_init();
-    ret = sev_ioctl(sev->sev_fd, cmd, NULL, &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/trace-events b/target/i386/trace-events
index 2cd8726eeb..2dca4ee117 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -2,6 +2,7 @@
 
 # sev.c
 kvm_sev_init(void) ""
+kvm_sev_es_init(void) ""
 kvm_memcrypt_register_region(void *addr, size_t len) "addr %p len 0x%zx"
 kvm_memcrypt_unregister_region(void *addr, size_t len) "addr %p len 0x%zx"
 kvm_sev_change_state(const char *old, const char *new) "%s -> %s"
-- 
2.40.1

[RFC PATCH 3/8] i386/sev: Replace LAUNCH_START ioctl with sev library equivalent

[Tyler Fanelli]

The sev library offers an equivalent API for SEV_LAUNCH_START. The
library contains some internal state for each VM it's currently running,
and organizes the internal state for each VM via it's file descriptor.
Therefore, the VM's file descriptor must be provided as input.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 80 ++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 50 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index f0fd291e68..49be072cbc 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -715,51 +715,6 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
     return 0;
 }
 
-static int
-sev_launch_start(SevGuestState *sev)
-{
-    gsize sz;
-    int ret = 1;
-    int fw_error, rc;
-    struct kvm_sev_launch_start start = {
-        .handle = sev->handle, .policy = sev->policy
-    };
-    guchar *session = NULL, *dh_cert = NULL;
-
-    if (sev->session_file) {
-        if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
-            goto out;
-        }
-        start.session_uaddr = (unsigned long)session;
-        start.session_len = sz;
-    }
-
-    if (sev->dh_cert_file) {
-        if (sev_read_file_base64(sev->dh_cert_file, &dh_cert, &sz) < 0) {
-            goto out;
-        }
-        start.dh_uaddr = (unsigned long)dh_cert;
-        start.dh_len = sz;
-    }
-
-    trace_kvm_sev_launch_start(start.policy, session, dh_cert);
-    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, &start, &fw_error);
-    if (rc < 0) {
-        error_report("%s: 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);
-    sev->handle = start.handle;
-    ret = 0;
-
-out:
-    g_free(session);
-    g_free(dh_cert);
-    return ret;
-}
-
 static int
 sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
 {
@@ -913,11 +868,13 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     SevGuestState *sev
         = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
+    gsize sz;
     char *devname;
-    int ret, fw_error;
+    int ret = -1, fw_error;
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
+    guchar *session = NULL, *dh_cert = NULL;
     KVMState *s = kvm_state;
 
     if (!sev) {
@@ -1007,23 +964,46 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         goto err;
     }
 
-    ret = sev_launch_start(sev);
+    if (!sev->session_file || !sev->dh_cert_file) {
+        goto err;
+    }
+
+    if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
+        goto err;
+    }
+
+    if (sev_read_file_base64(sev->dh_cert_file, &dh_cert, &sz) < 0) {
+        goto err;
+    }
+
+    ret = sev_launch_start(s->vmfd, sev->policy, (void *) dh_cert,
+                           (void *) session, &fw_error);
     if (ret) {
-        error_setg(errp, "%s: failed to create encryption context", __func__);
+        error_setg(errp, "%s: LAUNCH_START ret=%d fw_error=%d '%s'",
+                   __func__, ret, fw_error, fw_error_to_str(fw_error));
         goto err;
     }
 
+    sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
+
     ram_block_notifier_add(&sev_ram_notifier);
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
     cgs->ready = true;
 
-    return 0;
+    ret = 0;
+    goto out;
+
 err:
     sev_guest = NULL;
     ram_block_discard_disable(false);
-    return -1;
+out:
+    g_free(session);
+    g_free(dh_cert);
+
+    return ret;
+
 }
 
 int
-- 
2.40.1

[RFC PATCH 4/8] i386/sev: Replace UPDATE_DATA ioctl with sev library equivalent

[Tyler Fanelli]

UPDATE_DATA takes the VM's file descriptor, a guest memory region to
be encrypted, as well as the size of the aforementioned guest memory
region.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 49be072cbc..615021a1a3 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -715,29 +715,6 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
     return 0;
 }
 
-static int
-sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
-{
-    int ret, fw_error;
-    struct kvm_sev_launch_update_data update;
-
-    if (!addr || !len) {
-        return 1;
-    }
-
-    update.uaddr = (__u64)(unsigned long)addr;
-    update.len = len;
-    trace_kvm_sev_launch_update_data(addr, len);
-    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_DATA,
-                    &update, &fw_error);
-    if (ret) {
-        error_report("%s: 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_vmsa(SevGuestState *sev)
 {
@@ -1009,15 +986,19 @@ out:
 int
 sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
 {
+    KVMState *s = kvm_state;
+    int fw_error;
+
     if (!sev_guest) {
         return 0;
     }
 
     /* 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 = sev_launch_update_data(s->vmfd, (__u64) ptr, len, &fw_error);
         if (ret < 0) {
-            error_setg(errp, "SEV: Failed to encrypt pflash rom");
+            error_setg(errp, "SEV: Failed to encrypt pflash rom fw_err=%d",
+                       fw_error);
             return ret;
         }
     }
-- 
2.40.1

[RFC PATCH 5/8] i386/sev: Replace LAUNCH_UPDATE_VMSA ioctl with sev library equivalent

[Tyler Fanelli]

The LAUNCH_UPDATE_VMSA API takes the VM's file descriptor, as well as a
field for any firmware errors as input.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 615021a1a3..adb35291e8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -715,27 +715,14 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
     return 0;
 }
 
-static int
-sev_launch_update_vmsa(SevGuestState *sev)
-{
-    int ret, fw_error;
-
-    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL, &fw_error);
-    if (ret) {
-        error_report("%s: LAUNCH_UPDATE_VMSA ret=%d fw_error=%d '%s'",
-                __func__, ret, fw_error, fw_error_to_str(fw_error));
-    }
-
-    return ret;
-}
-
 static void
 sev_launch_get_measure(Notifier *notifier, void *unused)
 {
     SevGuestState *sev = sev_guest;
-    int ret, error;
+    int ret, fw_error;
     g_autofree guchar *data = NULL;
     struct kvm_sev_launch_measure measurement = {};
+    KVMState *s = kvm_state;
 
     if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
         return;
@@ -743,18 +730,20 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
 
     if (sev_es_enabled()) {
         /* measure all the VM save areas before getting launch_measure */
-        ret = sev_launch_update_vmsa(sev);
+        ret = sev_launch_update_vmsa(s->vmfd, &fw_error);
         if (ret) {
+            error_report("%s: LAUNCH_UPDATE_VMSA ret=%d fw_error=%d '%s'",
+                __func__, ret, fw_error, fw_error_to_str(fw_error));
             exit(1);
         }
     }
 
     /* query the measurement blob length */
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    &measurement, &error);
+                    &measurement, &fw_error);
     if (!measurement.len) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
-                     __func__, ret, error, fw_error_to_str(errno));
+                     __func__, ret, fw_error, fw_error_to_str(fw_error));
         return;
     }
 
@@ -763,10 +752,10 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
 
     /* get the measurement blob */
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    &measurement, &error);
+                    &measurement, &fw_error);
     if (ret) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
-                     __func__, ret, error, fw_error_to_str(errno));
+                     __func__, ret, fw_error, fw_error_to_str(fw_error));
         return;
     }
 
-- 
2.40.1

[RFC PATCH 6/8] i386/sev: Replace LAUNCH_MEASURE ioctl with sev library equivalent

[Tyler Fanelli]

The LAUNCH_MEASURE API returns the measurement of the launched guest's
memory pages (and VMCB save areas if ES is enabled). The caller is
responsible for ensuring that the pointer (identified as the "data"
argument) is a valid pointer that can hold the guest's measurement (a
measurement in SEV is 48 bytes in size).

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 24 ++++++------------------
 target/i386/sev.h |  2 ++
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index adb35291e8..f53ff140e3 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -721,7 +721,6 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     SevGuestState *sev = sev_guest;
     int ret, fw_error;
     g_autofree guchar *data = NULL;
-    struct kvm_sev_launch_measure measurement = {};
     KVMState *s = kvm_state;
 
     if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
@@ -738,31 +737,20 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
         }
     }
 
-    /* query the measurement blob length */
-    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    &measurement, &fw_error);
-    if (!measurement.len) {
-        error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
-                     __func__, ret, fw_error, fw_error_to_str(fw_error));
-        return;
-    }
+    data = g_malloc(SEV_MEASUREMENT_SIZE);
 
-    data = g_new0(guchar, measurement.len);
-    measurement.uaddr = (unsigned long)data;
-
-    /* get the measurement blob */
-    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    &measurement, &fw_error);
+    ret = sev_launch_measure(s->vmfd, data, &fw_error);
     if (ret) {
-        error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
-                     __func__, ret, fw_error, fw_error_to_str(fw_error));
+        error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'", __func__,
+                       ret, fw_error, fw_error_to_str(fw_error));
+
         return;
     }
 
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_SECRET);
 
     /* encode the measurement value and emit the event */
-    sev->measurement = g_base64_encode(data, measurement.len);
+    sev->measurement = g_base64_encode(data, SEV_MEASUREMENT_SIZE);
     trace_kvm_sev_launch_measurement(sev->measurement);
 }
 
diff --git a/target/i386/sev.h b/target/i386/sev.h
index e7499c95b1..acb181358e 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -38,6 +38,8 @@ typedef struct SevKernelLoaderContext {
     size_t cmdline_size;
 } SevKernelLoaderContext;
 
+#define SEV_MEASUREMENT_SIZE 48
+
 #ifdef CONFIG_SEV
 bool sev_enabled(void);
 bool sev_es_enabled(void);
-- 
2.40.1

[RFC PATCH 7/8] i386/sev: Replace LAUNCH_SECRET ioctl with sev library equivalent

[Tyler Fanelli]

The LAUNCH_SECRET API can inject a secret into the VM once the
measurement has been retrieved.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 105 ++++++++++++++++------------------------------
 target/i386/sev.h |   2 -
 2 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index f53ff140e3..a4510b5437 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -983,88 +983,44 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
     return 0;
 }
 
-int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
-                             uint64_t gpa, Error **errp)
-{
-    struct kvm_sev_launch_secret input;
-    g_autofree guchar *data = NULL, *hdr = NULL;
-    int error, ret = 1;
-    void *hva;
-    gsize hdr_sz = 0, data_sz = 0;
-    MemoryRegion *mr = NULL;
-
-    if (!sev_guest) {
-        error_setg(errp, "SEV not enabled for guest");
-        return 1;
-    }
-
-    /* secret can be injected only in this state */
-    if (!sev_check_state(sev_guest, SEV_STATE_LAUNCH_SECRET)) {
-        error_setg(errp, "SEV: Not in correct state. (LSECRET) %x",
-                     sev_guest->state);
-        return 1;
-    }
-
-    hdr = g_base64_decode(packet_hdr, &hdr_sz);
-    if (!hdr || !hdr_sz) {
-        error_setg(errp, "SEV: Failed to decode sequence header");
-        return 1;
-    }
-
-    data = g_base64_decode(secret, &data_sz);
-    if (!data || !data_sz) {
-        error_setg(errp, "SEV: Failed to decode data");
-        return 1;
-    }
-
-    hva = gpa2hva(&mr, gpa, data_sz, errp);
-    if (!hva) {
-        error_prepend(errp, "SEV: Failed to calculate guest address: ");
-        return 1;
-    }
-
-    input.hdr_uaddr = (uint64_t)(unsigned long)hdr;
-    input.hdr_len = hdr_sz;
-
-    input.trans_uaddr = (uint64_t)(unsigned long)data;
-    input.trans_len = data_sz;
-
-    input.guest_uaddr = (uint64_t)(unsigned long)hva;
-    input.guest_len = data_sz;
-
-    trace_kvm_sev_launch_secret(gpa, input.guest_uaddr,
-                                input.trans_uaddr, input.trans_len);
-
-    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_LAUNCH_SECRET,
-                    &input, &error);
-    if (ret) {
-        error_setg(errp, "SEV: failed to inject secret ret=%d fw_error=%d '%s'",
-                     ret, error, fw_error_to_str(error));
-        return ret;
-    }
-
-    return 0;
-}
-
 #define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
 struct sev_secret_area {
     uint32_t base;
     uint32_t size;
 };
 
-void qmp_sev_inject_launch_secret(const char *packet_hdr,
-                                  const char *secret,
+void qmp_sev_inject_launch_secret(const char *hdr_b64,
+                                  const char *secret_b64,
                                   bool has_gpa, uint64_t gpa,
                                   Error **errp)
 {
+    int ret, fw_error = 0;
+    g_autofree guchar *hdr = NULL, *secret = NULL;
+    uint8_t *data = NULL;
+    KVMState *s = kvm_state;
+    gsize hdr_sz = 0, secret_sz = 0;
+    MemoryRegion *mr = NULL;
+    void *hva;
+    struct sev_secret_area *area = NULL;
+
     if (!sev_enabled()) {
         error_setg(errp, "SEV not enabled for guest");
         return;
     }
-    if (!has_gpa) {
-        uint8_t *data;
-        struct sev_secret_area *area;
 
+    hdr = g_base64_decode(hdr_b64, &hdr_sz);
+    if (!hdr || !hdr_sz) {
+        error_setg(errp, "SEV: Failed to decode sequence header");
+        return;
+    }
+
+    secret = g_base64_decode(secret_b64, &secret_sz);
+    if (!secret || !secret_sz) {
+        error_setg(errp, "SEV: Failed to decode secret");
+        return;
+    }
+
+    if (!has_gpa) {
         if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
             error_setg(errp, "SEV: no secret area found in OVMF,"
                        " gpa must be specified.");
@@ -1074,7 +1030,18 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
         gpa = area->base;
     }
 
-    sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
+    hva = gpa2hva(&mr, gpa, secret_sz, errp);
+    if (!hva) {
+        error_prepend(errp, "SEV: Failed to calculate guest address: ");
+        return;
+    }
+
+    ret = sev_inject_launch_secret(s->vmfd, hdr, secret, secret_sz,
+                                   hva, &fw_error);
+    if (ret < 0) {
+        error_setg(errp, "%s: LAUNCH_SECRET ret=%d fw_error=%d '%s'", __func__,
+                   ret, fw_error, fw_error_to_str(fw_error));
+    }
 }
 
 static int
diff --git a/target/i386/sev.h b/target/i386/sev.h
index acb181358e..f1af28eca0 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -53,8 +53,6 @@ uint32_t sev_get_reduced_phys_bits(void);
 bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
-int sev_inject_launch_secret(const char *hdr, const char *secret,
-                             uint64_t gpa, Error **errp);
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
 void sev_es_set_reset_vector(CPUState *cpu);
-- 
2.40.1

[RFC PATCH 8/8] i386/sev: Replace LAUNCH_FINISH ioctl with sev library equivalent

[Tyler Fanelli]

The LAUNCH_FINISH ioctl finishes the guest launch flow and transitions
the guest into a state ready to be run.

If this API ioctl call fails, fw_error will be set accordingly.

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
---
 target/i386/sev.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index a4510b5437..e52dcc67c3 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -785,35 +785,29 @@ static Notifier sev_machine_done_notify = {
     .notify = sev_launch_get_measure,
 };
 
-static void
-sev_launch_finish(SevGuestState *sev)
-{
-    int ret, error;
-
-    trace_kvm_sev_launch_finish();
-    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
-    if (ret) {
-        error_report("%s: 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");
-    migrate_add_blocker(sev_mig_blocker, &error_fatal);
-}
-
 static void
 sev_vm_state_change(void *opaque, bool running, RunState state)
 {
     SevGuestState *sev = opaque;
+    int ret, fw_error;
+    KVMState *s = kvm_state;
 
     if (running) {
         if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
-            sev_launch_finish(sev);
+            trace_kvm_sev_launch_finish();
+            ret = sev_launch_finish(s->vmfd, &fw_error);
+            if (ret) {
+                error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
+                             __func__, ret, fw_error,
+                             fw_error_to_str(fw_error));
+                exit(1);
+            }
+
+            sev_set_guest_state(sev, SEV_STATE_RUNNING);
+
+            // add migration blocker.
+            error_setg(&sev_mig_blocker, "SEV: Migration is not implemented");
+            migrate_add_blocker(sev_mig_blocker, &error_fatal);
         }
     }
 }
-- 
2.40.1

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Philippe Mathieu-Daudé]

Hi Tyler,

On 14/9/23 19:58, Tyler Fanelli wrote:
> These patches are submitted as an RFC mainly because I'm a relative
> newcomer to QEMU with no knowledge of the community's views on
> including Rust code, nor it's preference of using library APIs for
> ioctls that were previously implemented in QEMU directly.
> 
> Recently, the Rust sev library [0] has introduced a C API to take
> advantage of the library outside of Rust.
> 
> Should the inclusion of the library as a dependency be desired, it can
> be extended further to include the firmware/platform ioctls, the
> attestation report fetching, and more. This would result in much of
> the AMD-SEV portion of QEMU being offloaded to the library.
> 
> This series looks to explore the possibility of using the library and
> show a bit of what it would look like. I'm looking for comments
> regarding if this feature is desired.
> 
> [0] https://github.com/virtee/sev
> 
> Tyler Fanelli (8):
>    Add SEV Rust library as dependency with CONFIG_SEV
>    i386/sev: Replace INIT and ES_INIT ioctls with sev library equivalents
>    i386/sev: Replace LAUNCH_START ioctl with sev library equivalent
>    i386/sev: Replace UPDATE_DATA ioctl with sev library equivalent
>    i386/sev: Replace LAUNCH_UPDATE_VMSA ioctl with sev library equivalent
>    i386/sev: Replace LAUNCH_MEASURE ioctl with sev library equivalent
>    i386/sev: Replace LAUNCH_SECRET ioctl with sev library equivalent
>    i386/sev: Replace LAUNCH_FINISH ioctl with sev library equivalent

There is still one ioctl use, GET_ATTESTATION_REPORT. No libsev
equivalent for this one yet?

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Tyler Fanelli]

On 9/14/23 3:04 PM, Philippe Mathieu-Daudé wrote:
> Hi Tyler,
>
> On 14/9/23 19:58, Tyler Fanelli wrote:
>> These patches are submitted as an RFC mainly because I'm a relative
>> newcomer to QEMU with no knowledge of the community's views on
>> including Rust code, nor it's preference of using library APIs for
>> ioctls that were previously implemented in QEMU directly.
>>
>> Recently, the Rust sev library [0] has introduced a C API to take
>> advantage of the library outside of Rust.
>>
>> Should the inclusion of the library as a dependency be desired, it can
>> be extended further to include the firmware/platform ioctls, the
>> attestation report fetching, and more. This would result in much of
>> the AMD-SEV portion of QEMU being offloaded to the library.
>>
>> This series looks to explore the possibility of using the library and
>> show a bit of what it would look like. I'm looking for comments
>> regarding if this feature is desired.
>>
>> [0] https://github.com/virtee/sev
>>
>> Tyler Fanelli (8):
>>    Add SEV Rust library as dependency with CONFIG_SEV
>>    i386/sev: Replace INIT and ES_INIT ioctls with sev library 
>> equivalents
>>    i386/sev: Replace LAUNCH_START ioctl with sev library equivalent
>>    i386/sev: Replace UPDATE_DATA ioctl with sev library equivalent
>>    i386/sev: Replace LAUNCH_UPDATE_VMSA ioctl with sev library 
>> equivalent
>>    i386/sev: Replace LAUNCH_MEASURE ioctl with sev library equivalent
>>    i386/sev: Replace LAUNCH_SECRET ioctl with sev library equivalent
>>    i386/sev: Replace LAUNCH_FINISH ioctl with sev library equivalent
>
> There is still one ioctl use, GET_ATTESTATION_REPORT. No libsev
> equivalent for this one yet?
>
There is an equivalent, however the machine that I'm using currently 
hangs when trying to fetch an attestation report (not a libsev issue, as 
it hangs when I try with latest qemu release as well). When I can either 
update its firmware or get access to another SEV machine, I can test and 
confirm it behaves as intended with the libsev API. Once this is done, I 
can add that API to the patch series.


Tyler

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Daniel P. Berrangé]

On Thu, Sep 14, 2023 at 01:58:27PM -0400, Tyler Fanelli wrote:
> These patches are submitted as an RFC mainly because I'm a relative
> newcomer to QEMU with no knowledge of the community's views on
> including Rust code, nor it's preference of using library APIs for
> ioctls that were previously implemented in QEMU directly.

We've talked about Rust alot, but thus far most focus has been on
areas peripheral to QEMU. Projects that might have been part of
QEMU in the past, and now being done as separate efforts, and
have bene taking advantage of Rust. eg virtiofsd Rust replacing
QEMU's in -tree C impl. eg passt providing an alternative to
slirp. eg the dbus display in QEMU allowing a remote display
frontend to be provided, written in rust. eg libblkio providing
a block backend in Rust.

The libblkio work is likely closest to what you've proposed
here, in that it is a Rust create exposed as a C shared library
for apps to consume. In theory apps don't need to care that it
is written in Rust, as it is opaque.

The one key difference though is that it was not replacing
existing functionality, it was adding a new feature. So users
who didn't have libblkio or whom want to avoid Rust dependancies
didn't loose anything they were already using.

If we use the libsev.so we create a hard dependancy on the Rust
sev crate, otherwise users loose the SEV feature in QEMU. Right
now the sev crate C library is not present in *any* distro that
I can see.

If we treat 'sev' as just another opaque 3rd party library to be
provided by the distro, this creates a problem. Our support
policy is that we usually won't drop features in existing distros,
but that is what would happen if we applied this patchset today.
We did bend that rule slightly with virtiofsd, but that was already
a separate binary and we followed our deprecation path before
deleting it, giving distros time to adapt.


If we rollback the curtain, however, and decide to expose Rust
directly to QEMU we could address this problem. We could bundle
the dependant Rust crates directly with QEMU tarballs, and
generate the FFI C library as part of QEMU build and static
link the library. Distros would not have todo anything, though
they could have the choice of dyn linking if they really wanted
to.

If we directly exposed the notion of Rust to QEMU, then we are
also not limited by whether a Rust crate provides a C FFI itself.
QEMU could provide C FFI glue for any Rust crate it sees as
useful to its code.

This all forces us, however, to have the difficult discussion
about whether we're willing to make Rust a mandatory dependancy
of QEMU and permit (or even welcome) its use /anywhere/ in the
QEMU tree that looks relevant.

We've already queried whether Rust will actually benefit the
core QEMU codebase, or whether we'll end up punching too many
holes in its safety net to make it worthwhile. My opinion is
that we probably shouldn't obsess over that as I think it is
hard to predict the future, it has a habit of surprising us.
Your patch series here doesn't demonstrate an obvious safety
benefit, since we have existing working code and that code is
not especially complex. Once we open the doors to Rust code
in QEMU though, we will probably surprise ourselves with the
range of benefits we'll see 2, 3, 5 years down the road.

IOW, we shouldn't judge future benefits based on this patch
series. It is great that this series is actually quite simple,
because it lets us focus on how we might integrate Rust more
directly into QEMU, without worrying much about the actual
code being replaced.

> This series looks to explore the possibility of using the library and
> show a bit of what it would look like. I'm looking for comments
> regarding if this feature is desired.

My summary, is that I'd personally be in favour of opening the door
to Rust code as a mandatory pre-requisite for QEMU, at the very least
for system emulators. Not because this particular series is compelling,
but because I think Rust could be more beneficial to QEMU over the long
term than we expect. In terms of consuming it though, if we're going
to replace existing QEMU functionality, then I think we need to bundle
the Rust code and natively integrate it into the build system, as we
have recently started doing with our python deps, to detach ourselves
from the limits of what distros ship.

With 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 :|

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Stefan Hajnoczi]

On Fri, 15 Sept 2023 at 05:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Sep 14, 2023 at 01:58:27PM -0400, Tyler Fanelli wrote:
> > These patches are submitted as an RFC mainly because I'm a relative
> > newcomer to QEMU with no knowledge of the community's views on
> > including Rust code, nor it's preference of using library APIs for
> > ioctls that were previously implemented in QEMU directly.
>
> We've talked about Rust alot, but thus far most focus has been on
> areas peripheral to QEMU. Projects that might have been part of
> QEMU in the past, and now being done as separate efforts, and
> have bene taking advantage of Rust. eg virtiofsd Rust replacing
> QEMU's in -tree C impl. eg passt providing an alternative to
> slirp. eg the dbus display in QEMU allowing a remote display
> frontend to be provided, written in rust. eg libblkio providing
> a block backend in Rust.
>
> The libblkio work is likely closest to what you've proposed
> here, in that it is a Rust create exposed as a C shared library
> for apps to consume. In theory apps don't need to care that it
> is written in Rust, as it is opaque.
>
> The one key difference though is that it was not replacing
> existing functionality, it was adding a new feature. So users
> who didn't have libblkio or whom want to avoid Rust dependancies
> didn't loose anything they were already using.
>
> If we use the libsev.so we create a hard dependancy on the Rust
> sev crate, otherwise users loose the SEV feature in QEMU. Right
> now the sev crate C library is not present in *any* distro that
> I can see.
>
> If we treat 'sev' as just another opaque 3rd party library to be
> provided by the distro, this creates a problem. Our support
> policy is that we usually won't drop features in existing distros,
> but that is what would happen if we applied this patchset today.
> We did bend that rule slightly with virtiofsd, but that was already
> a separate binary and we followed our deprecation path before
> deleting it, giving distros time to adapt.
>
>
> If we rollback the curtain, however, and decide to expose Rust
> directly to QEMU we could address this problem. We could bundle
> the dependant Rust crates directly with QEMU tarballs, and
> generate the FFI C library as part of QEMU build and static
> link the library. Distros would not have todo anything, though
> they could have the choice of dyn linking if they really wanted
> to.
>
> If we directly exposed the notion of Rust to QEMU, then we are
> also not limited by whether a Rust crate provides a C FFI itself.
> QEMU could provide C FFI glue for any Rust crate it sees as
> useful to its code.
>
> This all forces us, however, to have the difficult discussion
> about whether we're willing to make Rust a mandatory dependancy
> of QEMU and permit (or even welcome) its use /anywhere/ in the
> QEMU tree that looks relevant.
>
> We've already queried whether Rust will actually benefit the
> core QEMU codebase, or whether we'll end up punching too many
> holes in its safety net to make it worthwhile. My opinion is
> that we probably shouldn't obsess over that as I think it is
> hard to predict the future, it has a habit of surprising us.
> Your patch series here doesn't demonstrate an obvious safety
> benefit, since we have existing working code and that code is
> not especially complex. Once we open the doors to Rust code
> in QEMU though, we will probably surprise ourselves with the
> range of benefits we'll see 2, 3, 5 years down the road.
>
> IOW, we shouldn't judge future benefits based on this patch
> series. It is great that this series is actually quite simple,
> because it lets us focus on how we might integrate Rust more
> directly into QEMU, without worrying much about the actual
> code being replaced.
>
> > This series looks to explore the possibility of using the library and
> > show a bit of what it would look like. I'm looking for comments
> > regarding if this feature is desired.
>
> My summary, is that I'd personally be in favour of opening the door
> to Rust code as a mandatory pre-requisite for QEMU, at the very least
> for system emulators. Not because this particular series is compelling,
> but because I think Rust could be more beneficial to QEMU over the long
> term than we expect. In terms of consuming it though, if we're going
> to replace existing QEMU functionality, then I think we need to bundle
> the Rust code and natively integrate it into the build system, as we
> have recently started doing with our python deps, to detach ourselves
> from the limits of what distros ship.

I support using Rust directly within QEMU.

David Gibson looked at Rust's operating system and CPU architecture
coverage a few years ago:
https://wiki.qemu.org/RustInQemu

Please update that support matrix to check that depending on Rust in
core QEMU code really works everywhere where QEMU is supported today.
This is probably just a formality at this stage since Rust has become
widely used over the past few years.

The library approach worked well for libblkio but the overhead of
creating a separate shared library and shipping it is significant.
When QEMU is the only user of some code, then it should definitely be
part of QEMU. Also, when QEMU needs early access to code that isn't
widely available yet, then bundling it inside QEMU until packages are
available also seems reasonable to me (I think we already do that for
libvfio-user and maybe other libraries).

I would prefer it if we minimize Rust wrappers for C APIs and instead
focus on using Rust to build new subsystems. Writing and maintaing two
sets of the same API is expensive and I hope we don't get bogged down
keeping C and Rust APIs in sync. That said, I think there's an
argument for wrapping core QEMU APIs needed for device emulation (e.g.
DeviceState, PCIDevice) because of the security benefits of writing
new device emulation code in Rust.

Stefan

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Daniel P. Berrangé]

On Fri, Sep 15, 2023 at 07:33:32AM -0400, Stefan Hajnoczi wrote:
> The library approach worked well for libblkio but the overhead of
> creating a separate shared library and shipping it is significant.
> When QEMU is the only user of some code, then it should definitely be
> part of QEMU. Also, when QEMU needs early access to code that isn't
> widely available yet, then bundling it inside QEMU until packages are
> available also seems reasonable to me (I think we already do that for
> libvfio-user and maybe other libraries).

Yep, avoiding the public shared library significantly cuts down the
maint burden, as you can freely adapt the exposed C  FFI API to
suit QEMU's needs and not worry about ABI compatibility.

> I would prefer it if we minimize Rust wrappers for C APIs and instead
> focus on using Rust to build new subsystems. Writing and maintaing two
> sets of the same API is expensive and I hope we don't get bogged down
> keeping C and Rust APIs in sync. That said, I think there's an
> argument for wrapping core QEMU APIs needed for device emulation (e.g.
> DeviceState, PCIDevice) because of the security benefits of writing
> new device emulation code in Rust.


With 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 :|

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Peter Maydell]

On Fri, 15 Sept 2023 at 10:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
> My summary, is that I'd personally be in favour of opening the door
> to Rust code as a mandatory pre-requisite for QEMU, at the very least
> for system emulators. Not because this particular series is compelling,
> but because I think Rust could be more beneficial to QEMU over the long
> term than we expect. In terms of consuming it though, if we're going
> to replace existing QEMU functionality, then I think we need to bundle
> the Rust code and natively integrate it into the build system, as we
> have recently started doing with our python deps, to detach ourselves
> from the limits of what distros ship.

I'm not against this, but there is a fair amount of work here
in figuring out how exactly to integrate Rust components
into the build system, questions like what our minimum required
rust version would be, liasing with downstream distros to
check that what we're proposing isn't a nightmare for them
to package, etc.

-- PMM

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Tyler Fanelli]

On 9/15/23 7:33 AM, Stefan Hajnoczi wrote:
> On Fri, 15 Sept 2023 at 05:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> On Thu, Sep 14, 2023 at 01:58:27PM -0400, Tyler Fanelli wrote:
>>> These patches are submitted as an RFC mainly because I'm a relative
>>> newcomer to QEMU with no knowledge of the community's views on
>>> including Rust code, nor it's preference of using library APIs for
>>> ioctls that were previously implemented in QEMU directly.
>> We've talked about Rust alot, but thus far most focus has been on
>> areas peripheral to QEMU. Projects that might have been part of
>> QEMU in the past, and now being done as separate efforts, and
>> have bene taking advantage of Rust. eg virtiofsd Rust replacing
>> QEMU's in -tree C impl. eg passt providing an alternative to
>> slirp. eg the dbus display in QEMU allowing a remote display
>> frontend to be provided, written in rust. eg libblkio providing
>> a block backend in Rust.
>>
>> The libblkio work is likely closest to what you've proposed
>> here, in that it is a Rust create exposed as a C shared library
>> for apps to consume. In theory apps don't need to care that it
>> is written in Rust, as it is opaque.
>>
>> The one key difference though is that it was not replacing
>> existing functionality, it was adding a new feature. So users
>> who didn't have libblkio or whom want to avoid Rust dependancies
>> didn't loose anything they were already using.
>>
>> If we use the libsev.so we create a hard dependancy on the Rust
>> sev crate, otherwise users loose the SEV feature in QEMU. Right
>> now the sev crate C library is not present in *any* distro that
>> I can see.

Yes, the C API is very new and not packaged in any distro at the moment.

>>
>> If we treat 'sev' as just another opaque 3rd party library to be
>> provided by the distro, this creates a problem. Our support
>> policy is that we usually won't drop features in existing distros,
>> but that is what would happen if we applied this patchset today.
>> We did bend that rule slightly with virtiofsd, but that was already
>> a separate binary and we followed our deprecation path before
>> deleting it, giving distros time to adapt.
>>
>>
>> If we rollback the curtain, however, and decide to expose Rust
>> directly to QEMU we could address this problem. We could bundle
>> the dependant Rust crates directly with QEMU tarballs, and
>> generate the FFI C library as part of QEMU build and static
>> link the library. Distros would not have todo anything, though
>> they could have the choice of dyn linking if they really wanted
>> to.
>>
>> If we directly exposed the notion of Rust to QEMU, then we are
>> also not limited by whether a Rust crate provides a C FFI itself.
>> QEMU could provide C FFI glue for any Rust crate it sees as
>> useful to its code.
>>
>> This all forces us, however, to have the difficult discussion
>> about whether we're willing to make Rust a mandatory dependancy
>> of QEMU and permit (or even welcome) its use /anywhere/ in the
>> QEMU tree that looks relevant.
>>
>> We've already queried whether Rust will actually benefit the
>> core QEMU codebase, or whether we'll end up punching too many
>> holes in its safety net to make it worthwhile. My opinion is
>> that we probably shouldn't obsess over that as I think it is
>> hard to predict the future, it has a habit of surprising us.
>> Your patch series here doesn't demonstrate an obvious safety
>> benefit, since we have existing working code and that code is
>> not especially complex.

Correct, there isn't any new features being added here. SEV on QEMU 
should work _exactly_ how it did before these patches.

>> Once we open the doors to Rust code
>> in QEMU though, we will probably surprise ourselves with the
>> range of benefits we'll see 2, 3, 5 years down the road.
>>
>> IOW, we shouldn't judge future benefits based on this patch
>> series. It is great that this series is actually quite simple,
>> because it lets us focus on how we might integrate Rust more
>> directly into QEMU, without worrying much about the actual
>> code being replaced.

It also shows that much of Rust's security benefits in QEMU would depend 
on Rust being integrated more directly, rather than just using C FFI. 
Most of the code in the libsev C API is unsafe Rust anyway (it must be 
in order to interact with C). Therefore there is not much of an added 
security benefit here. However, if Rust can be further expanded in QEMU, 
much of the unsafe bits can be removed entirely (i.e. by bypassing the C 
API).

>>
>>> This series looks to explore the possibility of using the library and
>>> show a bit of what it would look like. I'm looking for comments
>>> regarding if this feature is desired.
>> My summary, is that I'd personally be in favour of opening the door
>> to Rust code as a mandatory pre-requisite for QEMU, at the very least
>> for system emulators. Not because this particular series is compelling,
>> but because I think Rust could be more beneficial to QEMU over the long
>> term than we expect. In terms of consuming it though, if we're going
>> to replace existing QEMU functionality, then I think we need to bundle
>> the Rust code and natively integrate it into the build system, as we
>> have recently started doing with our python deps, to detach ourselves
>> from the limits of what distros ship.
> I support using Rust directly within QEMU.
>
> David Gibson looked at Rust's operating system and CPU architecture
> coverage a few years ago:
> https://wiki.qemu.org/RustInQemu
>
> Please update that support matrix to check that depending on Rust in
> core QEMU code really works everywhere where QEMU is supported today.
> This is probably just a formality at this stage since Rust has become
> widely used over the past few years.
>
> The library approach worked well for libblkio but the overhead of
> creating a separate shared library and shipping it is significant.
> When QEMU is the only user of some code, then it should definitely be
> part of QEMU. Also, when QEMU needs early access to code that isn't
> widely available yet, then bundling it inside QEMU until packages are
> available also seems reasonable to me (I think we already do that for
> libvfio-user and maybe other libraries).
>
> I would prefer it if we minimize Rust wrappers for C APIs and instead
> focus on using Rust to build new subsystems. Writing and maintaing two
> sets of the same API is expensive and I hope we don't get bogged down
> keeping C and Rust APIs in sync.

It would also allow QEMU to take advantage of other Rust crates without 
having to add some type of shim layer to facilitate the interaction.


Tyler

> That said, I think there's an
> argument for wrapping core QEMU APIs needed for device emulation (e.g.
> DeviceState, PCIDevice) because of the security benefits of writing
> new device emulation code in Rust.
>
> Stefan
>

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Stefan Hajnoczi]

On Fri, 15 Sept 2023 at 09:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 15 Sept 2023 at 10:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > My summary, is that I'd personally be in favour of opening the door
> > to Rust code as a mandatory pre-requisite for QEMU, at the very least
> > for system emulators. Not because this particular series is compelling,
> > but because I think Rust could be more beneficial to QEMU over the long
> > term than we expect. In terms of consuming it though, if we're going
> > to replace existing QEMU functionality, then I think we need to bundle
> > the Rust code and natively integrate it into the build system, as we
> > have recently started doing with our python deps, to detach ourselves
> > from the limits of what distros ship.
>
> I'm not against this, but there is a fair amount of work here
> in figuring out how exactly to integrate Rust components
> into the build system, questions like what our minimum required
> rust version would be, liasing with downstream distros to
> check that what we're proposing isn't a nightmare for them
> to package, etc.

Those details are similar to what librsvg2, libblkio, and other
libraries (like the sev crate in this patch series) have had to solve.

libblkio uses meson as the build system and has C tests that cover the
C API. Cargo is still used to build the Rust code. It is possible to
integrate the two and I think QEMU could take that approach. It's a
little ugly to glue together the two build systems, but it has been
shown to work.

Finding the minimum Rust version across QEMU's support matrix is doable.

Stefan

Re: [RFC PATCH 0/8] i386/sev: Use C API of Rust SEV library

[Marc-André Lureau]

Hi

On Fri, Sep 15, 2023 at 9:10 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Fri, 15 Sept 2023 at 09:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 15 Sept 2023 at 10:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > My summary, is that I'd personally be in favour of opening the door
> > > to Rust code as a mandatory pre-requisite for QEMU, at the very least
> > > for system emulators. Not because this particular series is compelling,
> > > but because I think Rust could be more beneficial to QEMU over the long
> > > term than we expect. In terms of consuming it though, if we're going
> > > to replace existing QEMU functionality, then I think we need to bundle
> > > the Rust code and natively integrate it into the build system, as we
> > > have recently started doing with our python deps, to detach ourselves
> > > from the limits of what distros ship.
> >
> > I'm not against this, but there is a fair amount of work here
> > in figuring out how exactly to integrate Rust components
> > into the build system, questions like what our minimum required
> > rust version would be, liasing with downstream distros to
> > check that what we're proposing isn't a nightmare for them
> > to package, etc.
>
> Those details are similar to what librsvg2, libblkio, and other
> libraries (like the sev crate in this patch series) have had to solve.
>
> libblkio uses meson as the build system and has C tests that cover the
> C API. Cargo is still used to build the Rust code. It is possible to
> integrate the two and I think QEMU could take that approach. It's a
> little ugly to glue together the two build systems, but it has been
> shown to work.

That's also what I did ~2y ago, when I added QAPI rust generator:
https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/

But meson has learned to support rust better during that time, and
there is some upcoming cargo support in 1.3
(https://github.com/mesonbuild/meson/pull/11856).

(I don't know the details)

-- 
Marc-André Lureau