Generate a C API for IGVM #3

roy-hopkins · 2023-10-03T16:32:37Z

This is a proposal on how to implement #1 and provide a C API for igvm.

The approach taken is to based on using cbindgen to analyze the rust code and produce header files usable from C. To support this, a set of C friendly functions have been added to igvm/src/c_api.rs which allows, at present, binary files to be parsed and processed.

The additional functions are hidden behind a rust feature named igvm-c. When performing a normal build (without the C API) the only change that has any effect is from the first commit 4da1043 which by necessity relaxes the unsafe_code restriction. Unfortunately this cannot be controlled by the feature.

The sample in igvm_c/sample and the unit test code in igvm/tests are probably the best places to start to see how to use the proposed API from C.

roy-hopkins · 2023-10-03T16:35:11Z

@microsoft-github-policy-service agree company="SUSE"

chris-oo · 2023-10-04T20:19:46Z

Thanks for the RFC! I'll try to find some time to look at this soon.

chris-oo

Broad strokes look fine, but I think we can add some rust methods to remove the need to allocate Vecs that we immediately throw away for a lot of these methods.

igvm/src/c_api.rs

igvm_c/README.md

igvm_c/sample/dump_igvm.c

igvm_c/tests/igvm_test.c

igvm/src/c_api.rs

igvm_c/Makefile

roy-hopkins · 2023-11-02T15:10:36Z

I've addressed the current review comments with the latest push. The most significant change was in the management of the buffers that are returned containing the headers and file data. Previously, the rust code was populating a Vec then copying the result into a C provided, pre-allocated buffer (or returning the size and BUFFER_TOO_SMALL if the buffer was not big enough.

I've now changed this so the Vec created by the rust functions are stored alongside a handle that is created when the file is initially parsed. This means that the rust code owns the buffer and knows the actual size, allowing a lot of unsafe code to be removed. Obviously to support this I have had to add functions to manage the lifetime of these handles and buffers but I think the result is safe and cleaner than the previous version.

igvm/src/lib.rs

igvm/src/c_api.rs

igvm/src/lib.rs

igvm/src/c_api.rs

igvm/src/lib.rs

chris-oo · 2023-11-14T00:32:57Z

I left another round of feedback but I agree that it's much better. I think refactoring to take advantage of Result<(), IgvmApiError> and OnceLock will help clean it up a bit more and remove all the unsafe code blocks. Thanks for doing this!

roy-hopkins · 2023-12-19T15:47:48Z

I've worked through the PR comments and suggestions and uploaded a new version. In addition I've added one more commit to package and install the library.

I've been using the version built by this branch for a few weeks now working on integrating an IGVM loader into QEMU so I am happy that the C library is working well. Therefore I consider this ready for removing the RFC/draft status.

igvm_c/cbindgen_igvm_defs.toml

joergroedel

Just a minor coding style issue, otherwise this looks good to me.

igvm_c/sample/dump_igvm.c

chris-oo · 2024-01-11T19:28:43Z

This is on my radar, I'll take a look at it tomorrow. Seems like I need to fix my github email settings because I'm not getting the emails I expect.

chris-oo

Thanks for cleaning up the changes!

A few comments left, mainly mechanical, but I'm happy to sign off and spin a new release once that's done. Thanks again for contributing these changes!

igvm_c/sample/dump_igvm.c

igvm_c/Makefile

igvm_c/test_data/src/main.rs

igvm_c/tests/igvm_test.c

chris-oo · 2024-01-12T21:51:00Z

igvm_defs/src/lib.rs

