Skip to content
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

Refactor VirtIO MMIO register map handling #23

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

shengwen-tw
Copy link
Collaborator

Two adjustments are conducted in this pull request:

  1. Replace the hard-coded register address checking with enum.
  2. Handle the configuration space read / write with device-specific structures.

virtio.h Outdated Show resolved Hide resolved
virtio.h Outdated Show resolved Hide resolved
@shengwen-tw shengwen-tw force-pushed the master branch 2 times, most recently from e6075d2 to 8d69110 Compare July 26, 2023 14:21
virtio.h Outdated Show resolved Hide resolved
@shengwen-tw shengwen-tw force-pushed the master branch 3 times, most recently from 51c542d to 5f1e37c Compare July 26, 2023 17:47
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Move virtio-net and virtio-blk specific structure definitions from virtio.h to dedicated C source files.

Apply the trick opaque pointer to hide the implementation details. That is, consider the following changes:

--- a/device.h
+++ b/device.h
@@ -144,14 +144,14 @@ typedef struct {
     /* queue config */
     uint32_t QueueSel;
     virtio_blk_queue_t queues[2];
-    /* device configuration */
-    struct virtio_blk_config config;
     /* status */
     uint32_t Status;
     uint32_t InterruptStatus;
     /* supplied by environment */
     uint32_t *ram;
     uint32_t *disk;
+    /* implementation-specific */
+    void *priv;
 } virtio_blk_state_t;
 
 void virtio_blk_read(vm_t *vm,
--- a/virtio-blk.c
+++ b/virtio-blk.c
@@ -21,6 +21,46 @@
 #define VBLK_QUEUE_NUM_MAX 1024
 #define VBLK_QUEUE (vblk->queues[vblk->QueueSel])
+
+#define PRIV(x) ((struct virtio_blk_config *) x->priv)
+
 static void virtio_blk_set_fail(virtio_blk_state_t *vblk)
 {
     vblk->Status |= VIRTIO_STATUS__DEVICE_NEEDS_RESET;
@@ -45,11 +85,11 @@ static void virtio_blk_update_status(virtio_blk_state_t *vblk, uint32_t status)
     /* Reset */
     uint32_t *ram = vblk->ram;
     uint32_t *disk = vblk->disk;
-    uint32_t capacity = vblk->config.capacity;
+    uint32_t capacity = PRIV(vblk)->capacity;
     memset(vblk, 0, sizeof(*vblk));
     vblk->ram = ram;
     vblk->disk = disk;
-    vblk->config.capacity = capacity;
+    PRIV(vblk)->capacity = capacity;
 }
 
 static void virtio_blk_write_handler(virtio_blk_state_t *vblk,

Of course, you have to allocate the necessary memory for the region pointed by priv member.

@shengwen-tw
Copy link
Collaborator Author

Should we also add destructor functions to clean up the allocated memory (i.e., vnet->priv and vblk->priv) explicitly even if the program will only be terminated by pressing Ctrl+A then X?

virtio-blk.c Outdated Show resolved Hide resolved
@jserv
Copy link
Collaborator

jserv commented Jul 27, 2023

Should we also add destructor functions to clean up the allocated memory (i.e., vnet->priv and vblk->priv) explicitly even if the program will only be terminated by pressing Ctrl+A then X?

Yes, you can do it in next pull request(s).
Follow the way how kvm-host does.

virtio-blk.c Outdated Show resolved Hide resolved
@sysprog21 sysprog21 deleted a comment from shengwen-tw Jul 27, 2023
@shengwen-tw shengwen-tw force-pushed the master branch 2 times, most recently from d810c12 to ed080c2 Compare July 27, 2023 02:58
virtio-blk.c Outdated Show resolved Hide resolved
virtio-net.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase the latest master branch and resolve the conflicts.

virtio-blk.c Outdated Show resolved Hide resolved
virtio-net.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Improve the git commit message. This commit addresses the following:

  1. Re-organizes the source structures to improve code organization and maintainability.
  2. Introduces the use of opaque pointer for information hiding, enhancing encapsulation and abstraction.
  3. Enforces mnemonic enums instead of numbers, enhancing code readability and maintainability.

By incorporating these changes, the codebase becomes more structured and easier to understand and maintain.

Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase the latest master branch.

This commit addresses the following:

1. Re-organizes the source structures to improve code organization and maintainability.
2. Introduces the use of opaque pointer for information hiding, enhancing encapsulation and abstraction.
3. Enforces mnemonic enums instead of numbers, enhancing code readability and maintainability.

By incorporating these changes, the codebase becomes more structured and easier to understand and maintain.
@jserv jserv merged commit 5eb7023 into sysprog21:master Jul 27, 2023
2 checks passed
@jserv
Copy link
Collaborator

jserv commented Jul 27, 2023

Thank @shengwen-tw for contributing!

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.

2 participants