vhost-device-gpu: Add support for GPU device path #903

dorindabassey · 2025-11-18T12:49:27Z

Summary of the PR

Add a new --gpu-path CLI argument that allows users to specify which GPU device to use for rendering when using the virglrenderer backend.
This is useful for systems with multiple GPUs where a specific device needs to be selected.

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.

vhost-device-gpu/src/lib.rs

vhost-device-gpu/README.md

vhost-device-gpu/CHANGELOG.md

vhost-device-gpu/src/backend/virgl.rs

vhost-device-gpu/src/lib.rs

vhost-device-gpu/src/backend/virgl.rs

Change all GPU adapter new() methods to return io::Result<Self>. This allows callers to decide how to handle initialization failures. Signed-off-by: Dorinda Bassey <[email protected]>

vhost-device-gpu/src/lib.rs

vhost-device-gpu/src/main.rs

germag · 2025-11-20T14:32:40Z

vhost-device-gpu/src/lib.rs

+        if flags.render_server_fd.is_some() && !matches!(gpu_mode, GpuMode::VirglRenderer) {
+            return Err(GpuConfigError::GpuPathNotSupportedByMode);
+        }
+


I must insist on using a builder pattern, so the validation can be higher in the caller

created an issue here - #910

Why not fix it now?

As I mentioned previously, the builder pattern refactor is out of scope for this PR. This PR adds the --gpu-device feature following the existing GpuConfig::new() pattern used throughout the codebase. Changing to a builder pattern would require refactoring how all backends instantiate GpuConfig and is a separate architectural change.

Is there a rush to merge this PR though? It'd make sense to first make the builder pattern change (in another PR or in a commit in this PR), then add the device path logic. There's no need to add something that you know you will remove.

I don't think it's out of scope btw, for what it's worth.

I disagree that this is in scope. The builder pattern would be a significant refactoring affecting the entire GpuConfig API and all three backends. The current implementation follows the existing pattern already in the codebase. This is a much bigger change than adding the --gpu-device feature alone. But if you feel strongly about it, I can implement the builder pattern first in another PR.

Sounds good, thanks!

I strongly disagree, I'm 100% with @epilys here, it doesn't make any sense to add something to be removed later.

I also don't think it will require lots of changes most places will change from GpuConfig::new(...) to something like GpuConfig::new(...).build(), and for VirglRenderer mode something like GpuConfig::new(GpuMode::VirglRenderer, ...).gpu_device(fd).build(), or something like that. Likewise, I think this PR is small enough to accommodate those changes.

As a side note, the description on issue #910 is unhelpful, adding a link to this thread is ok, but it should have more context

I'm ok of doing it in another PR first, thanks

vhost-device-gpu/src/backend/virgl.rs

vhost-device-gpu/src/backend/null.rs

vhost-device-gpu/README.md

Add a new --gpu-device CLI argument that allows users to specify which GPU device to use for rendering when using the virglrenderer backend. This is useful for systems with multiple GPUs where a specific device needs to be selected. Signed-off-by: Dorinda Bassey <[email protected]>

germag · 2025-11-21T11:31:34Z

vhost-device-gpu/src/main.rs

+    // Open GPU device file if path was provided
+    #[cfg(feature = "backend-virgl")]
+    let render_server_fd = args
+        .flags
+        .gpu_device
+        .as_ref()
+        .map(|gpu_device| {
+            File::open(gpu_device)
+                .map(Into::into)
+                .map_err(|_| GpuConfigError::InvalidGpuDevice(gpu_device.clone()))
+        })
+        .transpose()?;
+


this is not a suggestion to change the code, most probably it will need to be modified/moved to the builder

Is this AI generated?, starting from the comment that is basically useless, since I can see theFile::open(gpu_device). IMO comments should explain why something is not there, or the why's, but not a translation from code to natural language.

The other thing is, I have no problem of using a chain of methods, but don't try to be too "clever" (I made that sin also because is fun to write this type of code, but it really hurts legibility). In this case is not too problematic since is just opening a file.

germag · 2025-11-21T11:53:38Z

vhost-device-gpu/src/lib.rs

+        if flags.render_server_fd.is_some() && !matches!(gpu_mode, GpuMode::VirglRenderer) {
+            return Err(GpuConfigError::GpuPathNotSupportedByMode);
+        }
+


I strongly disagree, I'm 100% with @epilys here, it doesn't make any sense to add something to be removed later.

I also don't think it will require lots of changes most places will change from GpuConfig::new(...) to something like GpuConfig::new(...).build(), and for VirglRenderer mode something like GpuConfig::new(GpuMode::VirglRenderer, ...).gpu_device(fd).build(), or something like that. Likewise, I think this PR is small enough to accommodate those changes.

As a side note, the description on issue #910 is unhelpful, adding a link to this thread is ok, but it should have more context

dorindabassey · 2025-11-21T16:58:47Z

@germag @epilys @stefano-garzarella I'm leaving this here as a final note: I've spent a considerable amount of time addressing your feedbacks etc. I clearly stated multiple times that the builder pattern refactor is out of scope for this PR
#903 (comment)
#903 (comment)
#903 (comment)
#903 (comment)
Despite all this, you @germag and @epilys continued to push for architectural changes, dismissed my scope boundaries with "I don't think it's out of scope btw", and marked my PR as draft without discussion. Not only that but now @germag has moved to personal attacks, questioning whether my code is "AI generated", calling my comments "useless", and "but don't try to be too "clever"" comment. This is not productive collaboration, I'm closing this PR and the follow up issue. If the builder pattern is a must have as you insist, please open an issue proposing it so you all can discuss the architectural direction. The --gpu-device feature can wait.

germag · 2025-11-21T18:35:49Z

Please forgive this old and former teacher, and please don't do this, your code is not "you", it doesn't define you, so don't take any critic as a personal attack or as a critic to you. Assume good intentions, we most probably not only have a language barrier (english is not my first lang, not even my second, and I'm terrible at it), but maybe also a cultural one, so something that for me is innocuous for you is offensive and vice versa. And the written language cannot express the "mood" of what the other person wants to say, that can lead to misunderstandings.

Now I need to respond to this:

@germag @epilys @stefano-garzarella I'm leaving this here as a final note: I've spent a considerable amount of time addressing your feedbacks etc. I clearly stated multiple times that the builder pattern refactor is out of scope for this PR #903 (comment) #903 (comment) #903 (comment) #903 (comment) Despite all this, you @germag and @epilys continued to push for architectural changes, dismissed my scope boundaries with "I don't think it's out of scope btw",

Sorry, but we also invested time reviewing your changes. You didn't provide an argument why it is out of scope, just "too complex", same with returning an error that turns out to be trivial to implement. With all the other feedback you follow the same pattern: first "no" and then we had to explain again why it is important, for instance with the panic, exit, double open...

Why I think is in the scope, you are changing the API with an option restricted to a single mode, the restriction is checked too late in the call stack, and we think that is not ok, and adding the builder patter will not require too much work.

and marked my PR as draft without discussion.

I don't want to talk for @epilys, but to be honest, after that comment I understood you will open a PR implementing the builder, and then modify this one, so is totally fine to mark it draft if it depends on a future PR, I see not wrong from @epilys side...

Not only that but now @germag has moved to personal attacks, questioning whether my code is "AI generated", calling my comments "useless", and "but don't try to be too "clever"" comment.

Sorry, but no, I never do any personal comments. I just asked if that code was AI generated, because those types of comments are very typical of that. But later, I assumed that it wasn't so I tried to explain why is not a good comment, having a comment that add nothing is useless.

About being "clever", it's a well-known definition in the soft devel field:

Don’t be clever
Don't be clever
(and many more if you search for: "don't be clever" programming)

I just called "clever" the code not you, and is not an insult to you, just a description of code that do some clever things instead of being simple and straightforward, making it hard to read/understand.

Also, it's nothing wrong to use AI, but we as maintainers we need to know. However, it's not ok to just sent AI code putting all the burden of reviewing it on us (I'm not saying you did that, it's just a general comment)

This is not productive collaboration, I'm closing this PR and the follow up issue. If the builder pattern is a must have as you insist, please open an issue proposing it so you all can discuss the architectural direction. The --gpu-device feature can wait.

It's fine if you don't have time to work on it. But if you don't want to continue working on something because the maintainers are stubborn (whether they are right or wrong), don't slam the door while leaving, that can erode your reputation. You can just say "I don't have time to continue working on it, If anyone else wants to continue, that's fine with me"

and again take this as an honest attempt to help

dorindabassey requested review from epilys, stefano-garzarella, stsquad and vireshk as code owners November 18, 2025 12:49

dorindabassey mentioned this pull request Nov 18, 2025

vhost-device-gpu: Add support for specifying GPU path #900

Closed

dorindabassey force-pushed the gpuflag branch 3 times, most recently from 7f60738 to c27cb98 Compare November 18, 2025 13:05

stefano-garzarella reviewed Nov 18, 2025

View reviewed changes

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

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

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

vhost-device-gpu/src/backend/virgl.rs Outdated Show resolved Hide resolved

dorindabassey force-pushed the gpuflag branch from c27cb98 to a9daf88 Compare November 18, 2025 15:41

germag reviewed Nov 18, 2025

View reviewed changes

vhost-device-gpu/src/backend/virgl.rs Outdated Show resolved Hide resolved

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

dorindabassey force-pushed the gpuflag branch 2 times, most recently from c557101 to 91bf7f3 Compare November 19, 2025 11:50

stefano-garzarella reviewed Nov 19, 2025

View reviewed changes

vhost-device-gpu/src/backend/virgl.rs Outdated Show resolved Hide resolved

dorindabassey force-pushed the gpuflag branch 2 times, most recently from 0dd8175 to ec80528 Compare November 19, 2025 13:47

stefano-garzarella reviewed Nov 19, 2025

View reviewed changes

vhost-device-gpu/src/backend/virgl.rs Outdated Show resolved Hide resolved

vhost-device-gpu: Make Adapter new() return Result

9d66f7f

Change all GPU adapter new() methods to return io::Result<Self>. This allows callers to decide how to handle initialization failures. Signed-off-by: Dorinda Bassey <[email protected]>

dorindabassey force-pushed the gpuflag branch 4 times, most recently from cdf7bca to e67887c Compare November 20, 2025 13:46

germag reviewed Nov 20, 2025

View reviewed changes

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

dorindabassey force-pushed the gpuflag branch from e67887c to 912fd61 Compare November 21, 2025 08:21

dorindabassey mentioned this pull request Nov 21, 2025

refactor GpuConfig to use mode-specific configs with a builder pattern #910

Open

epilys marked this pull request as draft November 21, 2025 11:21

germag reviewed Nov 21, 2025

View reviewed changes

dorindabassey closed this Nov 21, 2025

vhost-device-gpu: Add support for GPU device path #903

vhost-device-gpu: Add support for GPU device path #903

Uh oh!

Conversation

dorindabassey commented Nov 18, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorindabassey commented Nov 21, 2025

Uh oh!

germag commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dorindabassey commented Nov 18, 2025 •

edited

Loading