@@ -228,8 +229,6 @@ pub enum IgvmArchitecture {
 #[derive(AsBytes, FromBytes, FromZeroes, Debug, Clone, Copy, PartialEq, Eq)]
 #[repr(u32)]
 pub enum IgvmVariableHeaderType {
-    /// Invalid.


What's the reason for removing this?

This was removed as the generated C header file ends up with multiple definitions of INVALID that conflict. The alternatives were to either:

Rename INVALID to something like IGVM_VHT_INVALID here.

Modify the cbindgen configuration to add a prefix to every enum entry. But this would result in C definitions such as IGVM_VARIABLE_HEADER_TYPE_IGVM_VHT_SUPPORTED_PLATFORM.

Perform some post-processing of the generated header file.

I'm happy to change this to whichever option works best. Although, why do we need to allow a header to contain INVALID when we have error handling in the library?

My rationale is that even if it looks fairly obvious that 0 is not a free value, it's nice for us to document in the format exactly that "hey this value is invalid and don't try to use it".

I'm fine with renaming it to IGVM_VHT_INVALID if that satisfies the C bindgen req. A bit unfortunate, though.

I'm happy to sign off once this last little bit is in, thanks again for your changes!

I've managed to use some C preprocessor wizardry in cbindgen_igvm_defs.toml to remove the duplicate INVALID definition on the header file so I've removed the commit that deleted this INVALID enum entry, restoring it to how it was before.

Great thanks!

igvm/src/c_api.rs

joergroedel

Looks good to me now.

Currently unsafe_code is forbidden for igvm code. In order to add a C compatible API we will need to accept and work with raw C pointers to data. This requires a relaxing of the unsafe rule from 'forbid' to 'deny' allowing the C API feature, when enabled, to override the setting. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

This adds the docsrs configuration which is already in igvm_defs to the igvm crate and allows any future optional features to be correctly labelled in the generated documentation. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>

In preparation for adding the code to provide a C API this patch adds a cargo feature to allow the C API to be conditionally built. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

The C API will need to convert variable headers from the rust representation to C compatible structures. As part of this the C API will need to determine the original IgvmVariableHeaderType from the rust representation. This patch adds the required functions behind the igvm-c feature as they are not used outside of that feature. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

The C API will use write_binary_header() for converting from the rust representation of headers back to raw C structures. The write_binary_header() function was not previously implemented for IgvmPlatformHeader so this patch adds the function behind the igvm-c feature. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

This adds the c_api.rs file which implements functions that allow parsing and querying of igvm files in binary format exported in a suitable way for access from the C language. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

This ensures both the existing dynamic library and a static library that can be used to link into C applications is generated by the build. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

In preparation for using cbindgen to generate C header files from the rust structure definitions, add an annotation that prevents incompatible and unsupported types from being included in the output. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

Add a new folder named igvm_c that implements the following: * Generates C header files for igvm and igvm_defs that can be used to access some functionality from C using the existing static library. * Sample application that demonstrates using the C API to read and dump the contents of a binary igvm file. * Unit test executable for testing basic C API functionality. * Makefile that builds all C API components into the target_c output directory. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

Add unit tests that cover initialisation and destruction of the IGVM file using the C API and the querying of header and file data within the parsed file. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

Add a new git workflow job that builds the C API and runs the C unit tests via the Makefile. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

Adds an install task to the C Makefile that copies the header files into /usr/include/igvm/ and the library into /usr/lib64/ and creates a pkg-config entry for the library. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>

roy-hopkins · 2024-02-01T10:42:14Z

Rebased on microsoft:main. No other changes.

chris-oo reviewed Oct 4, 2023

View reviewed changes

roy-hopkins force-pushed the igvm_c branch from 90f3cc3 to d3fc3f2 Compare November 2, 2023 14:57

chris-oo reviewed Nov 14, 2023

View reviewed changes

chris-oo self-assigned this Nov 28, 2023

roy-hopkins mentioned this pull request Dec 14, 2023

Add C-based IGVM file builder coconut-svsm/svsm#182

Merged

roy-hopkins force-pushed the igvm_c branch from d3fc3f2 to 40e6dc0 Compare December 19, 2023 15:43

roy-hopkins requested a review from a team as a code owner December 19, 2023 15:43

roy-hopkins changed the title ~~RFC: Generate a C API for IGVM~~ Generate a C API for IGVM Dec 19, 2023

roy-hopkins commented Jan 3, 2024

View reviewed changes

igvm_c/cbindgen_igvm_defs.toml Outdated Show resolved Hide resolved

roy-hopkins force-pushed the igvm_c branch from 40e6dc0 to 1984125 Compare January 3, 2024 10:38

roy-hopkins mentioned this pull request Jan 8, 2024

Integrate an IGVM loader into QEMU coconut-svsm/qemu#3

Closed

joergroedel reviewed Jan 11, 2024

View reviewed changes

igvm_c/sample/dump_igvm.c Outdated Show resolved Hide resolved

chris-oo reviewed Jan 12, 2024

View reviewed changes

roy-hopkins force-pushed the igvm_c branch from 1984125 to 7ef0048 Compare January 24, 2024 10:24

joergroedel approved these changes Jan 29, 2024

View reviewed changes

This was referenced Jan 30, 2024

Implement an IGVM builder in rust coconut-svsm/svsm#235

Merged

Documentation: Update build/install instructions to use IGVM coconut-svsm/svsm#237

Merged

roy-hopkins force-pushed the igvm_c branch from 7ef0048 to c2c4812 Compare January 31, 2024 13:58

chris-oo approved these changes Jan 31, 2024

View reviewed changes

roy-hopkins and others added 5 commits February 1, 2024 10:38

igvm: Add configuration to document optional features

5b69035

This adds the docsrs configuration which is already in igvm_defs to the igvm crate and allows any future optional features to be correctly labelled in the generated documentation. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>

igvm: Add a feature to allow C API to be conditionally built

a45a09f

In preparation for adding the code to provide a C API this patch adds a cargo feature to allow the C API to be conditionally built. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

roy-hopkins and others added 7 commits February 1, 2024 10:38

igvm: Add c_api to provide a C compatible API

9085cff

This adds the c_api.rs file which implements functions that allow parsing and querying of igvm files in binary format exported in a suitable way for access from the C language. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

igvm: Create a static library

ac9736c

This ensures both the existing dynamic library and a static library that can be used to link into C applications is generated by the build. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

igvm_defs: Add cbindgen annotation

ab02e19

In preparation for using cbindgen to generate C header files from the rust structure definitions, add an annotation that prevents incompatible and unsupported types from being included in the output. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

c_api: Add rust unit tests for C API functions

a4f0fe0

Add unit tests that cover initialisation and destruction of the IGVM file using the C API and the querying of header and file data within the parsed file. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

workflows: Add job that builds and tests the C API

6662e75

Add a new git workflow job that builds the C API and runs the C unit tests via the Makefile. Signed-off-by: Roy Hopkins <rhopkins@suse.de>

igvm_c: Add install task to Makefile

8588297

Adds an install task to the C Makefile that copies the header files into /usr/include/igvm/ and the library into /usr/lib64/ and creates a pkg-config entry for the library. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>

roy-hopkins force-pushed the igvm_c branch from c2c4812 to 8588297 Compare February 1, 2024 10:40

chris-oo enabled auto-merge (squash) February 1, 2024 17:59

chris-oo merged commit 4c6ee46 into microsoft:main Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate a C API for IGVM #3

Generate a C API for IGVM #3

roy-hopkins commented Oct 3, 2023

roy-hopkins commented Oct 3, 2023 via email

chris-oo commented Oct 4, 2023

chris-oo left a comment

roy-hopkins commented Nov 2, 2023

chris-oo commented Nov 14, 2023

roy-hopkins commented Dec 19, 2023

joergroedel left a comment

chris-oo commented Jan 11, 2024

chris-oo left a comment

chris-oo Jan 12, 2024

roy-hopkins Jan 24, 2024

chris-oo Jan 25, 2024

chris-oo Jan 25, 2024

roy-hopkins Jan 31, 2024

chris-oo Jan 31, 2024

joergroedel left a comment

roy-hopkins commented Feb 1, 2024

Generate a C API for IGVM #3

Generate a C API for IGVM #3

Conversation

roy-hopkins commented Oct 3, 2023

roy-hopkins commented Oct 3, 2023 via email

chris-oo commented Oct 4, 2023

chris-oo left a comment

Choose a reason for hiding this comment

roy-hopkins commented Nov 2, 2023

chris-oo commented Nov 14, 2023

roy-hopkins commented Dec 19, 2023

joergroedel left a comment

Choose a reason for hiding this comment

chris-oo commented Jan 11, 2024

chris-oo left a comment

Choose a reason for hiding this comment

chris-oo Jan 12, 2024

Choose a reason for hiding this comment

roy-hopkins Jan 24, 2024

Choose a reason for hiding this comment

chris-oo Jan 25, 2024

Choose a reason for hiding this comment

chris-oo Jan 25, 2024

Choose a reason for hiding this comment

roy-hopkins Jan 31, 2024

Choose a reason for hiding this comment

chris-oo Jan 31, 2024

Choose a reason for hiding this comment

joergroedel left a comment

Choose a reason for hiding this comment

roy-hopkins commented Feb 1, 2024