engine: aarch64 support - #1041
Conversation
dupondje
left a comment
There was a problem hiding this comment.
First of all thanks for this patch. I think this is a great addition! :)
I added some remarks in the code already. Think it's a very good start, just some things we need to improve.
Did you already had a running Arm VM in oVirt? Or not there yet?
I think it might be useful to have some full libvirt xml of the started VM also.
To see if everything is in there, and if we need to make some improvements.
Of course, we had manual installations with ubuntu, centos, sles and rhel.
See attached file |
201e924 to
f8746f7
Compare
|
Is there anything blocking this PR or can we get it merged? According to https://www.ovirt.org/develop/dev-process/install-nightly-snapshot.html
it would be great to reduce the effort for the enduser and get some feedback from the community. |
There was a problem hiding this comment.
Sorry for the delay.
We are currently focussing on getting everything build for CentOS 10, which takes most of our time.
Getting this in would be very nice, but I want to make sure it's (almost) perfect before we merge it.
I added some additional comments when I reviewed the code.
Next to that, I also found some old patch the openkylin project did to support ARM on oVirt:
https://gitee.com/openkylin-backup/ovirt-engine/pulls/1/files
It would also be nice if we could get rid of the TODO's before we merge it.
Cause 'TODO' comments are good, but they will be forgotten and it will stay unimplemented,
So I think it would be nice to have them correct before getting merged.
A last point of attention is the CPU type we should pick for ARM and which flags should be used. I don't know on which CPU you are testing this?
Next to that it could be nice if you joined our Matrix channel. Then we can discuss this easily :)
| int powerOf2 = Integer.highestOneBit(maxRam); | ||
| pageTable = (maxRam > powerOf2 ? powerOf2 * 2 : powerOf2) / 64; | ||
| } else { | ||
| // TODO, aarch64 |
There was a problem hiding this comment.
What still has to be fixed here?
There was a problem hiding this comment.
Looks like the current overhead calculation is a good guess.
This is what I can see from the two active VMs on different architectures:
aarch64:
oVirt: 48G = 49152 MB
VM /proc/meminfo: MemTotal: 48899820 kB
x86:
oVirt: 48G = 49152 MB
VM /proc/meminfo: MemTotal: 49316136 kB
I have not checked on ppc64le so far, but is this aarch64 specific overhead significant and should there be an aarch64 specific overhead?
|
lscpu.txt |
|
Sorry for the delayed reply! Based on our last discussion I've fixed a couple of things and had further tests. First of all, it looks like copr is not available for aarch64 and with that, restricting aarch64 support to compat level 4.8 requires a local build of the copr packages for aarch64 and several manual tasks. There might be other ways around but that's not what you want for a test setup, especially when anybody else might be interested in testing this PR. Secure boot support is not in the scope of this PR, I've already commented that in the review. On a bottom line it's not just another BiosType with a few modifications. But what I've seen, most of the related changes can be tested without the need for having a aarch64 host in a test setup. I guess there are only two topics left on the list - memory overhead and memory alignment. Could you please review the changes and let me know what has to be fixed to merge this PR. |
This patch adds basic support for aarch64 hosts. The 'Compatibility Version' 4.8 provides * CPU architecture aarch64 * CPU type 'ARM64 V8' CPU model/feature detection is based on the asmid flag. Several architecture specific settings have initial values and should be validated. The intended scope of this patch is to add initial aarch64 support and a starting point for further aarch64 related patches. Co-authored-by: Richard Treu <richard.treu@sap.com> Signed-off-by: Michael Trapp <michael.trapp@sap.com>
|
Hi @dupondje I hope you’re doing well. It’s been quite a while since the last activity on this PR. In the meantime, my colleague who originally opened it has stepped away from the project and transferred the repositories to me, so I’ll be maintaining this work going forward. I just wanted to ask whether you think there is anything still missing before this PR can move forward. Thanks! |
JasperB-TeamBlue
left a comment
There was a problem hiding this comment.
First and foremost, welcome to the project. I have quickly taken a glance at this PR and it seems that it still needs some love before it can move on. I would also like to urge you towards creating extra test functionality when for example changes are done to backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java. Even better even would be to even add new functionality to the ovirt system tests, but that is a nice to have.
| select fn_db_add_config_value_for_versions_up_to('IsMigrationSupported','{"undefined": "true", "x86": "true", "ppc" : "true", "s390x" : "true", "aarch64" : "true"}','4.8'); | ||
| -- snapshot support per architecture | ||
| select fn_db_add_config_value_for_versions_up_to('IsMemorySnapshotSupported','{"undefined": "true", "x86": "true", "ppc" : "true", "s390x" : "true"}','4.8'); | ||
| select fn_db_add_config_value_for_versions_up_to('IsMemorySnapshotSupported','{"undefined": "true", "x86": "true", "ppc" : "true", "s390x" : "true", "aarch64" : "true"}','4.8'); |
There was a problem hiding this comment.
This config value is not used anymore in the code, it was badly removed in packaging/dbscripts/upgrade/04_05_0300_remove_vdc_option_IsMemorySnapshotSupported.sql in which the value was dropped. It does still exist in the DB because this command runs after the upgrade scripts which inserts it again, but there are no stubs in the actual configValues class which call upon this value. it has also been marked deprecated in backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateSnapshotForVmParameters.java.
| select fn_db_add_config_value_for_versions_up_to('IsPortIsolationSupported', 'true', '4.8'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"false","ppc":"false","s390x":"false"}', '4.5'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"true","ppc":"true","s390x":"false"}', '4.8'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"false","ppc":"false","s390x":"false"', '4.5'); |
There was a problem hiding this comment.
why drop the } ??
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"false","ppc":"false","s390x":"false"}', '4.5'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"true","ppc":"true","s390x":"false"}', '4.8'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"false","ppc":"false","s390x":"false"', '4.5'); | ||
| select fn_db_add_config_value_for_versions_up_to('TpmDeviceSupported', '{"x86":"true","ppc":"true","s390x":"false","aarch64":"false"}', '4.8'); |
There was a problem hiding this comment.
For each add config value statement that is updated, an equal update_config_value statement should be added to ensure that people who already have a functional database with these values in them, get the updated version.
| select fn_db_update_config_value('HotPlugMemorySupported','{"x86":"true","ppc":"true","s390x":"false"}', '4.2'); | ||
| select fn_db_update_config_value('HotUnplugMemorySupported','{"x86":"true","ppc":"true","s390x":"false"}','4.2'); | ||
| select fn_db_update_config_value_for_versions_from_up_to('MaxNumOfVmCpus', '{"x86":384,"ppc":384,"s390x":384}', '4.2', '4.4'); | ||
| select fn_db_update_config_value_for_versions_from_up_to('MaxNumOfVmCpus', '{"x86":384,"ppc":384,"s390x":384', '4.2', '4.4'); |
There was a problem hiding this comment.
why drop the closing bracket?
| rxRate=Rx \u901F\u7387 | ||
| rxTotal=\u603B Rx | ||
| s390xChipset=zseries | ||
| aarch64Chipset=arm64 |
There was a problem hiding this comment.
It is not needed to translate this in every language if it is the same for each language. If only the english variant exist it will show the english variant by default. It is best to leave translations to happen on our translations portal.
| Baseboard Product Name: RHEL-AV hardcoded but added only if SkuToAVLevel options set | ||
| */ | ||
|
|
||
| if (vm.getClusterArch().getFamily() != ArchitectureType.x86) { |
There was a problem hiding this comment.
Is there any reason to safeguard this block from not executing for aarch64?
Issue
ovirt engine currently does not support aarch64 hosts
Changes introduced with this PR
Add basic support for aarch64
Are you the owner of the code you are sending in, or do you have permission of the owner?
Yes
--
We've been working on aarch64 support in ovirt engine and these are the required changes to get
an aarch64 VM started in our dev setup.
It is still work in progress and there are several open topics,
but we want to share the current status, because we are looking for feedback and support.
We've built and tested with a x86_64 container based setup,
a single aarch64 host on CentOS 9 and local storage.
Installed a VM with CentOS 10 aarch64.
Basic storage/ISO/network access works as expected.
VNC based console access with keyboard + mouse is available.
At the moment, aarch64 is configured to be available in 'compatibility version' 4.8
and the OS type is restricted to 'Other OS'/Linux. Most of the platform specific values in the code
(hugepagesize, maxcpu ...) are copied from other architectures and should be validated.
The feature/model detection for aarch64 seems to be incomplete, we haven't found any 'model_' labeling
for aarch64. This might need additional patches in vdsm or qemu. Due to that, the current model detection in this PR
is very basic - just to get it up and running.
Known issues:
QEMU recommends the 'virt' board for aarch64 VMs and you will also need the following vdsm patch on the aarch64 host
oVirt/vdsm@4127dfc
We had to disable the repo 'resilientstorage' because it seems to be unavailabel for aarch64.
Open topics: