vhost-device-gpu: Add support for GPU device path #903
Conversation
dorindabassey
requested review from
epilys,
stefano-garzarella,
stsquad and
vireshk
as code owners
dorindabassey
force-pushed
the
gpuflag
branch
3 times, most recently
from
7f60738 to
c27cb98
Compare
dorindabassey
force-pushed
the
gpuflag
branch
from
c27cb98 to
a9daf88
Compare
dorindabassey
force-pushed
the
gpuflag
branch
2 times, most recently
from
c557101 to
91bf7f3
Compare
dorindabassey
force-pushed
the
gpuflag
branch
2 times, most recently
from
0dd8175 to
ec80528
Compare
| // Open the GPU device if a path was specified | ||
| // Note: The path is validated early in start_backend(), so this should not fail | ||
| let render_server_fd = config | ||
| .flags() | ||
| .gpu_path | ||
| .as_ref() | ||
| .map(|path| File::open(path).map(|f| f.into()).unwrap()); |
There was a problem hiding this comment.
Sorry, I don't see the point. Now we can propagate the error, so what we are gaining to check this in advance?
Also this is racy, because the file can disappear between the 2 opens and so here it can crash...
There was a problem hiding this comment.
I put that validation early on because without it, when File::open() fails in VirglRendererAdapter::new(), the error is propagated up through the event loop but the daemon just hangs waiting for events, silently ignoring that the initialization failed. So now we have a clear error message that the user can see.
There was a problem hiding this comment.
Also this is racy, because the file can disappear between the 2 opens and so here it can crash...
About this, it's strange that a GPU device node like /dev/dri/renderD128 would disappear, they're not regular files. Also what are the chances that someone unloads the GPU driver between both validations? the time between both operations is like nanoseconds.
There was a problem hiding this comment.
Also this is racy, because the file can disappear between the 2 opens and so here it can crash...
About this, it's strange that a GPU device node like
/dev/dri/renderD128would disappear, they're not regular files. Also what are the chances that someone unloads the GPU driver between both validations? the time between both operations is like nanoseconds.
Looking better in the code after your concern, IIUC there is a lazy initialization of the backend with lazy_init_and_handle_event (that was the missing point, at least for me). So it's really not nanoseconds but depends on when the guest will send the first request IIUC. So between starting the vhost-user device, launching the VM, and having the guest driver loaded is more than several seconds and everything can happen (file permission changed, driver unloaded, etc).
In general, hoping that everything will be fine because we have "some time" is a red flag IMO. So again, I'm sorry, but opening a file/device or anything else twice, just to check if the path is accessible and hoping that between the 2 syscall nothing will happen, makes no sense to me; there are a thousand ways to attack this thing, and it is definitely not good programming practice.
That said, can we open the file while creating GpuConfig or VhostUserGpuBackend and pass the file (not the path) to VirglRendererAdapter::new() in some way ?
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
from
ec80528 to
2008c8f
Compare
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. Signed-off-by: Dorinda Bassey <[email protected]>
dorindabassey
force-pushed
the
gpuflag
branch
from
2008c8f to
7f577bb
Compare
5 participants
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:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.