Skip to content

Fix: Use Q35 chipset for UEFI x86_64 #4576

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.

Sign up for GitHub

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

Jump to bottom
Merged
rohityadavcloud merged 1 commit into from
Feb 1, 2021
Merged

Fix: Use Q35 chipset for UEFI x86_64 #4576

rohityadavcloud merged 1 commit into from
Feb 1, 2021

Conversation

@jairov4
Copy link
Contributor

@jairov4 jairov4 commented Jan 10, 2021 •
edited
Loading

Description

Fix #4245

This PR uses Q35 chipset for UEFI in x86_64.
Currently this mistakenly only enabled for secure boot

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Build and run a UEFI VM using OVMF firmware.
Both of Secure Boot and Legacy modes are working now

@rohityadavcloud rohityadavcloud changed the base branch from master to 4.15 January 14, 2021 00:52
@rohityadavcloud rohityadavcloud changed the base branch from 4.15 to master January 14, 2021 00:52
@rohityadavcloud
Copy link
Member

rohityadavcloud commented Jan 14, 2021

@jairov4 can you rebase to 4.15 branch?

@GabrielBrascher
Copy link
Member

GabrielBrascher commented Jan 15, 2021 •
edited
Loading

Considering that the bug was reported on 4.14.0.0, maybe it would be a nice bug to be fixed in branch 4.14, and then forwarded to 4.15 and master.

@jairov4 jairov4 changed the base branch from master to 4.14 January 15, 2021 14:30
@jairov4
Copy link
Contributor Author

jairov4 commented Jan 15, 2021

Hi, I rebased it to 4.14

DaanHoogland
DaanHoogland reviewed Jan 18, 2021
View reviewed changes
...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
guest.setBootType(GuestDef.BootType.UEFI);
guest.setBootMode(GuestDef.BootMode.LEGACY);
guest.setMachineType("q35");
Copy link
Contributor

@DaanHoogland DaanHoogland Jan 18, 2021

Choose a reason for hiding this comment

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

should we make assumptions about running on intel platforms and/or would that hurt? cc @wido @rhtyd #raspberryrules

Copy link
Contributor

@wido wido Jan 19, 2021

Choose a reason for hiding this comment

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

I know it's super fun to run on a Pi and people use it to dev with it.

But I think 99,999999% of all CloudStack installations runs on x86_64 CPUs from either Intel or AMD.

Copy link
Contributor

@DaanHoogland DaanHoogland Jan 20, 2021

Choose a reason for hiding this comment

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

i wanted to see that AMD thanks

Copy link
Contributor

@wido wido Jan 20, 2021

Choose a reason for hiding this comment

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

i wanted to see that AMD thanks

Keep in mind though that both AMD and Intel produce CPUs with the x86_64 Architecture.

As this is all virtualization I think this also works on AMD Epyc CPUs.

Copy link
Contributor

@DaanHoogland DaanHoogland Jan 21, 2021

Choose a reason for hiding this comment

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

Well, I did get some problem reports back on my quick search, but the general tone is that it should work indeed.

Copy link
Member

@rohityadavcloud rohityadavcloud Jan 26, 2021

Choose a reason for hiding this comment

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

I personally run a toy PI4 env for fun, so I would prefer a suitable if-else be added based on platform, arch parameters (I'm not sure if the hard-coded chipset works on RPi4)

@shwstppr
Copy link
Contributor

shwstppr commented Jan 25, 2021

@blueorangutan package

@shwstppr shwstppr added this to the 4.14.1.0 milestone Jan 25, 2021
@blueorangutan
Copy link

blueorangutan commented Jan 25, 2021

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2021

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2584

@rohityadavcloud
Copy link
Member

rohityadavcloud commented Jan 26, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jan 26, 2021

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

blueorangutan commented Jan 26, 2021

Trillian test result (tid-3415)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38478 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4576-t3415-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 82 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_07_deploy_kubernetes_ha_cluster Failure 3611.80 test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster Failure 0.05 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 102.81 test_kubernetes_clusters.py

rohityadavcloud
rohityadavcloud approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

LGTM (won't affect rpi4, which is uefi default, line no. 2245 has this check for aarch64)

shwstppr
shwstppr approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

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

LGTM

@rohityadavcloud rohityadavcloud merged commit e9dda98 into apache:4.14 Feb 1, 2021
@rohityadavcloud rohityadavcloud mentioned this pull request Feb 1, 2021
Closed
DaanHoogland pushed a commit that referenced this pull request Feb 1, 2021
Merge release branch 4.14 to 4.15
b6b778f 
* 4.14:
  server: select root disk based on user input during vm import (#4591)
  kvm: Use Q35 chipset for UEFI x86_64 (#4576)
  server: fix wrong error message when create isolated network without SourceNat (#4624)
  server: add possibility to scale vm to current customer offerings (#4622)
  server: keep networks order and ips while move a vm with multiple networks (#4602)
  server: throw exception when update vm nic on L2 network (#4625)
  doc: fix typo in install notes (#4633)
DaanHoogland pushed a commit that referenced this pull request Feb 1, 2021
Merge release branch 4.15 to master
ff376d8 
* 4.15:
  server: select root disk based on user input during vm import (#4591)
  kvm: Use Q35 chipset for UEFI x86_64 (#4576)
  server: fix wrong error message when create isolated network without SourceNat (#4624)
  server: add possibility to scale vm to current customer offerings (#4622)
  server: keep networks order and ips while move a vm with multiple networks (#4602)
  server: throw exception when update vm nic on L2 network (#4625)
  doc: fix typo in install notes (#4633)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wido wido wido left review comments

@DaanHoogland DaanHoogland DaanHoogland left review comments

@rohityadavcloud rohityadavcloud rohityadavcloud approved these changes

@shwstppr shwstppr shwstppr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.14.1.0

Development

Successfully merging this pull request may close these issues.

VM cannot run normally in kvm UEFI LEGACY mode

7 participants

@jairov4 @rohityadavcloud @GabrielBrascher @shwstppr @blueorangutan @wido @DaanHoogland