Skip to content

virtio: support dynamic device add - #103

Merged
agicy merged 1 commit into
syswonder:mainfrom
Jaxtonmax:virtio-dynamic-add-clean
Aug 24, 2026
Merged

agicy merged 1 commit into
syswonder:mainfrom
Jaxtonmax:virtio-dynamic-add-clean

Conversation

@Jaxtonmax

Copy link
Copy Markdown

virtio: support dynamic device add

Summary

Closes #57.

This PR adds a virtio add flow so new Virtio device configurations can be sent to an already running hvisor-tool Virtio backend daemon.

Main changes:

  • Add hvisor virtio add <virtio.json> CLI support.
  • Add a Unix domain socket control path at /run/hvisor-virtio.sock.
  • Let virtio add act as a client while the running daemon performs the actual state mutation.
  • Reuse the virtio start JSON parsing, memory mapping, validation, and device creation path for later add requests.
  • Split device creation from publishing to vdevs[].
  • Stage all enabled devices first, then publish them together only after all devices in the add request are created successfully.
  • Clean up unpublished devices on failure, including blk, console, net, gpu, virtqueue, fd, event, worker thread, mutex, and cond resources where applicable.
  • Document the new workflow in README.md and README-zh.md.

New Workflow

The existing one-shot startup flow is still supported:

nohup ./hvisor virtio start virtio_cfg.json &
./hvisor zone start zone1_linux.json

The new flow can add another Virtio configuration to the running daemon:

nohup ./hvisor virtio start virtio_start_empty.json &
./hvisor virtio add virtio_add_console_blk.json
./hvisor zone start zone1_linux.json

For MMIO Virtio devices, virtio add should be run before starting the zone that will use the newly added devices, unless full guest runtime hotplug is separately verified.

Implementation Notes

  • virtio add connects to the daemon through /run/hvisor-virtio.sock.
  • The daemon owns shared state such as vdevs[], zone_mem[], event monitor state, and device-specific resources.
  • VDEV_MUTEX protects global device table publication and MMIO address publication.
  • ZONE_MEM_MUTEX protects zone memory mapping records.
  • Device creation is staged:
    • create unpublished devices into a local list
    • if any device fails, destroy staged devices and return failure
    • if all devices succeed, publish them into vdevs[] together
  • Device cleanup paths now tolerate partially initialized state:
    • blk checks worker thread, image fd, mutex, and cond state
    • console removes epoll event and closes pty fds only when initialized
    • net removes epoll event and closes tap fd only when initialized
    • gpu records DRM/thread/synchronization state for safer cleanup

Validation

Runtime validation performed on QEMU AArch64 with QEMU 9.0.1:

  • Started root Linux on QEMU AArch64.
  • Started an empty Virtio daemon.
  • Ran virtio add with console + wrong blk:
    • observed virtio add failed
    • confirmed no console pty residue was left in /dev/pts
  • Ran virtio add with console + valid blk:
    • observed virtio add succeeded
    • started zone1 successfully
    • entered zone1 console
    • confirmed blk was accessible in the tested zone1 flow
  • Also manually checked the old virtio start style flow after updating the hvisor side.

Static/local checks performed during the branch work:

git diff --check upstream/main...HEAD

Notes

  • The runtime validation in this round focuses on blk + console.
  • net and gpu normal runtime paths are not fully covered by this validation; their changes are mainly initialization-failure and cleanup-path hardening.

@Jaxtonmax

Copy link
Copy Markdown
Author

@Inquisitor-201 @ForeverYolo Could you help review this PR when you have time? This PR mainly addresses #57 by adding dynamic Virtio device add support. Thanks!

@agicy agicy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is ~45 commits behind upstream main. Since your branch was created, upstream has merged several things that touch the same files:

  • PR #102 (perf/virtio-net): rewrote the net RX path (process_descriptor_chain_buf, update_used_ring_batch, IFF_VNET_HDR), added status_changed hook
  • PR #100 (refactor/zone-mem-struct): changed zone memory struct layout
  • PR #84 (feat-virtio-scmi): SCMI protocol support in both driver and tools

After rebase, the net event handler, SCMI handling, and status_changed hook will all be consistent with current upstream. The per-device close hardening and the staging pattern (unpublished -> publish) will still apply cleanly - those changes don't conflict with upstream.

Comment thread tools/virtio/virtio.c
Comment thread tools/virtio/virtio.c Outdated
Comment thread tools/virtio/virtio.c
@agicy

agicy commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Looking at the code, there are 4 separate switch-case / if-else blocks that dispatch on device type:

  • create_virtio_device_unpublished() - per-device init
  • destroy_unpublished_virtio_device() - per-device close
  • init_virtio_queue() - per-device queue setup
  • create_virtio_device_from_config() - arg0/arg1 construction

