Re: [pve-devel] [PATCH qemu-server] use non SMM ovmf code file for i440fx machines

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 11.10.21 13:42, Dominik Csapak wrote:
> ovmf with SMM enabled will not boot on i440fx (hangs on graphics
> initialization), so load the non SMM variant.
> 
> should be no issue regarding live-migration since it never worked with
> this anyway.
> 
> adapts the test and adds one with q35
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm                              |  2 +-
>  PVE/QemuServer.pm                             | 25 ++++++++++++-----
>  test/cfg2cmd/efi-secboot-and-tpm-q35.conf     |  6 ++++
>  test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd | 28 +++++++++++++++++++
>  test/cfg2cmd/efi-secboot-and-tpm.conf         |  2 +-
>  test/cfg2cmd/efi-secboot-and-tpm.conf.cmd     |  2 +-
>  6 files changed, 55 insertions(+), 10 deletions(-)
>  create mode 100644 test/cfg2cmd/efi-secboot-and-tpm-q35.conf
>  create mode 100644 test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index cc2a543..5c28d5b 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -184,7 +184,7 @@ my $create_disks = sub {
>  	    my $volid;
>  	    if ($ds eq 'efidisk0') {
>  		($volid, $size) = PVE::QemuServer::create_efidisk(
> -		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk);
> +		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $conf);

why do you pass the whole config if you effectively want to tell the method "smm or no-smm"?

>  	    } elsif ($ds eq 'tpmstate0') {
>  		# swtpm can only use raw volumes, and uses a fixed size
>  		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index eb29fc2..e3c6eea 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -64,6 +64,14 @@ eval {
>  my $EDK2_FW_BASE = '/usr/share/pve-edk2-firmware/';
>  my $OVMF = {
>      x86_64 => {
> +	'i440fx-4m' => [
> +	    "$EDK2_FW_BASE/OVMF_CODE_4M.fd",
> +	    "$EDK2_FW_BASE/OVMF_VARS_4M.fd",
> +	],
> +	'i440fx-4m-ms' => [
> +	    "$EDK2_FW_BASE/OVMF_CODE_4M.fd",
> +	    "$EDK2_FW_BASE/OVMF_VARS_4M.ms.fd",
> +	],

why is this i440fx? Can we avoid names deduced by correlation and use that what
makes it actual differnet? E.g.:

`4m-no-smm`
`4m-no-smm-ms`

>  	'4m' => [
>  	    "$EDK2_FW_BASE/OVMF_CODE_4M.secboot.fd",
>  	    "$EDK2_FW_BASE/OVMF_VARS_4M.fd",
> @@ -3152,8 +3160,8 @@ sub get_vm_machine {
>      return $machine;
>  }
>  
> -sub get_ovmf_files($$) {
> -    my ($arch, $efidisk) = @_;
> +sub get_ovmf_files($$$) {
> +    my ($arch, $efidisk, $conf) = @_;
>  
>      my $types = $OVMF->{$arch}
>  	or die "no OVMF images known for architecture '$arch'\n";
> @@ -3161,6 +3169,9 @@ sub get_ovmf_files($$) {
>      my $type = 'default';
>      if (defined($efidisk->{efitype}) && $efidisk->{efitype} eq '4m') {
>  	$type = $efidisk->{'pre-enrolled-keys'} ? "4m-ms" : "4m";
> +	if (!PVE::QemuServer::Machine::machine_type_is_q35($conf)) {
> +	    $type = 'i440fx-'.$type;
> +	}
>      }
>  
>      return $types->{$type}->@*;
> @@ -3427,7 +3438,7 @@ sub config_to_command {
>  	    $d = parse_drive('efidisk0', $efidisk);
>  	}
>  
> -	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d);
> +	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $conf);
>  	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
>  
>  	my ($path, $format);
> @@ -7523,7 +7534,7 @@ sub get_efivars_size {
>      my ($conf) = @_;
>      my $arch = get_vm_arch($conf);
>      my $efidisk = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
> -    my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk);
> +    my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk, $conf);
>      die "uefi vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
>      return -s $ovmf_vars;
>  }
> @@ -7548,10 +7559,10 @@ sub update_tpmstate_size {
>      $conf->{tpmstate0} = print_drive($disk);
>  }
>  
> -sub create_efidisk($$$$$$) {
> -    my ($storecfg, $storeid, $vmid, $fmt, $arch, $efidisk) = @_;
> +sub create_efidisk($$$$$$$) {
> +    my ($storecfg, $storeid, $vmid, $fmt, $arch, $efidisk, $conf) = @_;
>  
> -    my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk);
> +    my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk, $conf);
>      die "EFI vars default image not found\n" if ! -f $ovmf_vars;
>  
>      my $vars_size_b = -s $ovmf_vars;
> diff --git a/test/cfg2cmd/efi-secboot-and-tpm-q35.conf b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf
> new file mode 100644
> index 0000000..5d4b5f5
> --- /dev/null
> +++ b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf
> @@ -0,0 +1,6 @@
> +# TEST: Test newer 4MB efidisk with secureboot, smm enforce and a TPM device on Q35
> +smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
> +bios: ovmf
> +machine: q35
> +efidisk0: local:100/vm-disk-100-0.raw,efitype=4m,pre-enrolled-keys=1,size=528K
> +tpmstate0: local:108/vm-100-disk-1.raw,size=4M,version=v2.0
> diff --git a/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
> new file mode 100644
> index 0000000..b2a2662
> --- /dev/null
> +++ b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
> @@ -0,0 +1,28 @@
> +/usr/bin/kvm \
> +  -id 8006 \
> +  -name vm8006 \
> +  -no-shutdown \
> +  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
> +  -mon 'chardev=qmp,mode=control' \
> +  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
> +  -mon 'chardev=qmp-event,mode=control' \
> +  -pidfile /var/run/qemu-server/8006.pid \
> +  -daemonize \
> +  -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
> +  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE_4M.secboot.fd' \
> +  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=540672,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
> +  -smp '1,sockets=1,cores=1,maxcpus=1' \
> +  -nodefaults \
> +  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> +  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
> +  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
> +  -m 512 \
> +  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg
> +  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1'
> +  -chardev 'socket,id=tpmchar,path=/var/run/qemu-server/8006.swtpm' \
> +  -tpmdev 'emulator,id=tpmdev,chardev=tpmchar' \
> +  -device 'tpm-tis,tpmdev=tpmdev' \
> +  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
> +  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
> +  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
> +  -machine 'type=q35+pve0'
> diff --git a/test/cfg2cmd/efi-secboot-and-tpm.conf b/test/cfg2cmd/efi-secboot-and-tpm.conf
> index ba2601f..915424e 100644
> --- a/test/cfg2cmd/efi-secboot-and-tpm.conf
> +++ b/test/cfg2cmd/efi-secboot-and-tpm.conf
> @@ -1,4 +1,4 @@
> -# TEST: Test newer 4MB efidisk with secureboot, smm enforce and a TPM device
> +# TEST: Test newer 4MB efidisk with secureboot and a TPM device
>  smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
>  bios: ovmf
>  efidisk0: local:100/vm-disk-100-0.raw,efitype=4m,pre-enrolled-keys=1,size=528K
> diff --git a/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd b/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
> index 499dbab..400db42 100644
> --- a/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
> +++ b/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
> @@ -9,7 +9,7 @@
>    -pidfile /var/run/qemu-server/8006.pid \
>    -daemonize \
>    -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
> -  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE_4M.secboot.fd' \
> +  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE_4M.fd' \
>    -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=540672,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
>    -smp '1,sockets=1,cores=1,maxcpus=1' \
>    -nodefaults \
>