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
