Re: [RFC PATCH v2 1/9] Add Rust SEV library as subproject

[Daniel P . Berrangé]

On Wed, Oct 04, 2023 at 04:34:10PM -0400, Tyler Fanelli wrote:
> The Rust sev library provides a C API for the AMD SEV launch ioctls, as
> well as the ability to build with meson. Add the Rust sev library as a
> QEMU subproject with the goal of outsourcing all SEV launch ioctls to C
> APIs provided by it.
> 
> Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
> ---
>  meson.build                   | 8 ++++++++
>  meson_options.txt             | 2 ++
>  scripts/meson-buildoptions.sh | 3 +++
>  subprojects/sev.wrap          | 6 ++++++
>  target/i386/meson.build       | 2 +-
>  5 files changed, 20 insertions(+), 1 deletion(-)
>  create mode 100644 subprojects/sev.wrap
> 
> diff --git a/meson.build b/meson.build
> index 20ceeb8158..8a17c29de8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -960,6 +960,13 @@ if not get_option('slirp').auto() or have_system
>    endif
>  endif
>  
> +sev = not_found
> +if not get_option('sev').auto()
> +  sev = dependency('sev',
> +                   method: 'pkg-config',
> +                   required: get_option('sev'))
> +endif
> +

I've revisited this series and tested it now. As Stefan already
mentioned, this logic is flawed.

Currently QEMU is self-contained for SEV support. If we swap to the
sev crate, then we introduce libsev.so as a build time system library
dependancy, and it is highly unlikely that many existing distros will
add the package. IOW we'll cause a regression for users.

Thus we need to be able to *statically* link to the sev crate when it
is not available in the system.

I had a crack at changing this patch to support that and came up with
this diff on top of your patch here:

diff --git a/meson.build b/meson.build
index 1beb9e9f40..d6aba3fd7d 100644
--- a/meson.build
+++ b/meson.build
@@ -1116,12 +1116,6 @@ if not get_option('slirp').auto() or have_system
   endif
 endif
 
-sev = not_found
-if not get_option('sev').auto()
-  sev = dependency('sev',
-                   method: 'pkg-config',
-                   required: get_option('sev'))
-endif
 
 vde = not_found
 if not get_option('vde').auto() or have_system or have_tools
@@ -3003,6 +2997,7 @@ ignored = [ 'TARGET_XML_FILES', 'TARGET_ABI_DIR', 
'TARGET_ARCH' ]
 default_targets = 'CONFIG_DEFAULT_TARGETS' in config_host
 actual_target_dirs = []
 fdt_required = []
+sev_required = []
 foreach target : target_dirs
   config_target = { 'TARGET_NAME': target.split('-')[0] }
   if target.endswith('linux-user')
@@ -3124,6 +3119,9 @@ foreach target : target_dirs
     foreach k, v: config_devices
       config_devices_data.set(k, 1)
     endforeach
+    if 'CONFIG_SEV' in config_devices
+        sev_required += target
+    endif
     config_devices_mak_list += config_devices_mak
     config_devices_h += {target: configure_file(output: target + 
'-config-devices.h',
                                                 configuration: 
config_devices_data)}
@@ -3206,6 +3204,39 @@ if have_libvduse
   libvduse = libvduse_proj.get_variable('libvduse_dep')
 endif
 
+sev = not_found
+sev_opt = get_option('sev')
+if sev_required.length() > 0 or sev_opt == 'enabled'
+  if sev_opt == 'disabled'
+    error('sev disabled but required by targets ' + ', '.join(fdt_required))
+  endif
+
+  if sev_opt in ['enabled', 'auto', 'system']
+      if get_option('wrap_mode') == 'nodownload'
+          sev_opt = 'system'
+      endif
+      sev = dependency('sev',
+                       method: 'pkg-config',
+                       required: sev_opt == 'system')
+      if sev.found()
+          sev_opt = 'system'
+      elif sev_opt == 'system'
+          error('system libsev requested')
+      else
+          sev_opt = 'internal'
+          sev = not_found
+      endif
+  endif
+  if not sev.found()
+      assert(sev_opt == 'internal')
+      libsev_proj = subproject('sev', required: true,
+                               default_options: ['default_library=static'])
+      sev = libsev_proj.get_variable('sev_dep')
+  endif
+else
+  sev_opt = 'disabled'
+endif
+
 #####################
 # Generated sources #
 #####################
@@ -4453,7 +4484,7 @@ summary_info += {'libudev':           libudev}
 # Dummy dependency, keep .found()
 summary_info += {'FUSE lseek':        fuse_lseek.found()}
 summary_info += {'selinux':           selinux}
-summary_info += {'sev':               sev}
+summary_info += {'sev support':       sev_opt == 'disabled' ? false : sev_opt}
 summary_info += {'libdw':             libdw}
 if host_os == 'freebsd'
   summary_info += {'libinotify-kqueue': inotify}
