vhost-device-gpu: Expose more options as CLI flags #797

mtjhrc · 2025-01-02T09:55:38Z

Summary of the PR

This PR adds command line arguments to explicitly configure which capsets to enable and flags to pass to the Rutabaga backend. By specifying the individual capsets explicitly this also fixes a TODO, which hard-coded the number of enabled capsets.

Not all possible capsets from the underlying backends are currently exposed, this exposes only those that at least partially work without blob resources. Previously all capsets, were implicitly enabled (the default in RutabagaBuilder).

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

staging/vhost-device-gpu/src/virtio_gpu.rs

staging/vhost-device-gpu/README.md

epilys

Minor comments, will take a longer look tomorrow

staging/vhost-device-gpu/src/lib.rs

staging/vhost-device-gpu/src/main.rs

staging/vhost-device-gpu/README.md

staging/vhost-device-gpu/src/main.rs

staging/vhost-device-gpu/src/lib.rs

dorindabassey · 2025-01-08T09:05:33Z

Thanks for the PR, just one small thing - this PR is below the existing code coverage, can you add enough tests to make it up to the previous code coverage?

mtjhrc · 2025-01-08T15:01:21Z

Thanks for the PR, just one small thing - this PR is below the existing code coverage, can you add enough tests to make it up to the previous code coverage?

I added more tests:

test_config_from_args (this actually replaces the test which tested only the capset_names_into_capset part of argument handling)
test_default_cli_flags
test_gpu_mode_display_eq_arg_name.

Now the coverage should be 88.00% (up from 87.80% without this PR).

staging/vhost-device-gpu/CHANGELOG.md

staging/vhost-device-gpu/README.md

staging/vhost-device-gpu/CHANGELOG.md

stefano-garzarella · 2025-01-20T14:28:17Z

@mtjhrc can you check musl CI, something is failing: https://buildkite.com/rust-vmm/vhost-device-ci/builds/2957#01948413-0e80-4786-b583-62f6266fc826

Can you also rebase on latest main?

Thanks,
Stefano

The name of the project/backend is "virglrenderer", the dash was added by clap. Adding the dash seems confusing to the user, the old name is kept as a hidden alias. Signed-off-by: Matej Hrica <[email protected]>

Remove capset constants definitions in protocol.rs, these are unecessary, because we use the constants defined in the Rutabaga crate. Signed-off-by: Matej Hrica <[email protected]>

staging/vhost-device-gpu/src/lib.rs

staging/vhost-device-gpu/src/main.rs

Do not build the GPU device for musl targets. To compile it properly would require setting up a build enivorment with all the native dependencies compiled with musl as well. Signed-off-by: Matej Hrica <[email protected]>

