-
Notifications
You must be signed in to change notification settings - Fork 7.8k
drivers: vhost: Add Xen-MMIO VIRTIO backend #91605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 added project Note: This message is automatically posted and updated by the Manifest GitHub Action. |
drivers/virtio/Kconfig
Outdated
@@ -22,6 +22,21 @@ config VIRTIO_MMIO | |||
help | |||
Enable options for VIRTIO over MMIO | |||
|
|||
config VIRTIO_XEN_MMIO_BACKEND |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have both VIRTIO_XEN_MMIO_BACKEND and XEN_VIRTIO_MMIO_BACKEND :)
s/VIRTIO_XEN_MMIO_BACKEND/XEN_VIRTIO_MMIO_BACKEND/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review.
We will address it gradually.
As you can see, the code is horribly messy at the moment,
and as I said in the title, a lot of updates are still needed.
You might want to wait a little longer to take a proper look.
If anything, this status is an indication that I'm making progress on the tasks, and this is like a report that we have passed the proof-of-concept level.
I think there are some people who are interested in safety features who is interested this.
9451269
to
144315f
Compare
144315f
to
91ba84f
Compare
4741c1a
to
141d60f
Compare
21b1787
to
98d1f30
Compare
d64559b
to
986ef95
Compare
bf1eefc
to
5fed92a
Compare
Add the Xen include files as a module. Signed-off-by: TOKITA Hiroshi <[email protected]>
Currently, the MIT-licensed Xen public headers were incorporated under the `include/zephyr/xen/public`. However, the recent policy is to not include sources other than Apache-2 in the main tree, so we will externalize them. Signed-off-by: TOKITA Hiroshi <[email protected]>
Update Xen module. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add HYPERVISOR_dm_op hypercall implementation for ARM64 architecture. This hypercall is required for device model operations in Xen virtualization environment. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add more hypercall wrappers for supporting virtio-mmio backend. - dmop - dmop_create_ioreq_server HYPERVISOR_dm_op(XEN_DMOP_create_ioreq_server) - dmop_map_io_range_to_ioreq_server HYPERVISOR_dm_op(XEN_DMOP_map_io_range_to_ioreq_server) - dmop_set_ioreq_server_state: HYPERVISOR_dm_op(XEN_DMOP_set_ioreq_server_state) - dmop_nr_vcpus: HYPERVISOR_dm_op(XEN_DMOP_nr_vcpus) - dmop_set_irq_level: HYPERVISOR_dm_op(XEN_DMOP_set_irq_level) - memory - xendom_acquire_resource: HYPERVISOR_memory_op(XENMEM_acquire_resource) - sched - sched_poll: HYPERVISOR_sched_op(SCHEDOP_poll) Signed-off-by: TOKITA Hiroshi <[email protected]>
Add grant table functionality for Xen virtualization support. Implements grant table setup, mapping, and reference management for memory sharing between domains. Signed-off-by: TOKITA Hiroshi <[email protected]>
Move macros derived from the virtio specification to a shared include. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add functions for accessing XenStore. Currently not support write feature. Signed-off-by: TOKITA Hiroshi <[email protected]>
This introduces the vhost driver framework, providing standard APIs for VIRTIO backend implementations in Zephyr. Includes vringh utility for host-side VIRTIO ring processing based on Linux kernel implementation. Signed-off-by: TOKITA Hiroshi <[email protected]>
Implements VirtIO backend over Xen MMIO interface Signed-off-by: TOKITA Hiroshi <[email protected]>
Add sample application demonstrating vhost driver usage for VIRTIO backend implementations. Includes basic setup and configuration examples for Xen MMIO VirtIO backend. Signed-off-by: TOKITA Hiroshi <[email protected]>
5fed92a
to
9376df3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for proposed changes and your activity, I appreciate that and will try to assist you with advises.
Unfortunately, I need to pause my review on following commit - "drivers: xen: add basic XenStore access features" due to a lack of time. I strongly suggest you to divide this PR in few smaller. Some of the changes may be easily merged already if it will have a good explanation.
I see no major issues with reviewing and merging this changes step by step:
- dm_op
- adding wrapper functions
- gnttab change (if you will explain why it is actually needed right inside a driver)
- Virtion config
- Vhost
- Virtio MMIO
- sample
Regarding xenstore.c
- what you implemented actually called xenbus (xenstore - aka "filesystem" and server for Dom0, "xenbus" - interface between domains through the ring buffer). It is very complex thing and require a lot of review, I believe it should be done in separate PR.
FYI regarding xenstore (for Zephyr Dom0): after some discussions we decided to move it to separate project instead of having it as a driver (and potentially move it to userspace) - please check zephyr-xenlib/xenstore-srv
@@ -6,13 +6,18 @@ | |||
#ifndef ZEPHYR_INCLUDE_ARCH_ARM64_HYPERCALL_H_ | |||
#define ZEPHYR_INCLUDE_ARCH_ARM64_HYPERCALL_H_ | |||
|
|||
#include <xen/public/xen.h> | |||
|
|||
struct xen_dm_op_buf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point for this one? I believe there should be just #include <zephyr/xen/dmop.h>
on the top of the file
@@ -23,6 +23,7 @@ HYPERCALL(sched_op); | |||
HYPERCALL(event_channel_op); | |||
HYPERCALL(hvm_op); | |||
HYPERCALL(memory_op); | |||
HYPERCALL(dm_op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic comment regarding commit message for arch: arm64: core: xen: hypercall: Add dm_op hypercall
:
You need to not only describe what was added, but also explain why it is needed in Zephyr and how it will be used.
int dmop_map_io_range_to_ioreq_server(domid_t domid, ioservid_t id, uint32_t type, uint64_t start, | ||
uint64_t end); | ||
|
||
int dmop_unmap_io_range_from_ioreq_server(domid_t domid, ioservid_t id, uint32_t type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one has missing documentation comment. I understand that this and previous are opposite to each other, but when docs will be generated it will look strange.
*/ | ||
int dmop_create_ioreq_server(domid_t domid, uint8_t handle_bufioreq, ioservid_t *id); | ||
|
||
int dmop_destroy_ioreq_server(domid_t domid, ioservid_t id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this one(as for dmop_unmap_io_range_from_ioreq_server)
@@ -5,5 +5,7 @@ zephyr_sources(hvm.c) | |||
zephyr_sources(events.c) | |||
zephyr_sources_ifdef(CONFIG_XEN_GRANT_TABLE gnttab.c) | |||
zephyr_sources(memory.c) | |||
zephyr_sources(sched.c) | |||
zephyr_sources(dmop.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall comment for commit drivers: xen: add more hypercall wrapper
:
I strongly suggest you to split this commit into 3 separate commits and add more description regarding how it will be used in further development. Also, I believe that sched.c
and dmop.c
should be guarded with separate CONFIG to have a possibility to disable it.
void gnttab_put_page(void *page_addr) | ||
void *gnttab_get_page(void) | ||
{ | ||
return gnttab_get_pages(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like half-way change, I believe if plural form is actually needed, that it is better to fix it everywhere and not leave things like this.
reservation.nr_extents = nr_extents; | ||
set_xen_guest_handle(reservation.extent_start, &page); | ||
|
||
ret = HYPERVISOR_memory_op(XENMEM_populate_physmap, &reservation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hypercall supports multiple pages out of the box, you don't need a loop here. Please, pay attention to nr_extents
field, it can be done it 1 action.
@@ -286,8 +305,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops, unsigned int cou | |||
return HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap_ops, count); | |||
} | |||
|
|||
|
|||
static const char * const gnttab_error_msgs[] = GNTTABOP_error_msgs; | |||
static const char *const gnttab_error_msgs[] = GNTTABOP_error_msgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style fixes should be in separate commit.
@@ -50,13 +50,17 @@ int32_t gnttab_alloc_and_grant(void **map, bool readonly); | |||
*/ | |||
void *gnttab_get_page(void); | |||
|
|||
void *gnttab_get_pages(unsigned int npages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said previously, I believe that user should see only 1 form for this (and it should have a documentation block above)
@@ -7,6 +7,7 @@ | |||
#include <zephyr/logging/log.h> | |||
#include <zephyr/sys/byteorder.h> | |||
#include <zephyr/drivers/virtio.h> | |||
#include <zephyr/drivers/virtio/virtio_config.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment for drivers: virtio: Separate macros into a common header
:
I am not responsible for this part of code, but I have no idea why did you change it, commit message definitely should describe it better.
This PR introduces the VHost driver framework for Zephyr, enabling VIRTIO backend implementations with initial support for Xen hypervisor environments.
This means we can use(create) Zephyr as VIRTIO devices in a Xen (and potentially other hypervisors).
VHost Driver Framework
Standard VHost API: Unified interface for VirtIO backend implementations
vringh Integration: Host-side VirtIO ring handling based on Linux kernel design
Xen MMIO VIRTIO Backend
Implementing VIRTIO-MMIO in a Xen environment using the VHost driver framework
Enhanced Xen Infrastructure
Some enhancements of Xen functions
XenStore Integration: Key-value store access for domain configuration
Add more hypercall wrapper: DM-op, scheduler, ...
note:
This pr needs #91643 finished.