Add "view, edit, and add TPM devices" to overview

[Shotaro-Kawaguchi]

Added a feature to display TPM device in virtual machine overview.
With this commit, it will be possible to add, edit, and delete TPM device.
We implemented this feature because there was a use case where upgrading the Windows OS to Windows11 required additional TPM devices.

Screenshot overview

Click the "add" button to display the following screen.

Click the "edit" button to display the following screen.

[martinpitt]

Thanks @Shotaro-Kawaguchi ! I had a very cursory look. I'm mostly interested in the use case -- so far we've treated that as an implementation detail, and osinfo-db was telling libvirt whether the target OS needs a TPM (like Windows) or can use a TPM (EFI BIOS), and automatically adds an appropriate one. In which cases does that not work? Every bit of UI addition is code to maintain, increases combinatorial explosion of testing, and is a mental load to users, so we don't want to do that lightly.

This needs a round of design review and updates -- the dialogs are way too technical, and give the admin no guidance at all. When to use "tis" or "crb", and what's the difference? What's the (dis)advantage of emulator vs. passthrough? Is there any reason to use "1.2" when "2.0" is available? and so on. This needs some (i) popups with documentation links, and if possible some clearer descriptions.

Also this needs integration tests for all code paths. testConfigureBeforeInstall() has some cursory check that a software TPM was added, so our test infrastructure supports it. testConfigureBeforeInstallBiosTPM() also covers it explicitly.

Unfortunately it will take our team very long to help with that as we don't have a current machines maintainer. So it would be good if you could work on tests yourself? (Happy to guide, of course)

[Shotaro-Kawaguchi]

@martinpitt Thank you for your response.

Thanks @Shotaro-Kawaguchi ! I had a very cursory look. I'm mostly interested in the use case -- so far we've treated that as an implementation detail, and osinfo-db was telling libvirt whether the target OS needs a TPM (like Windows) or can use a TPM (EFI BIOS), and automatically adds an appropriate one. In which cases does that not work? Every bit of UI addition is code to maintain, increases combinatorial explosion of testing, and is a mental load to users, so we don't want to do that lightly.

A TPM device is required for Windows 11, but it is not needed for Windows 10 and older. Therefore, I believe that a TPM device will not be automatically added when a virtual machine is upgraded to Windows 11.
I think it is useful to be able to add a TPM device when upgrading from Windows 10 or older to Windows 11.

Also this needs integration tests for all code paths.

I will try to create the test code.

[martinpitt]

I think it is useful to be able to add a TPM device when upgrading from Windows 10 or older to Windows 11.

OK, I see that. But I still don't believe that we should throw all these technicalities to the user. The dialog allows 8 combinations, which is too much both for the user to decide on (and document) and also for testing. This needs to be reduced, automatically pick the most appropriate one (virt-xml should help with that), and the remaining options better explained. But ideally there should not be any options at all.

Thanks!

[jelly]

For upgrading it seems sensible to add a TPM, I assume most users would want a swtpm (software tpm). Passing through a TPM sounds like extra hurdles (plus the host might use it?).

For UEFI, it seems virt-install these days already adds a TPM

https://github.com/virt-manager/virt-manager/blob/2da4884962d7a8d4e66e4d6f273a452fdd3f2d4f/virtinst/guest.py#L1081

[garrett]

Agreed; this should work by default and shouldn't have to expose any configuration UI.

Creating a VM in UEFI mode should ideally automatically create a software TPM compatible with Windows 11. (I'm not sure if it does. It seems like it might, according to @jelle's comment.) And if it's compatible with Windows 11, it should also be compatible with Linux too.

Are there any cases where having a software TPM that's compatible with Windows 11 isn't enough for either Windows or Linux?

[jelly]

Agreed; this should work by default and shouldn't have to expose any configuration UI.

Creating a VM in UEFI mode should ideally automatically create a software TPM compatible with Windows 11. (I'm not sure if it does. It seems like it might, according to @jelle's comment.) And if it's compatible with Windows 11, it should also be compatible with Linux too.

Are there any cases where having a software TPM that's compatible with Windows 11 isn't enough for either Windows or Linux?

Well as the contributor noted, when upgrading there is no TPM added. Say you have an old Windows 10 machine you want to upgrade. Current workflow would be to detach the disk, and re-create a new virtual machine and specify windows 11 as OS.

---

I can confirm that changing an existing VM from BIOS => UEFI adds a tpm as expected.

[jelle@t14s][~/projects/arch-mkosi-boxes/images]%virsh dumpxml arch-test | grep tpm
    <tpm model='tpm-crb'>
      <alias name='tpm0'/>
    </tpm>

[garrett]

Should a UEFI VM without a TPM be considered a bug? Should we automatically add it? Or have an inline alert banner to add it, or something like that?

[Shotaro-Kawaguchi]

@martinpitt If the implementation is such that pressing the add button adds the TPM device with all options set to default values, would it be possible to merge it?

[martinpitt]

@Shotaro-Kawaguchi You are actually in a better position to answer that -- as you proposed and want the feature, you should also know how much configurability you need for your use case. From my point of view, I think that "defaults" would be a great start -- let virt-xml figure out the appropriate defaults, and add an integration test for adding and removal -- then we can land and refine with more options later if/when they become necessary.

Thanks!

[martinpitt]

@Shotaro-Kawaguchi We just discussed this in our meeting. We feel that this is a too rare use case to put this into the overview all the time. If the VM has a TPM, then we don't need to show anything, and for most existing VMs you don't need to change it either.

A good place for these is the kebab menu, similar to the "Replace SPICE" functionality (which is a similar corner case):

This could just grow an "Add TPM" action if the VM doesn't already have one. I don't think that you'd even need to remove it, or do you have such a use case?

Thanks!

[Shotaro-Kawaguchi]

@martinpitt Thank you for your feedback.
Since there are no use cases to delete, we will place the "Add TPM" action in the kebab menu if the VM does not yet have a TPM.

[Shotaro-Kawaguchi]

Changed to select "Add TPM" from the kebab menu.
Selecting "Add TPM" will add a TPM device with default settings.
"Add TPM" is only displayed if no TPM device exists.

[martinpitt]

Thanks @Shotaro-Kawaguchi ! This looks nice now (aside from some code cleanup and the missing "needs shutdown" integration). This now needs tests -- do you want to work on them, or need help?

[Shotaro-Kawaguchi]

Thank you.
I will proceed with modifying the source code.

This now needs tests -- do you want to work on them, or need help?

Since I don't yet have the know-how to create a test program, is it possible to get some help?

[martinpitt]

@Shotaro-Kawaguchi No worries. @mvollmer or I can spend some time on this next week and complete it with tests. So are you happy with the functionality now?

[Shotaro-Kawaguchi]

@martinpitt Thank you for your support.
Yes, I am happy with the current functionality.