diff mbox series

[v2,01/10] meson: Add optional dependency on IGVM library

Message ID 0da76c3956bf776a9bbb0e18a1813b8dc5e20bf8.1712141833.git.roy.hopkins@suse.com (mailing list archive)
State New, archived
Headers show
Series [v2,01/10] meson: Add optional dependency on IGVM library | expand

Commit Message

Roy Hopkins April 3, 2024, 11:11 a.m. UTC
The IGVM library allows Independent Guest Virtual Machine files to be
parsed and processed. IGVM files are used to configure guest memory
layout, initial processor state and other configuration pertaining to
secure virtual machines.

This adds the --enable-igvm configure option, enabled by default, which
attempts to locate and link against the IGVM library via pkgconfig and
sets CONFIG_IGVM if found.

The library is added to the system_ss target in backends/meson.build
where the IGVM parsing will be performed by the ConfidentialGuestSupport
object.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 backends/meson.build          | 3 +++
 meson.build                   | 8 ++++++++
 meson_options.txt             | 2 ++
 scripts/meson-buildoptions.sh | 3 +++
 4 files changed, 16 insertions(+)

Comments

Daniel P. Berrangé April 16, 2024, 2:13 p.m. UTC | #1
On Wed, Apr 03, 2024 at 12:11:32PM +0100, Roy Hopkins wrote:
> The IGVM library allows Independent Guest Virtual Machine files to be
> parsed and processed. IGVM files are used to configure guest memory
> layout, initial processor state and other configuration pertaining to
> secure virtual machines.

Looking at the generated header file for the IGVM library, I see
some quite bad namespace pollution. eg igvm_defs.h has:

typedef uint64_t u64_le;
typedef uint32_t u32_le;

#define UINT32_FLAGS_VALUE(x) *((uint32_t*)&(x))

#define MAKE_INVALID_IMPL(x, y) x##y
#define MAKE_INVALID(x, y) MAKE_INVALID_IMPL(x, y)
#define INVALID MAKE_INVALID(INVALID_, __COUNTER__)

enum IgvmPageDataType {
  NORMAL = 0,
  SECRETS = 1,
  CPUID_DATA = 2,
  CPUID_XF = 3,
};

enum IgvmPlatformType {
  NATIVE = 0,
  VSM_ISOLATION = 1,
  SEV_SNP = 2,
  TDX = 3,
};



enum IgvmVariableHeaderType {
  INVALID = 0,
  ...
}

There are soo many more examples in igvm_defs.h that I won't
list them all here.


These are all way too generic as names to be exposing in library
header file. We may be lucky right now that these definitions
don't clash with anything else defined in the compilation namespace
of the consuming application, but that's a bad bet to make long
term.

IMHO this really needs fixing before there's any use of this igvm
library, since fixing it will be a backwards-incompatible change.

Essentially everything in the header needs to have an 'IGVM/Igvm/igvm'
prefix on it.

With regards,
Daniel
Roy Hopkins May 1, 2024, 8:53 a.m. UTC | #2
On Tue, 2024-04-16 at 15:13 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 03, 2024 at 12:11:32PM +0100, Roy Hopkins wrote:
> > The IGVM library allows Independent Guest Virtual Machine files to be
> > parsed and processed. IGVM files are used to configure guest memory
> > layout, initial processor state and other configuration pertaining to
> > secure virtual machines.
> 
> Looking at the generated header file for the IGVM library, I see
> some quite bad namespace pollution. eg igvm_defs.h has:
> 
> typedef uint64_t u64_le;
> typedef uint32_t u32_le;
> 
> #define UINT32_FLAGS_VALUE(x) *((uint32_t*)&(x))
> 
> #define MAKE_INVALID_IMPL(x, y) x##y
> #define MAKE_INVALID(x, y) MAKE_INVALID_IMPL(x, y)
> #define INVALID MAKE_INVALID(INVALID_, __COUNTER__)
> 
> enum IgvmPageDataType {
>   NORMAL = 0,
>   SECRETS = 1,
>   CPUID_DATA = 2,
>   CPUID_XF = 3,
> };
> 
> enum IgvmPlatformType {
>   NATIVE = 0,
>   VSM_ISOLATION = 1,
>   SEV_SNP = 2,
>   TDX = 3,
> };
> 
> 
> 
> enum IgvmVariableHeaderType {
>   INVALID = 0,
>   ...
> }
> 
> There are soo many more examples in igvm_defs.h that I won't
> list them all here.
> 
> 
> These are all way too generic as names to be exposing in library
> header file. We may be lucky right now that these definitions
> don't clash with anything else defined in the compilation namespace
> of the consuming application, but that's a bad bet to make long
> term.
> 
> IMHO this really needs fixing before there's any use of this igvm
> library, since fixing it will be a backwards-incompatible change.
> 
> Essentially everything in the header needs to have an 'IGVM/Igvm/igvm'
> prefix on it.
> 
> With regards,
> Daniel