diff --git a/meson_options.txt b/meson_options.txt
index 749fc87fd7..405d1abfd4 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -208,8 +208,6 @@ option('sdl_image', type : 'feature', value : 'auto',
        description: 'SDL Image support for icons')
 option('seccomp', type : 'feature', value : 'auto',
        description: 'seccomp support')
-option('sev', type : 'feature', value : 'auto',
-        description: 'Rust AMD SEV library')
 option('smartcard', type : 'feature', value : 'auto',
        description: 'CA smartcard emulation support')
 option('snappy', type : 'feature', value : 'auto',
@@ -313,6 +311,9 @@ option('capstone', type: 'feature', value: 'auto',
 option('fdt', type: 'combo', value: 'auto',
        choices: ['disabled', 'enabled', 'auto', 'system', 'internal'],
        description: 'Whether and how to find the libfdt library')
+option('sev', type: 'combo', value: 'auto',
+       choices: ['disabled', 'enabled', 'auto', 'system', 'internal'],
+       description: 'Whether and how to find the libsev library')
 
 option('selinux', type: 'feature', value: 'auto',
        description: 'SELinux support in qemu-nbd')




The caveat is that this does not actually work because the way the
meson rules in the sev crate are written prevents building it as a
static library:

$ cd sev
$ meson build -Ddefault_library=static
The Meson build system
Version: 1.2.3

...snip...

src/meson.build:18:6: ERROR: Cannot link_whole a custom or Rust target 
'libsev.a' into a static library 'sev'. Instead, pass individual object files 
with the "objects:" keyword argument if possible.


The problem is that it is running 'cargo-build.sh' to compile the
SEV crate, which creates a libsev.a static library. It is then
using that as an input to "library('sev',....)" which will also
want to create a new static library with the contents of the first
static library. Never mind that the 2nd library will also be called
libsev.so, meson simply doesn't support linking static libraries
into static libraries AFAICT. There's an open RFE for it that has
no recent attention.


I observe, however, that if cargo-build.sh has already created a
libsev.a static library, then there's no need to tell meson to
create another static library from that. We should be able to use
the first libsev.a directly.

So I tried this change to the 'sev' crate:

diff --git a/meson.build b/meson.build
index 2bf68c3..915ebfb 100644
--- a/meson.build
+++ b/meson.build
@@ -16,5 +16,10 @@ subdir('docs')
 subdir('include')
 subdir('src') # requires: include
 
-sev_dep = declare_dependency(include_directories: inc,
-                             link_with: lib)
+if get_option('default_library') != 'static'
+  sev_dep = declare_dependency(include_directories: inc,
+                               link_with: libsev_so)
+else
+  sev_dep = declare_dependency(include_directories: inc,
+                               link_with: libsev_a)
+endif
diff --git a/src/meson.build b/src/meson.build
index 30bfe49..0632303 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -2,25 +2,27 @@ cargo_build = find_program('cargo-build.sh')
 
 v = meson.project_version().split('.')
 
-staticlib_name = 'libsev.a'
+libsev_a_name = 'libsev.a'
 
-staticlib_target = custom_target(staticlib_name,
+libsev_a = custom_target(libsev_a_name,
   build_by_default : true,
   build_always_stale : true,
   command : [cargo_build, get_option('debug').to_string(),
              get_option('optimization'), meson.current_build_dir() / 'target',
              '@OUTPUT@'],
   console : true,
-  output : [staticlib_name])
+  output : [libsev_a_name])
 
 math = meson.get_compiler('c').find_library('m', required: true)
 
-lib = library('sev',
-  link_whole: staticlib_target,
-  dependencies: [math],
-  install: true,
-  soversion: meson.project_version())
-
+if get_option('default_library') != 'static'
+  libsev_so = shared_library('sev',
+    link_whole: libsev_a,
+    dependencies: [math],
+    install: true,
+    soversion: meson.project_version())
+endif
+  
 # generate pkg-config file
 
 import('pkgconfig').generate(libraries : ['-lsev'],


It is a little bit gross, but it seems to work making it possible to
static link to the sev crate from QEMU with my QEMU patch earlier.


Now, the second issue is that my patch to QEMU's meson.build where
I look for "CONFIG_SEV" is wrong. I've not tested whether it behaves
correctly on non-x86 hosts - basically I'm hoping that CONFIG_SEV is
*NOT* present if building qemu-system-x86_64 on an aarch64 host.

Assuming we get this logic correct though, this unblocks one issue
with getting this merged - Rust platform support.

We've said we want Rust platform support to be a match for QEMU's
platform support. We're probably pretty close, but still it is a
review stumbling block.

If, however, we demonstrate that we /only/ try to use libsev crate
when building on an x86_64 host, then we don't need to think about
Rust platform support in any detail. We know Rust is fully supported
on x86_64 on Linux, and we're not introducing any Rust dependency
for QEMU on other build target arches.

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