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

CPU: Add new test case for SVE support. #5272

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

Aakanksha0311
Copy link
Contributor

@Aakanksha0311 Aakanksha0311 commented Nov 13, 2023

This commit adds a new test case to check
the SVE support on the host. The new
test case checks for the absence of SVE
on the host.

Tested with libvirt version 8.0.0

Test results:
avocado run --vt-type libvirt --test-runner=runner --vt-machine-type arm64-mmio aarch64_cpu_sve --vt-connect-uri qemu:///system
(1/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.boot_test.enable_sve: CANCEL: Host doesn't support SVE (27.25 s)
(2/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.boot_test.disable_sve: CANCEL: Host doesn't support SVE (27.34 s)
(3/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.vector_length_test.valid_length.single_vector.sve128: CANCEL: Host doesn't support SVE (29.39 s)
(4/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.vector_length_test.valid_length.single_vector.sve256: CANCEL: Host doesn't support SVE (27.25 s)
(5/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.vector_length_test.valid_length.single_vector.sve512: CANCEL: Host doesn't support SVE (27.37 s)
(6/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.positive_test.vector_length_test.valid_length.mutiple_vector: CANCEL: Host doesn't support SVE (29.32 s)
(7/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.negative_test.vector_length_test.invalid_length: CANCEL: Host doesn't support SVE (27.31 s)
(8/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.negative_test.vector_length_test.conflict_length: CANCEL: Host doesn't support SVE (27.37 s)
(9/9) type_specific.io-github-autotest-libvirt.aarch64_cpu_sve.negative_test.host_sve_check: PASS (30.05 s)

@@ -41,9 +41,15 @@
vector_lenth_list = '[{"sve":"disable"}, {"sve128":"require"}]'
define_error = "yes"
expect_msg = "SVE disabled, but SVE vector lengths provided"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the blank line, thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

only negative_test
check_host_sve = "yes"
status_error = "yes"

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 62 to 69

# Check host sve support
host_sve_check = subprocess.run(
["lscpu | grep sve"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)

if host_sve_check.returncode != 0:
logging.info("Host doesn't support SVE")

Copy link
Contributor

Choose a reason for hiding this comment

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

They can be covered by L71, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I've removed this portion.

test.cancel("Host doesn't support SVE")
else:
return

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to cancel this test if the host supports sve, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, modified it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstood me, what I meant is something as following:

            if process.run(check_sve,
                           ignore_status=True, shell=True).exit_status:
                if not check_host_sve:
                    test.cancel("Host doesn't support SVE")
                else:
                    return
            else:
                if check_host_sve:
                    test.cancel("Host supports SVE")

BTW, the variant 'check_host_sve' doesn't seem appropriate, how about changing it to 'host_no(or without)_sve'? what do you think?

@Aakanksha0311 Aakanksha0311 force-pushed the VIRT-296824 branch 2 times, most recently from 2d46173 to 5d68ae3 Compare November 15, 2023 09:13
test.cancel("Host doesn't support SVE")
else:
return

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstood me, what I meant is something as following:

            if process.run(check_sve,
                           ignore_status=True, shell=True).exit_status:
                if not check_host_sve:
                    test.cancel("Host doesn't support SVE")
                else:
                    return
            else:
                if check_host_sve:
                    test.cancel("Host supports SVE")

BTW, the variant 'check_host_sve' doesn't seem appropriate, how about changing it to 'host_no(or without)_sve'? what do you think?

@@ -41,9 +41,13 @@
vector_lenth_list = '[{"sve":"disable"}, {"sve128":"require"}]'
define_error = "yes"
expect_msg = "SVE disabled, but SVE vector lengths provided"
- host_sve_check:
Copy link
Contributor

Choose a reason for hiding this comment

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

We only cover a test to start vm when host does not support sve cpu flag. so can you update it to a proper name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll update.

BTW, the variant 'check_host_sve' doesn't seem appropriate, how about changing it to 'host_no(or without)_sve'? what do you think?
I think it sounds good to me will update

Copy link
Contributor

@Yingshun Yingshun left a comment

Choose a reason for hiding this comment

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

Others LGTM

ignore_status=True, shell=True).exit_status:
test.cancel("Host doesn't support SVE")

# Cancel test if the Host doesn't support SVE
Copy link
Contributor

Choose a reason for hiding this comment

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

Plz update this comment accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor

@Yingshun Yingshun left a comment

Choose a reason for hiding this comment

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

LGTM

@Yingshun Yingshun requested a review from dzhengfy November 20, 2023 01:07
Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

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

Codes LGTM.
For commit message body, please remove
"It includes modifications to the configuration file and updates to the test script. " because usually we do not need to include how in our commit message. It is formal if we only include what and why in our commit message body. Thanks.

This commit adds a new test case
to check the SVE support on the
host.

Signed-off-by: Aakanksha Tripathi <[email protected]>
@Aakanksha0311
Copy link
Contributor Author

Codes LGTM. For commit message body, please remove "It includes modifications to the configuration file and updates to the test script. " because usually we do not need to include how in our commit message. It is formal if we only include what and why in our commit message body. Thanks.

Yes, understood and removed that portion!

Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

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

LGTM

@dzhengfy dzhengfy merged commit cf2f7f5 into autotest:master Nov 23, 2023
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.

3 participants