-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
Add "view, edit, and add TPM devices" to overview #1796
base: main
Are you sure you want to change the base?
Conversation
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) |
@martinpitt Thank you for your response.
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 will try to create the test code. |
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 ( Thanks! |
For upgrading it seems sensible to add a TPM, I assume most users would want a For UEFI, it seems virt-install these days already adds a TPM |
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.
|
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? |
@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? |
@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! |
@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! |
@martinpitt Thank you for your feedback. |
Remove "edit" from TPM device Remove "remove" from TPM device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thank you.
Since I don't yet have the know-how to create a test program, is it possible to get some help? |
@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? |
@martinpitt Thank you for your support. |
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.