PCIe, Slots in pcie-root numbered [1,31] #4729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Depends on #4709 |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
6 times, most recently
from
d4f1d32
to
3143809
Compare
|
#4709 was merged. Please rebase. Thanks. |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
4 times, most recently
from
061b796
to
d5ffd30
Compare
|
New tests pass on RHEL 9.2, with the exception of one: The exception is caused by avocado-vt issue avocado-framework/avocado-vt#3630 |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
d5ffd30
to
de4eb57
Compare
|
@dzhengfy Please review. |
dzhengfy
reviewed
Feb 22, 2023
| wipe_devices = "yes" | ||
| test_define_only = "yes" | ||
| controller_address = '{"type": "pci", "domain": "0x0000", "bus": "0x00", "slot": "0x03", "function":"0x0"}' | ||
| - address_slot_max_value: |
There was a problem hiding this comment.
Please remove this test because sata controller now is automatically inserted by libvirt. We need not to test it explicitely.
There was a problem hiding this comment.
Are you sure? Sata is being set up here to test address slot 0x1f. Because pcie-root-port cannot have slot 0x1f. So we're testing the slot not sata controller.
There was a problem hiding this comment.
From the manual case, I saw the test scenario is to test with sata controller attached to pcie-root. Currently this controller is already added by libvirt by default. The pci address is same as you defined. So when I test with your pr, I got double use for that adress. You can check your vm in your environment.
| - address_slot_too_high: | ||
| wipe_devices = "yes" | ||
| test_define_only = "yes" | ||
| controller_address = '{"type": "pci", "domain": "0x0000", "bus": "0x02", "slot": "0x20", "function":"0x0"}' |
There was a problem hiding this comment.
There should be a checking for failure_message
|
|
||
| if test_define_only: | ||
| return | ||
|
|
||
| index = 1 |
There was a problem hiding this comment.
Line 34 , and 35, why not directly to use index = 2?
There was a problem hiding this comment.
Index should be 1. The line below should be deleted. Fixed.
| "controller_index": index, | ||
| 'controller_target': target, | ||
| "controller_addr": str(address)} | ||
| print("contr_dict" + str(contr_dict)) |
There was a problem hiding this comment.
Remove this line or replace with LOG
| contr_object = libvirt.create_controller_xml(contr_dict) | ||
| print(contr_object) |
There was a problem hiding this comment.
Remove this line or replace with LOG
| contr_object = libvirt.create_controller_xml(contr_dict) | ||
| print(contr_object) | ||
| if index == 0: | ||
| contr_object.index = 0 # Workaround for non-working avocado-vt |
|
Could you please provide all test results for |
dzhengfy
reviewed
Feb 22, 2023
| if not check_slot_in_controller(vmxml, index, expected_value, | ||
| slot_equal_after_define): | ||
| test.fail("Controller slot doesn't have the" | ||
| "expected value of %s.", expected_value) | ||
| else: | ||
| libvirt.check_exit_status(cmd_result, status_error) | ||
|
|
There was a problem hiding this comment.
Move line 139 to 147 to Line 150, like:
if failure_message:
libvirt.check_result(cmd_result, [failure_message])
else:
libvirt.check_exit_status(cmd_result, status_error)
if check_slot:
vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(
params.get("main_vm", "avocado-vt-vm1"))
expected_value = address["slot"]
if not check_slot_in_controller(vmxml, index, expected_value,
slot_equal_after_define):
test.fail("Controller slot doesn't have the"
"expected value of %s.", expected_value)
dzhengfy
reviewed
Feb 22, 2023
| # We want to default to None so we can differentiate between None and 0 | ||
| controller_index = params.get("controller_index", None) | ||
| model = params.get("controller_model") | ||
| if controller_index is not None: | ||
| if controller_index == "xxx": |
There was a problem hiding this comment.
xxx? What's this? Did you test it?
There was a problem hiding this comment.
It is related to test pcie_root_port_controller.negative_test.controller_index_invalid. It is related to another polarion case. Check the configuration file.
There was a problem hiding this comment.
This value is random and customized. So to make it more clear, we can also use some customized value, like test, aaa...
There was a problem hiding this comment.
Agree.xxx seems too inofficial. Usually we use a meaningful value like invalid_index
|
Test results from RHEL 9.2: pcie_root_port_controller.negative_test.device_slot_wrong_number fails by virsh define being successful when failure is expected. However when I check the VM XML the related part is set up correctly: So I think the error is in libvirt and not the script. |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
de4eb57
to
9e19bfc
Compare
|
@vasekhodina For that failed case device_slot_wrong_number, please add for that interface device. |
dzhengfy
reviewed
Feb 23, 2023
| @@ -125,14 +135,51 @@ def check_define_vm(vmxml, params): | |||
| libvirt.check_result(cmd_result, [failure_message]) | |||
| else: | |||
| libvirt.check_exit_status(cmd_result, status_error) | |||
| if check_slot: | |||
| libvirt.check_exit_status(cmd_result, status_error) | |||
There was a problem hiding this comment.
Is this duplcated with line 137?
There was a problem hiding this comment.
libvirt.check_exit_status() will be executed twice when check_slot is True.
|
All tests pass now on RHEL 9.2: |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
3 times, most recently
from
aaa4f93
to
63cf311
Compare
|
Tests pass on RHEL 8.8: |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
63cf311
to
1f57f56
Compare
meinaLi
reviewed
Feb 27, 2023
| @@ -190,6 +237,19 @@ def check_vm_start(vm, params, **virsh_options): | |||
| libvirt.check_exit_status(cmd_result, status_error) | |||
|
|
|||
|
|
|||
| def wipe_pcie_root_ports(vmxml): | |||
There was a problem hiding this comment.
Could we use wipe_pcie_controllers(vmxml)? Because I think vmxml.remove_all_device_by_type("controller") in this function is to remove all controllers but not pcie-root-port.
| index = 0 | ||
| contr_dict = {"type": "pci", "model": "pcie-root", "index": index} | ||
| libvirt_vmxml.modify_vm_device(vmxml, "controller", contr_dict, index) | ||
| wipe_pcie_root_ports(vmxml) | ||
|
|
||
| if test_define_only: | ||
| return |
There was a problem hiding this comment.
Just curious, what does this step do?
| # We want to default to None so we can differentiate between None and 0 | ||
| controller_index = params.get("controller_index", None) | ||
| model = params.get("controller_model") | ||
| if controller_index is not None: | ||
| if controller_index == "xxx": |
There was a problem hiding this comment.
This value is random and customized. So to make it more clear, we can also use some customized value, like test, aaa...
| failure_message = "Invalid PCI address .* slot must be <= 0" | ||
| - controllers_same_chassis_same_port: | ||
| second_controller_model = "pcie-root-port" | ||
| second_controller_target = "{'chassis':1,'port':'0x8'}" | ||
| index_offset = 2 |
There was a problem hiding this comment.
So here it means we create a pcie controller with "index='3'" in python file, and the "index='2'" will be generated automatically? For controllers_same_chassis_same_port, which codes means it has the same port?
There was a problem hiding this comment.
See line 6 controller_target = '{"chassis":1,"port":"0x8"}'
|
Test results from RHEL 8.8: |
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
1f57f56
to
276e4f1
Compare
dzhengfy
reviewed
Feb 28, 2023
| @@ -190,6 +238,19 @@ def check_vm_start(vm, params, **virsh_options): | |||
| libvirt.check_exit_status(cmd_result, status_error) | |||
|
|
|||
|
|
|||
| def wipe_pcie_controllers(vmxml): | |||
| """ | |||
| Function that removes all pcie-root-port devices from VM XML | |||
There was a problem hiding this comment.
It seems the docstring also needs update.
| # We want to default to None so we can differentiate between None and 0 | ||
| controller_index = params.get("controller_index", None) | ||
| model = params.get("controller_model") | ||
| if controller_index is not None: | ||
| if controller_index == "xxx": |
There was a problem hiding this comment.
Agree.xxx seems too inofficial. Usually we use a meaningful value like invalid_index
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
276e4f1
to
b8fd91c
Compare
meinaLi
approved these changes
Feb 28, 2023
This commit adds automation of tests where pcie-root-port controllers should have target with slot numbered from 1 to 31 (in hexadecimal numbers). VIRT-55414
vasekhodina
force-pushed
the
LIBVIRTAT-13924
branch
from
b8fd91c
to
ddfebab
Compare
dzhengfy
approved these changes
Feb 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds automation of tests where pcie-root-port controllers should have target with slot numbered from 1 to 31 (in hexadecimal numbers).
VIRT-55414