Plus parse_virtio_device_config() which has per-device JSON parsing. Each new device type requires touching all of them, and they'll inevitably drift.

I think we should consolidate into ops table:

// Runtime dispatch - used throughout the daemon's lifetime
struct virtio_dev_ops {
    VirtioDeviceType type;
    uint64_t features;
    uint32_t num_queues;
    uint32_t queue_max_size;
    int  (*init)(VirtIODevice *vdev, void *params);
    void (*close)(VirtIODevice *vdev);
    void (*reset)(VirtIODevice *vdev);
    void (*status_changed)(VirtIODevice *vdev, uint32_t status);
    int  (*notify_handlers[VIRTIO_MAX_VQUEUES])(VirtIODevice *, VirtQueue *);
};

// Config parsing - only used during virtio_start / virtio_add JSON parsing
struct virtio_config_ops {
    int  (*parse)(cJSON *json, void **params_out);
    void (*free_params)(void *params);
};

With a clear contract:

  • init() returns 0 on success, negative on failure, and does not self-clean - the caller always calls close() afterward regardless of outcome.
  • close() is idempotent - safe after any init outcome (full success, partial, or init never called). The bool flags you added (mutex_initialized, cond_initialized, thread_started) already follow this pattern.

Then the switch-case blocks collapse to:

// create_virtio_device_unpublished
const struct virtio_dev_ops *ops = lookup_dev_ops(dev_type);
vdev->regs.dev_feature = ops->features;
if (ops->init(vdev, params) != 0) goto err;
if (init_virtio_queue(vdev, ops) != 0) goto err;

// destroy_unpublished_virtio_device
vdev->virtio_close(vdev);  // each device sets this = ops->close

// init_virtio_queue
vdev->vqs_len = ops->num_queues;
for (i = 0; i < ops->num_queues; i++) {
    vqs[i].queue_num_max = ops->queue_max_size;
    vqs[i].notify_handler = ops->notify_handlers[i];
    ...
}

// JSON parsing in virtio_start / virtio_add path
const struct virtio_config_ops *cfg_ops = lookup_config_ops(dev_type);
void *params = NULL;
if (cfg_ops->parse(device_json, &params) != 0) return -1;
// ... later: cfg_ops->free_params(params);

Your staging architecture - create_virtio_device_unpublished -> publish_virtio_devices, all-or-nothing, clean-only-once - stays exactly as you designed it. The ops table just replaces how each device-specific step is dispatched.

I already have a draft of this on a branch. If the direction looks right to you, I'll push it so you can rebase on top of it. Curious what you think.

@Jaxtonmax

Jaxtonmax commented Aug 4, 2026

Copy link
Copy Markdown
Author

@agicy Thanks for the detailed review. The latest upstream changes and the control-thread shutdown fix have been pushed to the PR branch. I am reviewing the updated diff and rerunning the tests now. I will reply to each point with the detailed results after the review is complete.
The ops-table direction looks reasonable to me. Since you already have a draft, please push the branch when convenient so I can review how it interacts with the staged creation and cleanup logic. Before I make further structural changes, could you also clarify whether the config union, ops table, and virtio.c split are expected in PR #103 or can be handled in follow-up PRs?

@agicy

agicy commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@Jaxtonmax Thanks for your quick fix.

The ops-table draft is up at #112. It currently includes the commits we discussed - init/close contract, lifecycle ops table, and the virtio_config_ops for JSON parsing.

That PR depends on refactor/virtio-scmi landing first, so it carries the SCMI refactoring commits as its base. Once refactor/virtio-scmi is merged into upstream/main, I'll rebase the ops-table PR to a clean linear history.

@Jaxtonmax

Copy link
Copy Markdown
Author

Thanks for the clarification. I understand that the ops-table and related lifecycle/config refactoring are being handled in PR #112.

I will keep PR #103 focused on the dynamic-add implementation, staged creation, failure cleanup, and control-thread shutdown fix.

The QEMU AArch64 validation for PR #103 covers the blk + console paths, including failed add rollback, successful add, zone startup, legacy virtio start, and daemon shutdown. Net and SCMI runtime validation are not covered by the current QEMU configuration because they require separate TAP/bridge and SCMI platform setups.

I have completed the review of the updated diff and will keep the remaining changes scoped to PR #103. Please re-review it when convenient.

@Jaxtonmax
Jaxtonmax force-pushed the virtio-dynamic-add-clean branch from ae462b7 to 7464cfd Compare August 24, 2026 03:24

@agicy agicy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@agicy
agicy merged commit bab6d63 into syswonder:main Aug 24, 2026
1 check passed
agicy added a commit that referenced this pull request Aug 24, 2026
agicy added a commit that referenced this pull request Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor virtio initialization to support dynamic device attachment

2 participants