On 4/4/22 11:49 AM, Charles Arnold wrote: > > > On 4/4/22 6:50 AM, Daniel P. Berrangé wrote: >> On Fri, Apr 01, 2022 at 12:13:17PM -0600, Charles Arnold wrote: >>> From d700e8cee7cd525c0022b5a9a440f64c4ab149f0 Mon Sep 17 00:00:00 2001 >>> From: Charles Arnold <carnold@xxxxxxxx> >>> Date: Fri, 1 Apr 2022 12:01:21 -0600 >>> Subject: [PATCH 1/1] Add support for enabling Secure Encrypted >>> Virtualization >>> in the GUI >>> >>> Add an "Enable Launch Security" checkbox on the Details memory tab. >>> Do the minimal configuration required for libvirt to enable this feature >>> on compatible hardware. >>> >> Don't we need to turn on the 'iommu' option for all virtio devices >> too, and disable PXE on any NICs ? >> >> https://libvirt.org/kbase/launch_security_sev.html#virtio >> > > I used to enumerate through the virtio devices in an old version of this > patch > for virt-manager and enable iommu but it really wasn't reasonable for > virt-manager to track which virtio devices needed iommu enabled. > Additionally, > libvirt will sometimes add a device when a VM is created. This patch > leans on libvirt to do the right thing when sev is enabled similar to what > happens when launch security is specified on the virt-install command line. > Yeah, I would still like to see libvirt do this unless there's a good reason why it can't. From my July 2020 mail https://listman.redhat.com/archives/virt-tools-list/2020-July/017087.html > if sev > launchSecurity _requires_ every virtio device to have iommu='on' then > libvirt should be setting that itself. It doesn't need to hardcode it > into the XML, it can set it at VM startup time. If the user set an > explicit value in the XML then honor that but otherwise fill it in at > runtime when it is required. Trying to deal with this in an app where we > want to advertise turning the config off is basically an impossible > problem to know if we are going to undo any explicit user config or not. danpb does this sound reasonable? If so I can work on this. Also, anyone know if TDX and SNP will require virtio iommu setting as well? Anyways, until we can make this 'just work' I don't think it makes sense in virt-manager UI. If making this work already requires XML edits that virt-manager doesn't expose, IMO it's fine to tell users to also do: sudo virt-xml VMNAME --edit --launchSecurity type=sev && sudo virt-xml VMNAME --edit --memoryBacking locked=yes Putting it in the UI before it works out of the box is just going to create support headaches IMO. Thanks, Cole
