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

Fix virtIO MMIO QueueNotify field #36

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

alexandermbrown
Copy link
Contributor

The QueueNotify field in the MMIO Device Register Layout was previously not set.

@alexandermbrown alexandermbrown self-assigned this Jan 17, 2024
@@ -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

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

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;

@Ivan-Velickovic Ivan-Velickovic merged commit aa2d3b3 into main Jan 30, 2024
5 checks passed
@Ivan-Velickovic Ivan-Velickovic deleted the alexbr/fix_virtio_queue_notify branch March 25, 2024 02:11
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