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

HPt630 tests #2

Open
wants to merge 33 commits into
base: 3mdeb-lab
Choose a base branch
from
Open

HPt630 tests #2

wants to merge 33 commits into from

Conversation

philipandag
Copy link
Collaborator

No description provided.

Perform installation followed by AEM testing:

```
openqa-cli api -X POST isos DISTRI=qubesos VERSION=4.2 ARCH=x86_64 BUILD=4.2.0 FLAVOR=install-iso-optiplex
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to use versions we're testing it with, and also, optiplex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, it was so long since I copy-pasted this README from optiplex that I forgot that it still has to be changed. I'll look through the whole generalhw/hpt630 again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

* `PACKAGES_BASE_URL` - where to look for AEM-related packages.
* `AEM_VER` - version of `anti-evil-maid` package
* `GRUB_VER` - version of `grub2-*` packages
* `XEN_VER` - version of `xen-*` packages
Copy link
Member

Choose a reason for hiding this comment

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

Missing SKL_VER.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests/aem_hw.pm Outdated
@@ -50,17 +50,20 @@ use Data::Dumper;
# they are generally machine-specific, so don't assume anything
my $boot_disk;
my $boot_part;
if (check_var('MACHINE', 'optiplex') || check_var('MACHINE', 'supermicro')) {
if ((check_var('MACHINE', 'optiplex') || check_var('MACHINE', 'supermicro') || check_var('MACHINE', 'hpt630v1')) and (check_var('OS_INSTALL_LEGACY', '1'))) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be moved below DRTM and BIOS kind checks, and then we could test just OS_INSTALL_LEGACY value, as all other platforms would be filtered out by BIOS/DRTM checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests/aem_hw.pm Outdated Show resolved Hide resolved
# echo "$0: not doing anything"
# exit 0

function sonoff_pwr() {
Copy link
Member

Choose a reason for hiding this comment

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

We probably should add some delay after power off.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't encountered any issues with powering the platform on/off, but it surely won't hurt.
It's important to not add any delay when powering on, or the screen with the HP logo might not get registered

https://github.com/3mdeb/openqa-tests-qubesos/compare/52d01ba%5E..52d01ba


These settings can be added to `openqa-cli` posting command to specify which
packages to use. Current default uses
<https://dl.3mdeb.com/open-source-firmware/QubesOS/trenchboot_aem_v0.3/>.
Copy link
Member

Choose a reason for hiding this comment

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

Update according to whatever will be used in template. The same may be needed for other platforms, so maybe this should be made generic and point to template so we won't have to bump it with each new version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/3mdeb/openqa-tests-qubesos/compare/c4107f9%5E..c4107f9

I am not sure about MSI. The README and the instructions to start the tests are different than for other platforms.
I think it should probably still be fine to override the parameters like in the case of the other platforms.


# allow all USB devices, this setting seems to appear on first boot, hence
# the update is performed here
sed -i -e 's/authorized_default=0/authorized_default=1/' /boot/grub2/grub.cfg /etc/default/grub
Copy link
Member

Choose a reason for hiding this comment

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

For this platform, we should also add spec-ctrl=no-ibpb-entry to Xen cmdline, otherwise VMs may not be able to start before timeout under some conditions. It will also make it much faster, and for testing we care more about time than about security implications this option has.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@krystian-hebel krystian-hebel Feb 5, 2025

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 escape $ so it won't be parsed by shell executing echo. Maybe putting the whole string in single quotes and unescaping inner double quotes instead would be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -350,18 +452,31 @@ sub setup_aem {
# cleanup in case AEM was previously initialized (useful for debugging this test)
Copy link
Member

Choose a reason for hiding this comment

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

(comment to code few lines above, but GH won't allow me to add it there)

Currently, install_packages fails on dnf reinstall step, because there is nothing to reinstall. Ideally we shouldn't use dnf directly, instead relying on qubes-dom0-update, as was done in blog posts. There still may be some install vs reinstall issues, and I don't know if assert_script_run is enough to catch all problems, like missing packages or version conflicts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

assert_script_run returns the command output, so there is no reason not to check it and use it to decide on fail/pass.
I'll see if using qubes-dom0-update helps and if there are some characteristic output meaning a success/fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@krystian-hebel
In the blog post we first update dom0 before installing anything. The tests are not doing that. Should a general update to the whole dom0 be added?

Copy link
Member

Choose a reason for hiding this comment

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

This is worth considering, but I don't think we should do it here and now. Let's focus on getting it to work in the first place.

Copy link
Member

@krystian-hebel krystian-hebel left a comment

Choose a reason for hiding this comment

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

Do we need v1 in the platform name? Ideally we should reuse the same setup for both platforms.

cmd=$(cut -d "p" -f2 <<< "$cmd" )
post_params="cmnd=Power $cmd"
echo "Sending sonoff POST request: $post_params\n"
curl -X POST http://192.168.10.157/cm -d "$post_params" && echo "\n"
Copy link
Member

Choose a reason for hiding this comment

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

Hardcoded IP, perhaps we should pass it as $1 in place of $rte_ip, AFAICT we're not using it for anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. Now I see that, for example, supermicro also has a different meaning of the first parameter. It's a good idea then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Signed-off-by: Filip Gołaś <[email protected]>
(It's really slow...)

Signed-off-by: Filip Gołaś <[email protected]>
@philipandag
Copy link
Collaborator Author

Do we need v1 in the platform name? Ideally we should reuse the same setup for both platforms.

The two platforms (v1 and v2) have different versions of TPM chips. Other than that I think they are identical, and the tests don't need to take that into account. If it is not important to clearly differentiate them when scheduling / presenting the test results, then I think this suffix can be removed without problems.

Co-authored-by: Krystian Hebel <[email protected]>
Signed-off-by: Filip Gołaś <[email protected]>
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