Sorry for the long review delay, I've been hoarding virt-manager bits in
a folder until I could find the time.
I pushed patch #1
The interconnected sev + iommu=on + locked memory requirement scares me.
Let's say user clicks the sev checkbox but it the VM fails to boot for
some reason. They unclick the icon, but we don't remove the iommu
attribute for all their devices. We can't really 'undo' this operation
in a safe way without possibly changing explicit user config.
The questions we need to answer are:
* How does sev behave when enabled but devices are lacking iommu='on'
* How does VM behave when sev is not in use but devices have iommu='on'
I said this back when we added virt-install support but: 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.
There's also the problem that in some cases libvirt will add default
virtio devices of its own so it's tough for us to get this correct at VM
install time.
Same questions for locked memory. If it is required for sev then libvirt
should probably enable it when the user hasn't specified explicitly one
way or another. If libvirt can't enable it for some reason, like it's
not safe in all cases or requires certain host config, then we need to
consider those reasons in virt-manager too.
The main thing I'm thinking is if a user sees this checkbox, clicks it,
something goes wrong and VM doesn't boot, they unclick the checkbox, but
their VM is now in a state that they totally lack understanding of how
to fix. Not fun for them or us
Also FWIW I don't have easy access to a sev enabled machine so it's
tough for me to experiment with some of this myself.
Some comments below about mechanical stuff
On 5/5/20 11:25 AM, Charles Arnold wrote:
> Wire up the GUI changes for enabling launch security (SEV). The
> checkbox remains desensitized unless the underlying hardware and
> guest supports SEV.
>
> ---
> virtManager/details/details.py | 31 +++++++++++++++++++++++++++++++
> virtManager/object/domain.py | 29 ++++++++++++++++++++++++++++-
> virtinst/domain/memorybacking.py | 3 +++
> 3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/virtManager/details/details.py b/virtManager/details/details.py
> index e6ca45b9..2603f296 100644
> --- a/virtManager/details/details.py
> +++ b/virtManager/details/details.py
> @@ -465,6 +465,7 @@ class vmmDetails(vmmGObjectUI):
> "on_cpu_topology_enable_toggled": self.config_cpu_topology_enable,
>
> "on_mem_memory_changed": self.config_memory_changed,
> + "on_enable_launch_security_changed": self.config_launch_security_changed,
>
>
> "on_boot_list_changed": self.config_bootdev_selected,
> @@ -733,6 +734,24 @@ class vmmDetails(vmmGObjectUI):
> uiutil.set_grid_row_visible(
> self.widget("overview-firmware-title"), show_firmware)
>
> + # Launch Security
> + warn_icon = self.widget("sev-warn")
> + warn_icon.set_visible(True)
> + if (domcaps.supports_sev_launch_security() and
> + self.vm.get_xmlobj().is_uefi() and self.vm.get_xmlobj().os.is_q35()):
Since these lines are duplicating code we already have in virtinst
DomainLaunchSecurity, I'd like to see it shared. Could be
DomainLaunchSecurity.supports_sev(guest)
> + self.widget("enable-launch-security").set_sensitive(True)
> + self.widget("enable-launch-security-label").set_sensitive(True)
> + warn_icon.set_tooltip_text(
> + _("Enabling launch security also enables iommu for all virtio devices. "
> + "It is recommended you backup your guest definition before enabling "
> + "this feature. See 'virsh dumpxml <guest>'"))
> + else:
> + self.widget("enable-launch-security").set_sensitive(False)
> + self.widget("enable-launch-security-label").set_sensitive(False)
> + warn_icon.set_tooltip_text(
> + _("Enable Launch Security requires SEV compatible hardware "
> + "and a guest created with OVMF (UEFI) boot."))
> +
> # Chipset
> combo = self.widget("overview-chipset")
> model = Gtk.ListStore(str, str)
> @@ -1204,6 +1223,8 @@ class vmmDetails(vmmGObjectUI):
> def config_memory_changed(self, src_ignore):
> self.enable_apply(EDIT_MEM)
>
> + def config_launch_security_changed(self, src_ignore):
> + self.enable_apply(EDIT_MEM)
>
> # VCPUS
> def config_get_vcpus(self):
> @@ -1542,6 +1563,7 @@ class vmmDetails(vmmGObjectUI):
> if self.edited(EDIT_MEM):
> memory = uiutil.spin_get_helper(self.widget("mem-memory"))
> kwargs["memory"] = int(memory) * 1024
> + kwargs["sevmem"] = self.widget("enable-launch-security").get_active()
>
I think you want a separate EDIT_SEVMEM here. Otherwise if the user
changes guest memory, the sev code will always be triggered, which could
overwrite some explicit user config in the XML.
> return vmmAddHardware.change_config_helper(self.vm.define_memory,
> kwargs, self.vm, self.err)
> @@ -2068,6 +2090,15 @@ class vmmDetails(vmmGObjectUI):
> curmem = self.widget("mem-memory")
> curmem.set_value(int(round(vm_cur_mem)))
>
> + domcaps = self.vm.get_domain_capabilities()
> + show_sev = domcaps.supports_sev_launch_security()
> + self.widget("enable-launch-security").set_sensitive(show_sev)
> + self.widget("enable-launch-security-label").set_sensitive(show_sev)
> + if self.vm.get_launch_security_type():
> + self.widget("enable-launch-security").set_active(True)
> + else:
> + self.widget("enable-launch-security").set_active(False)
> +
> def refresh_disk_page(self, disk):
> path = disk.path
> devtype = disk.device
> diff --git a/virtManager/object/domain.py b/virtManager/object/domain.py
> index 9621eb97..8d6f6250 100644
> --- a/virtManager/object/domain.py
> +++ b/virtManager/object/domain.py
> @@ -562,12 +562,36 @@ class vmmDomain(vmmLibvirtObject):
> guest.cpu.set_model(guest, model)
> self._redefine_xmlobj(guest)
>
> - def define_memory(self, memory=_SENTINEL):
> + def define_memory(self, memory=_SENTINEL, sevmem=_SENTINEL):
> guest = self._make_xmlobj_to_define()
>
> if memory != _SENTINEL:
> guest.currentMemory = int(memory)
> guest.memory = int(memory)
> + if sevmem != _SENTINEL:
> + if sevmem is True:
> + guest.launchSecurity.type = "sev"
> + guest.launchSecurity.set_defaults(guest)
> + guest.memoryBacking.set_locked(True)
> + devtypes = guest.devices._XML_PROP_ORDER
You can use guest.devices.get_all() rather than mess with devtype
> + # Enable iommu for all virtio devices
> + for devtype in devtypes:
> + devices = getattr(guest.devices, devtype)
> + if not devices:
> + continue
> + for dev in devices:
> + if hasattr(dev, 'virtio_driver') is True:
> + if ((hasattr(dev, 'bus') is True and 'virtio' in dev.bus) or
> + (hasattr(dev, 'model') is True and dev.model and 'virtio' in dev.model) or
> + (hasattr(dev, 'type') is True and dev.type and 'virtio' in dev.type) or
> + (hasattr(dev, 'target_type') is True and dev.target_type and 'virtio' in dev.target_type)):
> + dev.virtio_driver.iommu = True
This checking could be Device.is_virtio() which each subclass overwrites
if it can do virtio. That could be its own prep patch but would need
unittests. Parsing a static XML document in test_xmlparse.py is probably
the best place
> + else:
> + guest.launchSecurity.type = None
> + guest.launchSecurity.cbitpos = None
> + guest.launchSecurity.reducedPhysBits = None
> + guest.launchSecurity.policy = None
> + guest.memoryBacking.set_locked(False)
> self._redefine_xmlobj(guest)
>
This logic should be moved into virtinst. Maybe a public function in
DomainLaunchSecurity. That way we can unit test it and play with
exposing it via virt-install/virt-xml first.
> def define_overview(self, machine=_SENTINEL, description=_SENTINEL,
> @@ -1239,6 +1263,9 @@ class vmmDomain(vmmLibvirtObject):
> def get_description(self):
> return self.get_xmlobj().description
>
> + def get_launch_security_type(self):
> + return self.get_xmlobj().launchSecurity.type
> +
I would drop this and just opencode it in details.py. These accessors
mostly predate the get_xmlobj() function
> def get_cpu_config(self):
> return self.get_xmlobj().cpu
>
> diff --git a/virtinst/domain/memorybacking.py b/virtinst/domain/memorybacking.py
> index c883c57d..e2ee1c66 100644
> --- a/virtinst/domain/memorybacking.py
> +++ b/virtinst/domain/memorybacking.py
> @@ -36,3 +36,6 @@ class DomainMemoryBacking(XMLBuilder):
> allocation_mode = XMLProperty("./allocation/@mode")
>
> pages = XMLChildProperty(_HugepagesPage, relative_xpath="./hugepages")
> +
> + def set_locked(self, value):
> + self.locked = value
>
I would drop set_locked and opencode it too since it's not doing
anything except manipulating an XML property
Thanks,
Cole
