Skip to content

Refactor uefi32#2305

Open
frankiejol wants to merge 8 commits intomainfrom
refactor/uefi32_v2
Open

Refactor uefi32#2305
frankiejol wants to merge 8 commits intomainfrom
refactor/uefi32_v2

Conversation

@frankiejol
Copy link
Member

Allows UEFI in 32 bits Virtual machines

@frankiejol frankiejol added this to the v2.4.6 milestone Mar 20, 2026
Copilot AI review requested due to automatic review settings March 20, 2026 11:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the KVM ISO/CDROM UEFI testing and updates KVM UEFI XML generation to support 32-bit (i686) UEFI firmware selection.

Changes:

  • Move CDROM add/remove/base lifecycle ISO coverage from t/vm/d20_disks.t into a dedicated t/kvm/uefi_iso.t test.
  • Update UEFI loader filename expectations in t/kvm/uefi.t.
  • Extend KVM UEFI XML generation to handle i686 and adjust related test fixtures.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
t/vm/d20_disks.t Removes the embedded test_cdrom test and its invocation (test moved elsewhere).
t/request/31_hw_boot_order.t Updates the test XML “machine” attribute dynamically via _find_machine_type.
t/kvm/uefi_iso.t Adds a new dedicated KVM UEFI ISO/CDROM regression test covering CDROM add/remove and base ops.
t/kvm/uefi.t Relaxes the OVMF loader filename match for q35+UEFI test cases.
t/kvm/pcie.t Adjusts the sample libvirt XML seclabel model in the fixture.
lib/Ravada/VM/KVM.pm Updates _xml_add_uefi to introduce i686 handling and change x86_64 behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +197 to +199
my $machine_type = $domain->_vm->_find_machine_type('i686',qr'pc-i440fx');
my ($os_type) = $doc->findnodes('/domain/os/type');
$os_type->setAttribute('machine' => $machine_type);
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

_fix_domain_config() sets the domain machine attribute using an i686 machine type, but the embedded XML being reloaded is explicitly arch='x86_64'. This arch/machine mismatch can make libvirt reject the domain or behave unexpectedly when the test later starts the VM. Use the arch from the XML (or from the existing domain XML) when calling _find_machine_type(), or update the arch attribute consistently if the intent is to force i686.

Suggested change
my $machine_type = $domain->_vm->_find_machine_type('i686',qr'pc-i440fx');
my ($os_type) = $doc->findnodes('/domain/os/type');
$os_type->setAttribute('machine' => $machine_type);
my ($os_type) = $doc->findnodes('/domain/os/type');
if ($os_type) {
my $arch = $os_type->getAttribute('arch') // 'i686';
my $machine_type = $domain->_vm->_find_machine_type($arch, qr'pc-i440fx');
$os_type->setAttribute('machine' => $machine_type);
}

Copilot uses AI. Check for mistakes.
Comment on lines +2053 to +2059
} elsif ($arch eq 'i686') {
if (!$self->file_exists($ovmf)) {
$ovmf = '/usr/share/OVMF/OVMF32_CODE_4M.fd';
}
die "Unsupported UEFI in this architecture ".$type->toString()
." missing file $ovmf"
unless $self->file_exists($ovmf);
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

In _xml_add_uefi(), the i686 branch keeps the default '/usr/share/OVMF/OVMF_CODE.fd' whenever that file exists, and only switches to the 32-bit OVMF path when OVMF_CODE.fd is missing. On hosts that have both files, i686 guests will never use OVMF32_CODE_4M.fd, which undermines the goal of enabling 32-bit UEFI. Prefer selecting the i686-specific OVMF file first (and only falling back to a generic one if the 32-bit file is missing), with an explicit existence check for the chosen path.

Copilot uses AI. Check for mistakes.
Comment on lines +2051 to 2052
if ( $arch eq 'x86_64' ) {
$ovmf = '/usr/share/OVMF/OVMF_CODE_4M.fd';
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

_xml_add_uefi() no longer ensures that x86_64 UEFI domains use a q35 machine type (the previous logic upgraded non-q35 machines to a pc-q35-* variant). Since the default domain XML templates in etc/xml include pc-i440fx-* machines, requesting UEFI without explicitly setting machine can now produce UEFI+i440fx configs that the rest of the test suite avoids (e.g., tests only exercise UEFI on q35). Consider restoring the q35 enforcement for x86_64 UEFI or handling invalid machine/UEFI combinations earlier with a clear error.

Copilot uses AI. Check for mistakes.
$found_q35++;
my ($loader) = $doc->findnodes('/domain/os/loader/text()');
like($loader,qr/OVMF_CODE_4M.fd$/) or die $domain->name;
like($loader,qr/OVMF_CODE.4M.fd$/i) or die $domain->name;
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The regex qr/OVMF_CODE.4M.fd$/i uses . which matches any character, so it will also match unintended loader filenames. If the goal is to accept both underscore and dash variants, use a more specific pattern (e.g., a character class) so the test only matches the expected OVMF 4M firmware name.

Suggested change
like($loader,qr/OVMF_CODE.4M.fd$/i) or die $domain->name;
like($loader,qr/OVMF_CODE[_-]4M\.fd$/i) or die $domain->name;

Copilot uses AI. Check for mistakes.
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