File I/O DMA region access mode#842
Conversation
This adds an enum to convey how a DMA region is to be accessed. Previously, this was implicit in the presence of a file descriptor in the DMA region tracking information. With this change, a new file I/O access mode is added that designates accesses should happen via file I/O syscalls, such as `pread()`/`pwrite()`. This is useful for file descriptors that don't support `mmap()`, such as `/proc/<pid>/mem`. The access mode to be used is determined from new flag bits in the VFIO_USER_DMA_MAP message, and the message handler makes sure the passed flags and file descriptor presence are consistent. Existing code is updated to use the access mode in conditionals rather than testing file descriptor presence. Signed-off-by: Mattias Nissler <mnissler@meta.com>
When performing DMA from/to a caller-supplied buffer via `vfu_sgl_write` and `vfu_sgl_read`, respectively, we can use file descriptor based access modes as configured for the region, rather than servicing the DMA operation via IPC unconditionally. Signed-off-by: Mattias Nissler <mnissler@meta.com>
jlevon
left a comment
There was a problem hiding this comment.
Not sure I'm quite clear on the use case - when would I be sharing something like /proc files with the server?
This is for a case where a non-VMM client wants to expose buffers to the server, and it isn't feasible in practice to change all memory allocation code paths in the client to use |
This seems like it would only be useful in full-trust scenarios - not something we have as a typical design principle - but regardless the basic plumbing functionality seems reasonable. |
Yes, this gives the server full access to the entire address space, and it's certainly not something that a VMM would want to do with a potentially hostile server. I would argue though that access via mmap can turn into a big foot-gun as well if not used carefully, so clients already have to be careful which file descriptors are OK to expose. |
Share tear down logic across `remove_region` and `remove_all_regions`, check access mode to select appropriate tear down actions. Signed-off-by: Mattias Nissler <mnissler@meta.com>
Use the new `dma_controller_destroy_region` helper and make sure the passed-in fd gets closed only when we fail before deduplicating it. Signed-off-by: Mattias Nissler <mnissler@meta.com>
Signed-off-by: Mattias Nissler <mnissler@meta.com>
Put file descriptor based direct access behind the new VFU_SGL_DIRECT_ACCESS flag. Update documentation to clarify behavior. Signed-off-by: Mattias Nissler <mnissler@meta.com>
3418e40 to
c8f3bb6
Compare
Signed-off-by: Mattias Nissler <mnissler@meta.com>
c8f3bb6 to
c35569a
Compare
|
@mnissler-rivos are you waiting for a reveiw for this? |
Yes, this is ready for another round of review. |
Signed-off-by: Mattias Nissler <mnissler@meta.com>
Signed-off-by: Mattias Nissler <mnissler@meta.com>
| uint64_t size, int fd, off_t offset, | ||
| uint32_t prot) | ||
| dma_controller_add_region(dma_controller_t *dma, void *dma_addr, uint64_t size, | ||
| int *fd, off_t offset, uint32_t prot, |
There was a problem hiding this comment.
@jlevon @mnissler-rivos This commit does not compile in my environment. The previous master commit ee13121 compiles without issue.
I am in a Yocto 6.0 build environment with the following package versions:
gcc 15.2.0
cmake 4.3.1
cmocka 2.0.2
meson 1.10.2
ninja 1.13.2
json-c 0.18
Cmocka does not like the change from 'int fd' to 'int *fd'
| x86_64-pokysdk-linux-gcc -march=x86-64 --sysroot=/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/recipe-sysroot -Itest/unit_tests.p -Itest -I../sources/libvfio-user-0.0.1/test -Iinclude -I../sources/libvfio-user-0.0.1/include -Ilib -I../sources/libvfio-user-0.0.1/lib -I/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/recipe-sysroot/usr/local/oe-sdk-hardcoded-buildpath/sysroots/x86_64-pokysdk-linux/usr/include/json-c -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O2 -fcanon-prefix-map -ffile-prefix-map=/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/sources/libvfio-user-0.0.1=/usr/src/debug/nativesdk-libvfio-user/0.0.1 -ffile-prefix-map=/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/build=/usr/src/debug/nativesdk-libvfio-user/0.0.1 -ffile-prefix-map=/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/recipe-sysroot= -ffile-prefix-map=/opt/workspace/demo/yocto/build/target/work/x86_64-nativesdk-pokysdk-linux/nativesdk-libvfio-user/0.0.1/recipe-sysroot-native= -pipe -DUNIT_TEST -DWITH_TRAN_PIPE -D_GNU_SOURCE -Wno-missing-field-initializers -Wmissing-declarations -Wwrite-strings -DHAVE_LINUX_KCMP_H -MD -MQ test/unit_tests.p/mocks.c.o -MF test/unit_tests.p/mocks.c.o.d -o test/unit_tests.p/mocks.c.o -c ../sources/libvfio-user-0.0.1/test/mocks.c | ../sources/libvfio-user-0.0.1/test/mocks.c: In function 'dma_controller_add_region': | ../sources/libvfio-user-0.0.1/test/mocks.c:127:5: error: initialization of 'long int' from 'int *' makes integer from pointer without a cast [-Wint-conversion] | 127 | check_expected_int(fd); | | ^~~~~~~~~~~~~~~~~~ | ../sources/libvfio-user-0.0.1/test/mocks.c:127:5: note: (near initialization for '(anonymous).int_val')
There was a problem hiding this comment.
please file an issue (and a PR if you have one)
There was a problem hiding this comment.
I think it should be check_expected_ptr() maybe
There was a problem hiding this comment.
Yes, check_expected_ptr() works. I submitted #863.
This PR adds a new mode to perform DMA operations using file I/O system calls. This is useful in cases where the client can supply a file descriptor, but the file descriptor doesn't support
mmap(). This is the case for/proc/<pid>/memfor example, which can be used to give the server access to the client's address space.