Daniel,

I've just submitted a patch for the IGVM C library to fix this, ensuring
everything is now uniquely identified with an IGVM_/Igvm_ prefix and fixing some
of the other issues in the generated header file:
https://github.com/microsoft/igvm/pull/52

I'll update this with the changes once merged.

Thanks,
Roy
diff mbox series

Patch

diff --git a/backends/meson.build b/backends/meson.build
index 8b2b111497..d550ac19f7 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -30,5 +30,8 @@  if have_vhost_user_crypto
 endif
 system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
 system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
+if igvm.found()
+  system_ss.add(igvm)
+endif
 
 subdir('tpm')
diff --git a/meson.build b/meson.build
index c9c3217ba4..f0b5a29ce7 100644
--- a/meson.build
+++ b/meson.build
@@ -1232,6 +1232,12 @@  if host_os == 'linux' and (have_system or have_tools)
                        method: 'pkg-config',
                        required: get_option('libudev'))
 endif
+igvm = not_found
+if not get_option('igvm').auto() or have_system
+  igvm = dependency('igvm',
+                       method: 'pkg-config',
+                       required: get_option('igvm'))
+endif
 
 mpathlibs = [libudev]
 mpathpersist = not_found
@@ -2320,6 +2326,7 @@  config_host_data.set('CONFIG_CFI', get_option('cfi'))
 config_host_data.set('CONFIG_SELINUX', selinux.found())
 config_host_data.set('CONFIG_XEN_BACKEND', xen.found())
 config_host_data.set('CONFIG_LIBDW', libdw.found())
+config_host_data.set('CONFIG_IGVM', igvm.found())
 if xen.found()
   # protect from xen.version() having less than three components
   xen_version = xen.version().split('.') + ['0', '0']
@@ -4456,6 +4463,7 @@  summary_info += {'seccomp support':   seccomp}
 summary_info += {'GlusterFS support': glusterfs}
 summary_info += {'hv-balloon support': hv_balloon}
 summary_info += {'TPM support':       have_tpm}
+summary_info += {'IGVM support':      igvm}
 summary_info += {'libssh support':    libssh}
 summary_info += {'lzo support':       lzo}
 summary_info += {'snappy support':    snappy}
diff --git a/meson_options.txt b/meson_options.txt
index 0a99a059ec..4eaba64f4b 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -109,6 +109,8 @@  option('dbus_display', type: 'feature', value: 'auto',
        description: '-display dbus support')
 option('tpm', type : 'feature', value : 'auto',
        description: 'TPM support')
+option('igvm', type: 'feature', value: 'auto',
+       description: 'Independent Guest Virtual Machine (IGVM) file support')
 
 # Do not enable it by default even for Mingw32, because it doesn't
 # work on Wine.
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 680fa3f581..38a8183625 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -126,6 +126,7 @@  meson_options_help() {
   printf "%s\n" '  hv-balloon      hv-balloon driver (requires Glib 2.68+ GTree API)'
   printf "%s\n" '  hvf             HVF acceleration support'
   printf "%s\n" '  iconv           Font glyph conversion support'
+  printf "%s\n" '  igvm            IGVM file support'
   printf "%s\n" '  jack            JACK sound support'
   printf "%s\n" '  keyring         Linux keyring support'
   printf "%s\n" '  kvm             KVM acceleration support'
@@ -342,6 +343,8 @@  _meson_option_parse() {
     --iasl=*) quote_sh "-Diasl=$2" ;;
     --enable-iconv) printf "%s" -Diconv=enabled ;;
     --disable-iconv) printf "%s" -Diconv=disabled ;;
+    --enable-igvm) printf "%s" -Digvm=enabled ;;
+    --disable-igvm) printf "%s" -Digvm=disabled ;;
     --includedir=*) quote_sh "-Dincludedir=$2" ;;
     --enable-install-blobs) printf "%s" -Dinstall_blobs=true ;;
     --disable-install-blobs) printf "%s" -Dinstall_blobs=false ;;