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.

Sign up for GitHub

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

Fix virtIO MMIO QueueNotify field #36

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix virtIO MMIO QueueNotify field #36

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/virtio/mmio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ static bool handle_virtio_mmio_reg_write(virtio_device_t *dev, size_t vcpu_id, s
}
break;
case REG_RANGE(REG_VIRTIO_MMIO_QUEUE_NOTIFY, REG_VIRTIO_MMIO_INTERRUPT_STATUS):
dev->data.QueueNotify = (uint32_t)data;
Copy link
Collaborator

@Ivan-Velickovic Ivan-Velickovic Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm understanding correctly we should be able to do this:

Suggested change
dev->data.QueueNotify = (uint32_t)data;
/* We do not support VIRTIO_F_NOTIFICATION_DATA so the data will always be the queue index. */
dev->data.QueueNotify = (uint32_t)data;

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better if we could assert VIRTIO_F_NOTIFICATION_DATA is not negotiated, I cannot remember if this is information we can get from the MMIO implementation.

Copy link
Contributor Author

@alexandermbrown alexandermbrown Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Negotiation happens in the actual driver implementations with get_device_features and set_driver_features. We could either trust devices to not set VIRTIO_F_NOTIFICATION_DATA or check / mask. the result of get_device_features

// mmio.c: handle_virtio_mmio_reg_read
case REG_RANGE(REG_VIRTIO_MMIO_DEVICE_FEATURES, REG_VIRTIO_MMIO_DEVICE_FEATURES_SEL):
    success = dev->funs->get_device_features(dev, &reg);
    reg &= ~BIT_HIGH(VIRTIO_F_NOTIFICATION_DATA);
    // could mask out other unsupported features...
    break;

success = dev->funs->queue_notify(dev);
break;
case REG_RANGE(REG_VIRTIO_MMIO_INTERRUPT_ACK, REG_VIRTIO_MMIO_STATUS):
Expand Down
Loading