-
Notifications
You must be signed in to change notification settings - Fork 5
Add a test that ensures correct BDF assignment #68
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
phip1611
left a comment
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.
Generally great work! Left a few remarks
b23fb51 to
4aac2f1
Compare
|
This is great work, can we also add 2 hotplug devices:
|
hertrste
left a comment
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.
We well written test case 👍
phip1611
left a comment
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.
I like the direction into that we are going here - good work! I left a few remarks
8089e2d to
9b27e1b
Compare
phip1611
left a comment
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.
LGTM but I left a few remarks. I'm already approving.
|
You need to rebase your PR. |
240f603 to
8654c40
Compare
tests/common.nix
Outdated
| <target dev='tap0'/> | ||
| <model type='virtio'/> | ||
| <driver queues='1'/> | ||
| all_static_bdf ? false |
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.
all_static_bdf ? false
this looks lile a oopsie and will land as is in the XML string.
It surprises me that libvirt doesnt fail
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.
Good Catch! Fixed.
|
@scholzp please resolve conflicts |
We don't use the DHCP anymore, so we can deactivate it. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
We need to specify slot IDs in the XML from which the VM config for CHV is created. This is in preparation for testing BDF fixing. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
When evaluating PCI BDF assignment we need a way to query these information from a test VM. This commit adds a handy way to obtain all BDFs with their assigned devices. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Some test, as the anticipated tests for BDF assignment might need to do some additional cleanup after terminating that is specific for this test only. This also needs to work reliably in case of tests failing. This commit therefore introduces infrastructure that allows to define some 'guard', that once garbage collected, executes the cleanup routines. Signed-off-by: Pascal Scholz pascal.scholz@cyberus-technology.de On-behalf-of: SAP pascal.scholz@sap.com
|
Please fix the ci. Are all issues in the corresponding libvirt pr now resolved? |
c038d6b to
68b98a1
Compare
Yes, they are. |
The added tests checks that libvirt and CHV assigned and propagate PCI BDF correctly to the guest's PCI devices. It furthermore ensures, that these assignment stay valid even after live migration. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
We test that the persistent config stores the correct BDF. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
This test ensures that BDFs stored in the live and the persistent config don't go out of sync when transiently removing a device. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
We need to test that an invalid BDF results in an error from libvirt and does not crash CHV. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
We need a negative test that shows, that adding a function ID when entering a BDF result in libvirt generating a failure and CHV not crashing. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
|
I just added negative tests for:
Both are expected to be blocked and errrored upon by libvirt. |
|
Great work! |
| Test that a BDFs containing a function ID leads to errors. | ||
| """ | ||
| # We don't support multi function devices currently | ||
| controllerVM.fail("virsh define /etc/domain-chv-static-bdf-with-function.xml") |
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 file doesn't exist?!
This test adds checks for verifying correct BDF assignment through libvirt and that CHV respects those assignments, even after live migration. Moreover, this test ensures that BDFs can be freed and reused when de-/attaching devices (might be more of a theoretical feature?).
For further details, see the test description.
Related to https://github.com/cobaltcore-dev/cobaltcore/issues/287.