Implement virtio-gpu 2D and virtio-input devices #121
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="scripts/rootfs_ext4.sh">
<violation number="1" location="scripts/rootfs_ext4.sh:14">
P2: This `cp` command will fail if the `extra_packages` directory doesn't exist. The directory is not present in the repository and there's no guard checking for its existence. Consider adding a conditional check to only copy when the directory exists.</violation>
</file>
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:598">
P2: Halfword reads (RV_MEM_LH, RV_MEM_LHU) are passing size=1 instead of size=2. While the FIXME comment suggests per-byte access, this could cause incorrect behavior if the guest issues actual halfword reads. Consider separating the cases or documenting why size=1 is correct for all access widths.</violation>
</file>
<file name="configs/riscv-cross-file">
<violation number="1" location="configs/riscv-cross-file:16">
P2: Invalid `cpu_family` value: `'riscv'` is not a valid Meson cpu_family. According to Meson's reference tables, valid values for RISC-V are `'riscv32'` or `'riscv64'`. Since this targets 32-bit RISC-V, use `'riscv32'`.</violation>
</file>
<file name="window-sw.c">
<violation number="1" location="window-sw.c:96">
P2: Missing NULL check: `SDL_CreateRGBSurfaceWithFormatFrom` can return NULL on allocation failure. The returned `surface` is used directly in `SDL_CreateTextureFromSurface` without validation, which could cause undefined behavior.</violation>
<violation number="2" location="window-sw.c:212">
P1: Race condition: `cursor_clear_sw` modifies shared state and signals condition variable without holding the mutex. This can cause data races with the window thread which reads `cursor_rect` and `cursor` while holding the lock. Add `window_lock_mutex(scanout_id)` before modifying state and `window_unlock_mutex(scanout_id)` after signaling.</violation>
<violation number="3" location="window-sw.c:246">
P2: Race condition: cursor position is updated before acquiring the mutex. The `cursor_rect.x` and `cursor_rect.y` assignments should be inside the critical section to prevent data races with the window thread.</violation>
</file>
<file name="virtio-gpu.h">
<violation number="1" location="virtio-gpu.h:143">
P1: Pointer `uint8_t *capset_data` inside a PACKED structure is problematic. PACKED structures are used for wire format serialization, and pointers (which contain host addresses and vary in size between architectures) are not suitable. Consider using a flexible array member `uint8_t capset_data[];` instead.</violation>
<violation number="2" location="virtio-gpu.h:299">
P2: Typo in field name: `trasfer_to_host_2d` should be `transfer_to_host_2d` (missing 'n'). This inconsistency with the corresponding enum `VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D` could lead to confusion and bugs.</violation>
<violation number="3" location="virtio-gpu.h:332">
P2: Typo in function name: `vgpu_destory_resource_2d` should be `vgpu_destroy_resource_2d` (misspelled 'destroy' as 'destory').</violation>
</file>
<file name="virtio-gpu-sw.c">
<violation number="1" location="virtio-gpu-sw.c:26">
P1: Memory leak: `res_2d->iovec` is allocated but not freed when returning early due to corrupted page address. Add `free(res_2d->iovec); res_2d->iovec = NULL;` before the return statement.</violation>
<violation number="2" location="virtio-gpu-sw.c:82">
P2: Incorrect buffer allocation formula mixes pixels and bytes. `request->width` is in pixels while `res_2d->stride` is in bytes (4096). The formula should likely be `res_2d->stride * request->height` since stride already represents bytes per row.</violation>
</file>
<file name="common.h">
<violation number="1" location="common.h:25">
P1: Using `1 << bit` where `bit` can be >= 32 causes undefined behavior. The literal `1` is of type `int` (32-bit), but `bit` is `unsigned long` and can exceed 31 on 64-bit systems (since `BITS_PER_LONG` would be 64). Use `1UL << bit` to ensure the shift operates on an `unsigned long`.</violation>
</file>
<file name="virtio-gpu.c">
<violation number="1" location="virtio-gpu.c:78">
P0: Buffer offset calculated twice: `dest` already includes `done` offset (line 76), but `memcpy` adds `done` again. This writes data to the wrong location (`buf + 2*done` instead of `buf + done`).</violation>
<violation number="2" location="virtio-gpu.c:273">
P1: Logical AND `&&` used instead of bitwise AND `&`. This results in `edid[9]` being set to 1 (if vendor_id is non-zero) instead of the lower 8 bits of vendor_id, producing an incorrect EDID vendor identifier.</violation>
</file>
<file name="scripts/build-image.sh">
<violation number="1" location="scripts/build-image.sh:180">
P1: Missing directory existence check before `git clone`. If the script is run twice, this will fail because the `DirectFB2` directory already exists. This is inconsistent with the patterns used in `do_buildroot` and `do_linux` which check `if [ ! -d directory ]` before cloning.</violation>
<violation number="2" location="scripts/build-image.sh:190">
P1: Missing directory existence check before `git clone`. Same issue as above - running the script twice will fail because the `DirectFB-examples` directory already exists.</violation>
</file>
<file name="window-events.c">
<violation number="1" location="window-events.c:86">
P1: Copy-paste error: `SDLK_F7` is mapped twice while `SDLK_F8` is missing. The F8 key won't work.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
This file is copied directly from the Linux kernel source.
Define the key codes required by semu and retain the reference to the original source to avoid license violations.
| [host_machine] | ||
| system = 'linux' | ||
| cpu_family = 'riscv' | ||
| cpu_family = 'riscv32' |
There was a problem hiding this comment.
Any reference to justify this?
There was a problem hiding this comment.
There are only riscv32 and riscv64 in the Meson reference table for CPU families
There was a problem hiding this comment.
I see, thanks for the clarification.
|
|
||
| struct key_map_entry key_map[] = { | ||
| /* Mouse */ | ||
| DEF_KEY_MAP(SDL_BUTTON_LEFT, BTN_LEFT), |
There was a problem hiding this comment.
This does not cover all key code, we can create a new GitHub issue once the pull request is merged.
| res_2d->stride = STRIDE_SIZE; | ||
| res_2d->image = malloc(bytes_per_pixel * (request->width + res_2d->stride) * | ||
| request->height); | ||
| res_2d->image = malloc(res_2d->stride * request->height); |
There was a problem hiding this comment.
Not sure why this change is made.
Could you compare with this reference? (line 511)
psun3x/acrn-hypervisor@edc137c
There was a problem hiding this comment.
The call graph in the reference you gave will call
pixman_image_create_bits(r2d->format, r2d->width, r2d->height, NULL, 0);This will get into create_bits_image_internal with rowstride_bytes set to 0. Then it call create_bits further, which is the place that will calculate the stride.
In create_bits, the main process for the calculation of stride is:
stride = width * bpp;: Calculate the total bit.stride += 0x1f;: Alignment.stride >>= 5;: Equivlaent to devided by 32, to calculate how many 32-bit it need.stride *= sizeof(uint32_t);: Transform to byte.
For example, take width as 100, if the format is VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM, which will be transformed to PIXMAN_x8r8g8b8, then the bit per pixel (bpp) is 32, thus the stride is ((100 * 32 + 31) / 32) * 4, which is 400 (byte).
After it get the stride, it will just calculate the size of buffer via height * stride directly and malloc(buf_size).
Thus, I think the point is that the size we need to malloc is height * stride, but the original implementation mixes pixels and bytes (request->width + res_2d->stride). The correct formula is (bytes_per_pixel * request->width) * request->height, where the first part is res_2d->stride.
There was a problem hiding this comment.
BTW, as @visitorckw mentioned below, the res_2d->stride should be calculated as width * bpp, but I'm not sure if I need to do the alignment here.
I briefly looked through the kernel code. For dumb buffers, it's simply width * bpp without alignment.
https://elixir.bootlin.com/linux/v6.12.64/source/drivers/gpu/drm/virtio/virtgpu_gem.c#L74
There was a problem hiding this comment.
The original implementation of my virtio-gpu used pixman. When I attempted to remove the pixman dependency, I introduced a hard-coded STRIDE_SIZE of 4096 as @visitorckw pointed out. After reviewing the VirtIO GPU spec. again, I think this may be incorrect. shengwen-tw@1120d55
In virtio-gpu 2D mode (VIRTIO_GPU_CMD_RESOURCE_CREATE_2D, VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D, and VIRTIO_GPU_CMD_SET_SCANOUT), the protocol does not define an explicit stride field. This suggests that the expected memory layout might be just tightly packed, i.e., each row is exactly width * bytes_per_pixel with no additional per-row padding. If so, explicit stride handling may be unnecessary or even incorrect.
@Mes0903 :
Could you help run some experiments to confirm whether we can remove the stride handling entirely? For example, by checking the size of the received buffer and testing what happens when STRIDE_SIZE is set to 0.
| function copy_buildroot_config | ||
| { | ||
| local buildroot_config="configs/buildroot.config" | ||
| local x11_config="configs/x11.config" |
There was a problem hiding this comment.
@jserv:
I prefer to keep the X11 build configuration optional, as we will need it later to test and verify VirtIO GPU 3D mode.
There was a problem hiding this comment.
I prefer to keep the X11 build configuration optional, as we will need it later to test and verify VirtIO GPU 3D mode.
X11 specific configurations should be excluded for this pull request, keeping it simple.
There was a problem hiding this comment.
@Mes0903
Let’s separate the X11 build option from this pull request and move it into a new commit, in case we need to use it later.
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
Add headless tests in CI/CD.
virtio-input.c
Outdated
| vinput->ram[queue->QueueUsed] &= MASK(16); | ||
| /* Update the used ring pointer (virtq_used.idx) */ | ||
| /* TODO: Check if the or-ing is valid or not */ | ||
| // vinput->ram[queue->QueueUsed] |= ((uint32_t) new_used) << 16; |
There was a problem hiding this comment.
@Mes0903
Could you check the correct implementation according to the VirtIO spec.?
There was a problem hiding this comment.
I think the current implementation already following the spec, but the MASK(16) is for lower 16 bits, which means keep lower 16 bits (flags) and clean higher 16 bits (idx). I think the original implementation is contrary to the comment.
I changed to the following implementation for readability for now:
/* Update used ring header */
uint16_t *used_hdr = (uint16_t *) &vinput->ram[queue->QueueUsed];
used_hdr[0] = 0; /* virtq_used.flags */
used_hdr[1] = new_used; /* virtq_used.idx */Also, the spec said
A device MUST NOT write to any descriptor table entry.
So I delete the desc[3] = 0 part and add some simple check.
virtio-input.c
Outdated
| break; | ||
| case RV_MEM_SB: | ||
| case RV_MEM_SH: | ||
| /* FIXME: virtio-input driver need to access device config register per |
There was a problem hiding this comment.
Please refactor the code here.
visitorckw
left a comment
visitorckw
left a comment
There was a problem hiding this comment.
Patches 4 and 5 appear to be fixups for earlier commits. This violates commit atomicity and breaks bisectability. Please use git rebase -i to squash them into the relevant patches.
virtio-gpu-sw.c
Outdated
| #include "virtio-gpu.h" | ||
| #include "window.h" | ||
|
|
||
| #define STRIDE_SIZE 4096 |
There was a problem hiding this comment.
How was this value derived?
Shouldn't this be calculated dynamically based on width * bytes_per_pixel (or the stride provided by the guest) to support arbitrary resolutions and avoid potential display corruption?
There was a problem hiding this comment.
I think you're right here, thank you.
@Mes0903 |
Mes0903
force-pushed
the
vgpu-2D-rework
branch
from
50961c6 to
6c8ed02
Compare
|
@Mes0903 , could you please upload the patched |
device.h
Outdated
| /* supplied by environment */ | ||
| uint32_t *ram; | ||
| /* implementation-specific */ | ||
| int id; // FIXME |
There was a problem hiding this comment.
More comment for this?
main.c
Outdated
| #if SEMU_HAS(VIRTIOGPU) | ||
| emu->vgpu.ram = emu->ram; | ||
| virtio_gpu_init(&(emu->vgpu)); | ||
| virtio_gpu_add_scanout(&(emu->vgpu), 1024, 768); |
There was a problem hiding this comment.
Can you reuse SCREEN_WIDTH and SCREEN_HEIGHT?
| bits_per_pixel = 32; | ||
| break; | ||
| case VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM: | ||
| bits_per_pixel = 32; |
There was a problem hiding this comment.
The cases here and above are all do the same thing: bits_per_pixel = 32; break;, can you merge them?
There was a problem hiding this comment.
11 issues found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="target/run.sh">
<violation number="1" location="target/run.sh:3">
P2: Avoid introducing an empty LD_LIBRARY_PATH entry when the variable was previously unset—an empty entry makes the dynamic linker search the current directory first, which is a security risk.</violation>
</file>
<file name="scripts/build-image.sh">
<violation number="1" location="scripts/build-image.sh:175">
P2: Inconsistent PATH handling: appends buildroot toolchain to PATH instead of prepending. Other functions (`do_linux`, `do_extra_packages`) correctly prepend to ensure the cross-compiler is found first. This could cause the wrong toolchain to be used if the system has conflicting tools.</violation>
</file>
<file name="virtio-gpu.h">
<violation number="1" location="virtio-gpu.h:7">
P2: Duplicate macro definition. `VIRTIO_GPU_MAX_SCANOUTS` is already defined in `virtio.h` which is included by this file. Remove this duplicate definition to avoid potential maintenance issues if the values diverge.</violation>
</file>
<file name="window-sw.c">
<violation number="1" location="window-sw.c:245">
P1: Missing NULL check after `vgpu_get_resource_2d`. If `res_id` is invalid, the function returns NULL and `resource->format` on line 249 will cause a null pointer dereference. Other call sites in virtio-gpu-sw.c always check for NULL.</violation>
<violation number="2" location="window-sw.c:271">
P1: Missing NULL check after `malloc`. If memory allocation fails, `memcpy` on line 267 will dereference NULL pointer.</violation>
</file>
<file name="scripts/rootfs_ext4.sh">
<violation number="1" location="scripts/rootfs_ext4.sh:16">
P2: Copying extra packages with `cp -r` drops ownership/permission data and dereferences symlinks; use `cp -a` to preserve metadata so the rootfs image contains valid files.</violation>
</file>
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:666">
P1: Missing bounds check on `vinput_dev_cnt` before using as array index. If `virtio_input_init` is called more than `VINPUT_DEV_CNT` (2) times, this causes an out-of-bounds write to `vinput_dev[]`.</violation>
</file>
<file name="window-events.c">
<violation number="1" location="window-events.c:138">
P1: Busy-waiting loop will consume 100% CPU when no events are pending. Consider using `SDL_WaitEvent` instead of `SDL_PollEvent`, or add `SDL_Delay(1)` inside the loop when no event is received.</violation>
<violation number="2" location="window-events.c:145">
P2: Unchecked return value of `sdl_key_to_linux_key`. When the key is not found, -1 is passed to `virtio_input_update_key`, which will be interpreted as 0xFFFFFFFF (invalid key code). Add a check before calling the virtio function.</violation>
</file>
<file name="virtio-gpu-sw.c">
<violation number="1" location="virtio-gpu-sw.c:32">
P1: Memory leak: when `malloc(res_2d->image)` fails, the previously allocated `res_2d` structure is not freed and not removed from the resource list. Call `vgpu_destroy_resource_2d(request->resource_id)` before returning to properly clean up.</violation>
<violation number="2" location="virtio-gpu-sw.c:209">
P1: Potential NULL pointer dereference: `res_2d->iovec` may be NULL if `attach_backing` hasn't been called yet for this resource. Add a NULL check before accessing `res_2d->iovec[0].iov_base`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,4 @@ | |||
| #!/usr/bin/bash | |||
|
|
|||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib | |||
There was a problem hiding this comment.
P2: Avoid introducing an empty LD_LIBRARY_PATH entry when the variable was previously unset—an empty entry makes the dynamic linker search the current directory first, which is a security risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At target/run.sh, line 3:
<comment>Avoid introducing an empty LD_LIBRARY_PATH entry when the variable was previously unset—an empty entry makes the dynamic linker search the current directory first, which is a security risk.</comment>
<file context>
@@ -0,0 +1,4 @@
+#!/usr/bin/bash
+
+export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib
+export PATH=$PATH:/usr/local/bin/
</file context>
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib | |
| export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}/usr/local/lib |
|
|
||
| function do_directfb | ||
| { | ||
| export PATH=$PATH:$PWD/buildroot/output/host/bin |
There was a problem hiding this comment.
P2: Inconsistent PATH handling: appends buildroot toolchain to PATH instead of prepending. Other functions (do_linux, do_extra_packages) correctly prepend to ensure the cross-compiler is found first. This could cause the wrong toolchain to be used if the system has conflicting tools.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-image.sh, line 175:
<comment>Inconsistent PATH handling: appends buildroot toolchain to PATH instead of prepending. Other functions (`do_linux`, `do_extra_packages`) correctly prepend to ensure the cross-compiler is found first. This could cause the wrong toolchain to be used if the system has conflicting tools.</comment>
<file context>
@@ -133,11 +170,72 @@ while [[ $# -gt 0 ]]; do
+function do_directfb
+{
+ export PATH=$PATH:$PWD/buildroot/output/host/bin
+ export BUILDROOT_OUT=$PWD/buildroot/output/
+ mkdir -p directfb
</file context>
| export PATH=$PATH:$PWD/buildroot/output/host/bin | |
| export PATH="$PWD/buildroot/output/host/bin:$PATH" |
|
|
||
| if [ -d extra_packages ] && [ -n "$(ls -A extra_packages 2>/dev/null)" ]; then | ||
| echo "[*] Copying extra packages..." | ||
| cp -r extra_packages/. $DIR/ |
There was a problem hiding this comment.
P2: Copying extra packages with cp -r drops ownership/permission data and dereferences symlinks; use cp -a to preserve metadata so the rootfs image contains valid files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/rootfs_ext4.sh, line 16:
<comment>Copying extra packages with `cp -r` drops ownership/permission data and dereferences symlinks; use `cp -a` to preserve metadata so the rootfs image contains valid files.</comment>
<file context>
@@ -11,6 +11,13 @@ echo "[*] Remove old rootfs directory..."
+if [ -d extra_packages ] && [ -n "$(ls -A extra_packages 2>/dev/null)" ]; then
+ echo "[*] Copying extra packages..."
+ cp -r extra_packages/. $DIR/
+else
+ echo "[*] No extra_packages directory or directory is empty, skipping..."
</file context>
| cp -r extra_packages/. $DIR/ | |
| cp -a extra_packages/. $DIR/ |
There was a problem hiding this comment.
17 issues found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="target/run.sh">
<violation number="1" location="target/run.sh:3">
P2: Prevent LD_LIBRARY_PATH from acquiring a leading empty entry (which adds the current directory to the search path) when the variable is initially unset.</violation>
<violation number="2" location="target/run.sh:4">
P2: Guard against introducing an empty PATH entry so the current working directory is not implicitly added ahead of trusted binaries.</violation>
</file>
<file name="configs/riscv-cross-file">
<violation number="1" location="configs/riscv-cross-file:8">
P1: `@GLOBAL_SOURCE_ROOT@` is not a valid Meson cross-file variable and won't be substituted. The build script copies this file directly without preprocessing. Consider using absolute paths or add a preprocessing step to substitute this placeholder before running meson.</violation>
</file>
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:168">
P2: Wraparound arithmetic is off by one. When handling uint16_t index wraparound, adding `UINT16_MAX` (65535) is incorrect; it should be `UINT16_MAX + 1` (65536) or `(1U << 16)` to properly account for the full 16-bit range.</violation>
<violation number="2" location="virtio-input.c:207">
P1: Missing bounds validation on `vq_desc.addr` before memory write. A malicious guest could provide an out-of-bounds address, causing writes outside the allocated RAM. Add a check: `if (vq_desc.addr + sizeof(struct virtio_input_event) > RAM_SIZE) return false;`</violation>
<violation number="3" location="virtio-input.c:666">
P1: Missing bounds check on `vinput_dev_cnt` before array access. If `virtio_input_init` is called more than `VINPUT_DEV_CNT` (2) times, this will cause an out-of-bounds write to `vinput_dev[]`. Add a bounds check before assignment.</violation>
</file>
<file name="window-sw.c">
<violation number="1" location="window-sw.c:245">
P1: Missing NULL check after `vgpu_get_resource_2d()`. If the resource is not found, the function returns NULL and accessing `resource->format` will cause a crash.</violation>
<violation number="2" location="window-sw.c:271">
P1: Missing NULL check after `malloc()`. If memory allocation fails, `memcpy` on line 263 will dereference NULL and crash.</violation>
</file>
<file name="virtio-gpu-sw.c">
<violation number="1" location="virtio-gpu-sw.c:24">
P1: Memory leak: `res_2d` is already created and added to the resource list when image allocation fails. Call `vgpu_destroy_resource_2d(request->resource_id)` before returning to properly clean up the resource.</violation>
<violation number="2" location="virtio-gpu-sw.c:209">
P1: Potential NULL pointer dereference: `res_2d->iovec[0].iov_base` is accessed without checking if `res_2d->iovec` is non-NULL. If transfer is called before backing memory is attached, this will crash. Add a null check before accessing iovec.</violation>
</file>
<file name="window-events.c">
<violation number="1" location="window-events.c:145">
P1: The return value of `sdl_key_to_linux_key` (-1 for unmapped keys) is not validated before being passed to `virtio_input_update_key`. This causes -1 to be implicitly converted to `UINT32_MAX` and sent as an invalid key code to the guest.</violation>
<violation number="2" location="window-events.c:150">
P1: Same issue as SDL_KEYDOWN: the return value of `sdl_key_to_linux_key` (-1 for unmapped keys) is not validated before being passed to `virtio_input_update_key`.</violation>
<violation number="3" location="window-events.c:155">
P2: The return value of `sdl_key_to_linux_key` is not validated before being passed to `virtio_input_update_mouse_button_state`. Unmapped mouse buttons (scroll wheel, side buttons) will result in invalid button codes.</violation>
<violation number="4" location="window-events.c:160">
P2: Same issue as SDL_MOUSEBUTTONDOWN: the return value of `sdl_key_to_linux_key` is not validated before being passed to `virtio_input_update_mouse_button_state`.</violation>
</file>
<file name="scripts/build-image.sh">
<violation number="1" location="scripts/build-image.sh:47">
P1: Missing `ASSERT` wrapper on merge_tool command. If the config merge fails, the script will continue with an incorrect or incomplete Buildroot configuration, leading to confusing build failures later.</violation>
<violation number="2" location="scripts/build-image.sh:190">
P2: Missing `ASSERT` wrapper on `meson install` command. If installation fails, the script continues silently, potentially resulting in incomplete package installation.</violation>
</file>
<file name="virtio-gpu.c">
<violation number="1" location="virtio-gpu.c:139">
P1: Missing bounds validation for guest address. The function converts guest addresses to host pointers without checking if `addr` is within valid RAM bounds. This could allow a malicious guest to cause out-of-bounds memory access by providing an invalid address in virtqueue descriptors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,18 @@ | |||
| [binaries] | |||
There was a problem hiding this comment.
P1: @GLOBAL_SOURCE_ROOT@ is not a valid Meson cross-file variable and won't be substituted. The build script copies this file directly without preprocessing. Consider using absolute paths or add a preprocessing step to substitute this placeholder before running meson.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At configs/riscv-cross-file, line 8:
<comment>`@GLOBAL_SOURCE_ROOT@` is not a valid Meson cross-file variable and won't be substituted. The build script copies this file directly without preprocessing. Consider using absolute paths or add a preprocessing step to substitute this placeholder before running meson.</comment>
<file context>
@@ -0,0 +1,18 @@
+ python = '/usr/bin/python3'
+
+[properties]
+ pkg_config_libdir = ['@GLOBAL_SOURCE_ROOT@' / '../buildroot/output/host/riscv32-buildroot-linux-gnu/sysroot/usr/local/lib/pkgconfig',
+ '@GLOBAL_SOURCE_ROOT@' / '../buildroot/output/host/riscv32-buildroot-linux-gnu/sysroot/usr/share/pkgconfig/',
+ '@GLOBAL_SOURCE_ROOT@' / '../buildroot/output/host/riscv32-buildroot-linux-gnu/sysroot/usr/lib/pkgconfig/'
</file context>
| @@ -0,0 +1,4 @@ | |||
| #!/usr/bin/bash | |||
|
|
|||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib | |||
There was a problem hiding this comment.
P2: Prevent LD_LIBRARY_PATH from acquiring a leading empty entry (which adds the current directory to the search path) when the variable is initially unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At target/run.sh, line 3:
<comment>Prevent LD_LIBRARY_PATH from acquiring a leading empty entry (which adds the current directory to the search path) when the variable is initially unset.</comment>
<file context>
@@ -0,0 +1,4 @@
+#!/usr/bin/bash
+
+export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib
+export PATH=$PATH:/usr/local/bin/
</file context>
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib | |
| export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+${LD_LIBRARY_PATH}:}/usr/local/lib |
| cp ../configs/riscv-cross-file . | ||
| ASSERT meson -Ddrmkms=true --cross-file riscv-cross-file build/riscv | ||
| ASSERT meson compile -C build/riscv | ||
| DESTDIR=$BUILDROOT_OUT/host/riscv32-buildroot-linux-gnu/sysroot meson install -C build/riscv |
There was a problem hiding this comment.
P2: Missing ASSERT wrapper on meson install command. If installation fails, the script continues silently, potentially resulting in incomplete package installation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-image.sh, line 190:
<comment>Missing `ASSERT` wrapper on `meson install` command. If installation fails, the script continues silently, potentially resulting in incomplete package installation.</comment>
<file context>
@@ -133,11 +170,72 @@ while [[ $# -gt 0 ]]; do
+ cp ../configs/riscv-cross-file .
+ ASSERT meson -Ddrmkms=true --cross-file riscv-cross-file build/riscv
+ ASSERT meson compile -C build/riscv
+ DESTDIR=$BUILDROOT_OUT/host/riscv32-buildroot-linux-gnu/sysroot meson install -C build/riscv
+ DESTDIR=../../../directfb meson install -C build/riscv
+ popd
</file context>
| res_2d->stride = STRIDE_SIZE; | ||
| res_2d->image = malloc(bytes_per_pixel * (request->width + res_2d->stride) * | ||
| request->height); | ||
| res_2d->image = malloc(res_2d->stride * request->height); |
There was a problem hiding this comment.
The original implementation of my virtio-gpu used pixman. When I attempted to remove the pixman dependency, I introduced a hard-coded STRIDE_SIZE of 4096 as @visitorckw pointed out. After reviewing the VirtIO GPU spec. again, I think this may be incorrect. shengwen-tw@1120d55
In virtio-gpu 2D mode (VIRTIO_GPU_CMD_RESOURCE_CREATE_2D, VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D, and VIRTIO_GPU_CMD_SET_SCANOUT), the protocol does not define an explicit stride field. This suggests that the expected memory layout might be just tightly packed, i.e., each row is exactly width * bytes_per_pixel with no additional per-row padding. If so, explicit stride handling may be unnecessary or even incorrect.
@Mes0903 :
Could you help run some experiments to confirm whether we can remove the stride handling entirely? For example, by checking the size of the received buffer and testing what happens when STRIDE_SIZE is set to 0.
Mes0903
force-pushed
the
vgpu-2D-rework
branch
from
6c8ed02 to
b30842d
Compare
|
Regarding how if (args->bpp != 32)
return -EINVAL;
pitch = args->width * 4;As shown, it pins bits-per-pixel to 32, and the pitch is directly Therefore, I currently follow the approach used by QEMU and ACRN (with pixman), namely applying an additional alignment step on top of this, as a precaution: stride = ((request->width * bits_per_pixel + 0x1f) >> 5) * sizeof(uint32_t);See QEMU implementation and pixman implementation for more details. Also, I recorded my trace process in my blog (in Chinese) |
|
I’ve uploaded bunzip2 Image.bz2 ext4.img.bz2
make check |
|
There are two remaining problem for now:
Did I miss anything? |
This commit adds a minimal virtio-based graphics + input stack by
implementing virtio-gpu in 2D mode and virtio-input for keyboard/mouse.
The goal is to reach a baseline that is sufficient for 2D scanout and
interactive control, while explicitly leaving 3D/blob/virgl acceleration
out of scope for now.
There are multiple ways to provide display/input:
1. Implement the full virtio-gpu 3D/blob path (virgl integration),
which requires a substantially larger surface area and additional
host dependencies.
2. Implement virtio-gpu 2D only, backed by a software renderer and a
host window system, which is enough for DRM dumb buffer style usage
and basic compositing.
3. Provide a non-virtio framebuffer-like device, which would not match
the intended virtio workflow and would not exercise guest virtio
drivers.
This commit adopts (2) to provide the smallest virtio-conformant
baseline that still enables real guest drivers and guest userspace
stacks.
virtio-gpu (2D):
- Add a core virtio-gpu device model with MMIO register handling
('virtio-gpu.c').
- Add a software backend for 2D resources and transfers
('virtio-gpu-sw.c').
- Add a host window path via SDL2 ('window-sw.c', 'window-events.c') to
present the scanout surface and feed input events to virtio-input.
- Generate EDID data so the guest can probe display identification and
modes.
Supported GPU commands (2D path):
- 'GET_DISPLAY_INFO'
- 'RESOURCE_CREATE_2D', 'RESOURCE_UNREF'
- 'SET_SCANOUT'
- 'TRANSFER_TO_HOST_2D'
- 'RESOURCE_FLUSH'
- 'RESOURCE_ATTACH_BACKING', 'RESOURCE_DETACH_BACKING'
- 'GET_EDID'
- 'UPDATE_CURSOR', 'MOVE_CURSOR'
Explicitly not implemented (3D/blob related):
- 'GET_CAPSET_INFO', 'GET_CAPSET', 'RESOURCE_ASSIGN_UUID'
- 'RESOURCE_CREATE_BLOB', 'SET_SCANOUT_BLOB'
The device model is intentionally limited to the 2D command set needed
for a basic DRM/KMS-based flow: create 2D resources, attach guest
backing memory, transfer to host, flush to scanout, and optionally
update/move the cursor.
virtio-input:
- Add virtio-input emulation for keyboard and mouse ('virtio-input.c').
- Provide SDL-to-evdev event mapping and queue update logic with
synchronization to avoid concurrent ring corruption.
- Implement device configuration queries via
'struct virtio_input_config' and 'VIRTIO_INPUT_CFG_*' selectors.
Supported configuration selectors:
- 'VIRTIO_INPUT_CFG_ID_NAME'
- 'VIRTIO_INPUT_CFG_ID_SERIAL'
- 'VIRTIO_INPUT_CFG_ID_DEVIDS'
- 'VIRTIO_INPUT_CFG_PROP_BITS'
- 'VIRTIO_INPUT_CFG_EV_BITS'
- 'VIRTIO_INPUT_CFG_ABS_INFO'
MMIO access rule:
virtio-mmio common registers are restricted to aligned 32-bit accesses,
The device-specific configuration region starts at Config (offset 0x100)
and permits byte/halfword accesses as required by virtio-input
('select', 'subsel', and byte-sized fields).
This prevents guest drivers from faulting when performing per-byte
configuration accesses, while keeping the common register model strict.
QEMU uses a shared virtio-mmio layer to handle sub-32-bit accesses in
the device-specific configuration region and to avoid duplicating MMIO
read/write boilerplate across each virtio device. In contrast, this
project keeps the device model intentionally minimal, so each virtio
device currently implements its own MMIO read/write handlers instead of
introducing a unified MMIO framework.
Build and configuration:
- Add build options for vgpu and an optional DirectFB2 backend.
- Update guest kernel configuration to enable virtio-gpu and
virtio-input.
- Update Buildroot configuration to include dependencies required for
basic DRM userspace (e.g., 'libdrm') and for the host window path
(SDL2).
Testing notes:
- Boot a guest kernel with virtio-gpu and virtio-input enabled.
- Verify that the guest detects a DRM device (virtio-gpu) and an input
device (virtio-input).
- Start a userspace UI stack (e.g., DirectFB2 on top of DRM/KMS) and
confirm:
- a host window is created and shows the guest scanout
- keyboard and mouse events are delivered to the guest userspace
- More specifically, execute 'source ./run.sh', then run DirectFB2
test programs to verify.
References:
- VirtIO Spec v1.3 (virtio-mmio, virtio-gpu, virtio-input)
- QEMU:
- hw/display/virtio-gpu.c
- Linux:
- drivers/virtio/virtio_ring.c
- drivers/virtio/virtio_mmio.c
- drivers/virtio/virtio_pci_common.c
- drivers/gpu/drm/virtio/virtgpu_drv.c
- drivers/gpu/drm/virtio/virtgpu_display.c
- drivers/gpu/drm/virtio/virtgpu_ioctl.c
- drivers/gpu/drm/virtio/virtgpu_vq.c
- drivers/gpu/drm/virtio/virtgpu_object.c
- drivers/gpu/drm/virtio/virtgpu_gem.c
- drivers/gpu/drm/virtio/virtgpu_prime.c
- drivers/gpu/drm/virtio/virtgpu_plane.c
Co-authored-by: Shengwen Cheng <shengwen1997.tw@gmail.com>
Add optional X11 support for Buildroot image building: - Add configs/x11.config with X11/Xorg related packages (mesa3d, xorg7, xserver, xterm, etc.) - Add --x11 flag to build-image.sh to enable X11 package merging - Add copy_buildroot_config function to handle config merging with X11 This allows building a Buildroot image with X11 support using: './scripts/build-image.sh --buildroot --x11' Also, the Xserver can be started by the 'startx' command. The X11 packages are optional and not required for basic virtio-gpu functionality with DirectFB2 or other non-X11 display backends.
Mes0903
force-pushed
the
vgpu-2D-rework
branch
from
b30842d to
5b63aff
Compare
|
I have now moved the X11-related changes into the second commit. The first commit can be tested by DirectFB2 test program. |
5 participants
This pull request is derived from #34 and implements VirtIO-GPU 2D and VirtIO-Input devices for semu, enabling graphics and input support without requiring 3D/VirGL dependencies, in conformance with the VirtIO specification v1.3.
VirtIO-GPU
VirtIO MMIO transport layer(i.e., Linux platform device)VirtIO-Input
Prerequisites
Build kernel image and test rootfs image
$ make rootfs.cpio # Download prebuilt minimal rootfs for initrd $ ./scripts/build-image.sh --all --x11 --external-root --extra-packagesUse
--x11for full setup; omit it for a minimal setup.Run X11
Launch semu:
Run
startxin semuAfter X11 shows up, launch the benchmark programs:
Run DirectFB2
Launch
semu:Kill
X11(Skip if--x11is not used)Set up environment variables via
run.sh:$ source ./run.shRun
DirectFB2test programs:Run kmscube
Launch
semu:Kill X11 (Skip if
--x11is not used)Run
kmscubeRun modetest
The simplest method to test VirtIO-GPU.
Launch
semu:Run
modetestprogramA test image should then show up in the display window.
Summary by cubic
Adds VirtIO-GPU 2D and VirtIO-Input to semu, enabling 2D display and keyboard/mouse without 3D/VirGL. Includes a software window backend and guest build changes to enable DRM and input.
New Features
Migration
Written for commit 5b63aff. Summary will update on new commits.