Remove the workaround to disable compiling the code on non-gnu targets. (the CI now doesn't attempt compiling on musl) Signed-off-by: Matej Hrica <[email protected]>

This was an oversight - for the user to be able to use the crate as a library, the start_backend needs to be part of the public API. Signed-off-by: Matej Hrica <[email protected]>

Add command line arguments for configuring which capsets and features are enabled when configuring Rutabaga. Since we now specify the capsets explicitly we can drop the MAX_NUM_CAPSETS constant and fix the TODO. The curently exposed capsets for virglrenderer are: virgl and virgl2. For gfxstream they are: gfxstream-vulkan and gfxstream-gles. Signed-off-by: Matej Hrica <[email protected]>

Update the README to better reflect the limitations of not supporting blob resources and remove the note about hardcoded number of capsets that has been fixed. Signed-off-by: Matej Hrica <[email protected]>

mtjhrc · 2025-01-22T12:32:55Z

As per @epilys suggestion, I excluded the gpu from building with musl in the CI - seems like much nicer solution then then conditional compilation disabling everything 😄

mtjhrc changed the title ~~vhost-user-gpu: Expose more options as CLI flags~~ vhost-device-gpu: Expose more options as CLI flags Jan 2, 2025

dorindabassey reviewed Jan 6, 2025

View reviewed changes

staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved

dorindabassey reviewed Jan 6, 2025

View reviewed changes

staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved

mtjhrc force-pushed the improved-cli branch 7 times, most recently from 34e9f4b to 33d97c0 Compare January 7, 2025 16:42

mtjhrc marked this pull request as ready for review January 7, 2025 16:52

mtjhrc requested review from stsquad, vireshk, stefano-garzarella and epilys as code owners January 7, 2025 16:52

epilys reviewed Jan 7, 2025

View reviewed changes

staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved

staging/vhost-device-gpu/src/main.rs Outdated Show resolved Hide resolved

staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved

dorindabassey reviewed Jan 8, 2025

View reviewed changes

staging/vhost-device-gpu/src/main.rs Outdated Show resolved Hide resolved

dorindabassey reviewed Jan 8, 2025

View reviewed changes

staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved

mtjhrc force-pushed the improved-cli branch 3 times, most recently from f4c67cc to 4787a30 Compare January 8, 2025 14:48

dorindabassey approved these changes Jan 9, 2025

View reviewed changes

stefano-garzarella reviewed Jan 10, 2025

View reviewed changes

staging/vhost-device-gpu/CHANGELOG.md Outdated Show resolved Hide resolved

staging/vhost-device-gpu/README.md Show resolved Hide resolved

staging/vhost-device-gpu/CHANGELOG.md Outdated Show resolved Hide resolved

staging/vhost-device-gpu/CHANGELOG.md Outdated Show resolved Hide resolved

mtjhrc force-pushed the improved-cli branch 2 times, most recently from 956ce52 to 679dedd Compare January 20, 2025 14:17

mtjhrc added 2 commits January 20, 2025 15:57

gpu: rename --gpu-mode virgl-renderer -> virglrenderer

1b9c597

The name of the project/backend is "virglrenderer", the dash was added by clap. Adding the dash seems confusing to the user, the old name is kept as a hidden alias. Signed-off-by: Matej Hrica <[email protected]>

gpu: remove unused duplicate constants

c66ca0d

Remove capset constants definitions in protocol.rs, these are unecessary, because we use the constants defined in the Rutabaga crate. Signed-off-by: Matej Hrica <[email protected]>

mtjhrc force-pushed the improved-cli branch from 679dedd to 76e59c6 Compare January 20, 2025 15:00

stefano-garzarella previously approved these changes Jan 20, 2025

View reviewed changes

epilys requested changes Jan 21, 2025

View reviewed changes

mtjhrc dismissed stefano-garzarella’s stale review via 55dbd7a January 21, 2025 13:01

mtjhrc force-pushed the improved-cli branch 3 times, most recently from f268f5e to e99832e Compare January 22, 2025 09:16

mtjhrc added 5 commits January 22, 2025 10:24

CI: do not build gpu for musl targets

d4402ae

Do not build the GPU device for musl targets. To compile it properly would require setting up a build enivorment with all the native dependencies compiled with musl as well. Signed-off-by: Matej Hrica <[email protected]>

gpu: remove CI workarounds

84deb1e

Remove the workaround to disable compiling the code on non-gnu targets. (the CI now doesn't attempt compiling on musl) Signed-off-by: Matej Hrica <[email protected]>

gpu: move start_backend from main.rs into lib.rs

9df5b05

This was an oversight - for the user to be able to use the crate as a library, the start_backend needs to be part of the public API. Signed-off-by: Matej Hrica <[email protected]>

gpu: update README to better reflect current limitations

5bca8b0

Update the README to better reflect the limitations of not supporting blob resources and remove the note about hardcoded number of capsets that has been fixed. Signed-off-by: Matej Hrica <[email protected]>

mtjhrc force-pushed the improved-cli branch from e99832e to 5bca8b0 Compare January 22, 2025 09:24

epilys approved these changes Jan 22, 2025

View reviewed changes

stefano-garzarella enabled auto-merge (rebase) January 22, 2025 14:15

stefano-garzarella approved these changes Jan 22, 2025

View reviewed changes

stefano-garzarella merged commit ec0f309 into rust-vmm:main Jan 22, 2025
2 checks passed

mtjhrc mentioned this pull request Jan 29, 2025

Consider removing the default test_description.json or add ability to disable musl builds rust-vmm/rust-vmm-ci#176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

vhost-device-gpu: Expose more options as CLI flags #797

vhost-device-gpu: Expose more options as CLI flags #797

Uh oh!

mtjhrc commented Jan 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

epilys left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorindabassey commented Jan 8, 2025

Uh oh!

mtjhrc commented Jan 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefano-garzarella commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtjhrc commented Jan 22, 2025

Uh oh!

Uh oh!

Uh oh!

vhost-device-gpu: Expose more options as CLI flags #797

vhost-device-gpu: Expose more options as CLI flags #797

Uh oh!

Conversation

mtjhrc commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the PR

Requirements

Uh oh!

Uh oh!

Uh oh!

epilys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorindabassey commented Jan 8, 2025

Uh oh!

mtjhrc commented Jan 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefano-garzarella commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtjhrc commented Jan 22, 2025

Uh oh!

Uh oh!

Uh oh!

mtjhrc commented Jan 2, 2025 •

edited

Loading