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

Jump to bottom

support for shadow ioeventfd #698

Merged
merged 11 commits into from
Jul 4, 2022
Merged

support for shadow ioeventfd #698

merged 11 commits into from
Jul 4, 2022

Conversation

tmakatos
Copy link
Member

Signed-off-by: Thanos Makatos [email protected]

@tmakatos tmakatos requested a review from jlevon June 17, 2022 21:21
@tmakatos
FIXME support for shadow ioeventfd
af4d138 
Signed-off-by: Thanos Makatos <[email protected]>
Change-Id: Iad849c94076ffa5988e034c8bf7ec312d01f095f
@jlevon
Copy link
Collaborator

jlevon commented Jun 20, 2022

should be make this a config option, as it's not something officially supported by anything else?

jlevon
jlevon reviewed Jun 20, 2022
View reviewed changes
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

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

should add some py tests too?

include/libvfio-user.h Outdated Show resolved Hide resolved
lib/libvfio-user.c Outdated Show resolved Hide resolved
@tmakatos
Copy link
Member Author

My primary goal was to get some initial feeback, I think until we hear back from ElenaU about ioregionfd we should leave this unmerged.

@tmakatos
Copy link
Member Author

should be make this a config option, as it's not something officially supported by anything else?

sure

@tmakatos
experimental support for shadow ioeventfd
c273dcb 
When an ioeventfd is written to, KVM discards the value since it has no
memory to write it to, and simply kicks the eventfd. This a problem for
devices such a NVMe controllers that need the value (e.g. doorbells on
BAR0). This patch allows the vfio-user server to pass a file descriptor
that can be mmap'ed and KVM can write the ioeventfd value to this
_shadow_ memory instead of discarding it. This shadow memory is not
exposed to the guest.

Signed-off-by: Thanos Makatos <[email protected]>
Change-Id: Iad849c94076ffa5988e034c8bf7ec312d01f095f
@tmakatos
Copy link
Member Author

@tmakatos tmakatos closed this Jun 23, 2022
@tmakatos tmakatos reopened this Jun 23, 2022
@tmakatos
Copy link
Member Author

should be make this a config option, as it's not something officially supported by anything else?

done

@tmakatos tmakatos requested a review from jlevon June 23, 2022 20:53
@tmakatos
Copy link
Member Author

should add some py tests too?

yep I forgot

@tmakatos
fix broken unit tests
0ecdd26 
Signed-off-by: Thanos Makatos <[email protected]>
Change-Id: Idedbdde5e6315e5ce568cc14d4d6710d8a9af094
@tmakatos
Copy link
Member Author

tmakatos commented Jun 23, 2022 •
edited
Loading

should add some py tests too?

yep I forgot

I'm not sure how to add new tests because we rely on a #define at build time to enable support for shadow ioeventfd, do you know if this is straightforward to do in meson? Alternatively, we could always include support for shadow ioeventfd and enable it during runtime differently, e.g. via an environment variable or setting some flag when creating the context? Or would that be too dangerous?

@jlevon
Copy link
Collaborator

jlevon commented Jun 23, 2022

should add some py tests too?

yep I forgot

I'm not sure how to add new tests because we rely on a #define at build time to enable support for shadow ioeventfd, do you know if this is straightforward to do in meson? Alternatively, we could always include support for shadow ioeventfd and enable it during runtime differently, e.g. via an environment variable or setting some flag when creating the context? Or would that be too dangerous?

It should be a meson option, that then sets a define. then you can look at the meson option to decide whether to run those test cases.

https://mesonbuild.com/Build-options.html

@tmakatos
add unit test
b26918a 
Signed-off-by: Thanos Makatos <[email protected]>
@tmakatos
more test fixes
dcc9638 
Signed-off-by: Thanos Makatos <[email protected]>
@jlevon
Copy link
Collaborator

jlevon commented Jul 1, 2022

will review when tests are passing

@jlevon jlevon closed this Jul 1, 2022
@jlevon jlevon reopened this Jul 1, 2022
@tmakatos tmakatos requested a review from swapnili July 1, 2022 20:30
jlevon
jlevon reviewed Jul 1, 2022
View reviewed changes
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

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

Looks good, just nits. Mind adding a short explanation to docs/ too ?

include/libvfio-user.h Outdated Show resolved Hide resolved
include/libvfio-user.h Outdated Show resolved Hide resolved
lib/libvfio-user.c Outdated Show resolved Hide resolved
test/py/meson.build Outdated Show resolved Hide resolved
test/py/test_shadow_ioeventfd.py Outdated Show resolved Hide resolved
@tmakatos
add doc for ioregionfd
631d1b8 
Signed-off-by: Thanos Makatos <[email protected]>
@tmakatos
Copy link
Member Author

tmakatos commented Jul 4, 2022

Looks good, just nits. Mind adding a short explanation to docs/ too ?

sure

@tmakatos tmakatos requested a review from jlevon July 4, 2022 09:36
jlevon
jlevon reviewed Jul 4, 2022
View reviewed changes
docs/ioregionfd.md Outdated Show resolved Hide resolved
jlevon
jlevon approved these changes Jul 4, 2022
View reviewed changes
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

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

thanks

@tmakatos tmakatos merged commit 36beb63 into master Jul 4, 2022
@tmakatos tmakatos deleted the shadow-ioeventfd branch July 4, 2022 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlevon jlevon jlevon approved these changes

@swapnili swapnili Awaiting requested review from swapnili

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmakatos @jlevon