On 4/7/22 3:14 PM, Charles Arnold wrote: > From 9646a5dd083a5acdf6cf519863354efaf4bdd07c Mon Sep 17 00:00:00 2001 > From: Charles Arnold <carnold@xxxxxxxx> > Date: Thu, 7 Apr 2022 13:00:10 -0600 > Subject: [PATCH v2 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. > > Allow libvirt to determine the firmware file. This just requires > enabling of efi and setting an appropriate policy. Check the libvirt > domain capabilities to determine whether SEV or SEV-ES is supported. > Sorry for the embarrassing delay replying here :/ I separated out a commit with the launch_security.py change to use SEV-ES policy, added a test case, and pushed with you as author: https://github.com/virt-manager/virt-manager/commit/424283ad1db9c4da519fac698486967e6b6557b0 Some more comments below > Signed-off-by: Charles Arnold <carnold@xxxxxxxx> > --- > ui/details.ui | 15 ++++++++++++++- > virtManager/details/details.py | 15 ++++++++++++++- > virtManager/object/domain.py | 14 +++++++++++++- > virtinst/domain/launch_security.py | 23 +++++++++++++++-------- > virtinst/domain/memorybacking.py | 3 +++ > virtinst/domcapabilities.py | 8 +++++++- > 6 files changed, 66 insertions(+), 12 deletions(-) > > diff --git a/virtManager/object/domain.py b/virtManager/object/domain.py > index 70e4e49f..feb43bd2 100644 > --- a/virtManager/object/domain.py > +++ b/virtManager/object/domain.py > @@ -688,7 +688,7 @@ class vmmDomain(vmmLibvirtObject): > guest.memoryBacking.access_mode = access_mode > > def define_memory(self, memory=_SENTINEL, maxmem=_SENTINEL, > - mem_shared=_SENTINEL): > + mem_shared=_SENTINEL, sevmem=_SENTINEL): > guest = self._make_xmlobj_to_define() > > if memory != _SENTINEL: > @@ -697,6 +697,15 @@ class vmmDomain(vmmLibvirtObject): > guest.memory = int(maxmem) > if mem_shared != _SENTINEL: > self._edit_shared_mem(guest, mem_shared) > + if sevmem != _SENTINEL: > + if sevmem is True: > + guest.launchSecurity.type = "sev" > + guest.launchSecurity.set_defaults(guest) > + guest.memoryBacking.set_locked(True) > + else: > + guest.launchSecurity.type = None > + guest.launchSecurity.policy = None > + guest.memoryBacking.set_locked(False) > > self._redefine_xmlobj(guest) > The problem with toggling the locked memory setting in lockstep is, if user enables then disables sev, it could take the locked memory setting with it. Pretty niche, but this is the problem with coupling multiple options together under a single UI knob. The alternative is adding a 'Lock memory' option to the UI which sucks in its own way... > @@ -1310,6 +1319,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 > + > def get_boot_order(self): > legacy = not self.can_use_device_boot_order() > return self.xmlobj.get_boot_order(legacy=legacy) > diff --git a/virtinst/domain/launch_security.py > b/virtinst/domain/launch_security.py > index 7af71811..f4a4a4b8 100644 > --- a/virtinst/domain/launch_security.py > +++ b/virtinst/domain/launch_security.py > @@ -19,16 +19,23 @@ class DomainLaunchSecurity(XMLBuilder): > kernelHashes = XMLProperty("./@kernelHashes", is_yesno=True) > > def _set_defaults_sev(self, guest): > - if not guest.os.is_q35() or not guest.is_uefi(): > - raise RuntimeError(_("SEV launch security requires a Q35 > UEFI machine")) > + if not guest.os.is_q35(): > + raise RuntimeError(_("SEV launch security requires a Q35 > machine")) > + # Libvirt will select the appropriate firmware file based on > the policy > + # defined below and if efi is enabled. > + if not guest.is_uefi(): > + guest.os.firmware = 'efi' > We can't change firmware setting here. This same code path would be used for UI editing an existing VM, and switching from bios to efi could make that VM unbootable. The UI doesn't give any option for changing firmware for an existing VM for that reason. Also this is likely to cause issues in virt-install too where we have to take pains to handle uefi request early, since later VM defaults depend on it. For virt-manager 'customize' dialog, the solution is to disable the sev UI with a error tooltip saying UEFI is required, and make the user select that first. For an existing VM not using UEFI, they are out of luck. For virt-install if you wanted to make the defaults smarter, you could extend Guest.prefers_uefi to check for launchSecurity=sev. Would need to be demonstrated in the test suite though > - # 'policy' is a mandatory 4-byte argument for the SEV firmware, > - # if missing, let's use 0x03 which, according to the table at > - # https://libvirt.org/formatdomain.html#launchSecurity: > - # (bit 0) - disables the debugging mode > - # (bit 1) - disables encryption key sharing across multiple guests > + # The 'policy' is a mandatory 4-byte argument for the SEV > firmware. > + # If missing, we use 0x03 for the original SEV implementation and > + # 0x07 for SEV-ES. > + # Reference: https://libvirt.org/formatdomain.html#launchSecurity > if self.policy is None: > - self.policy = "0x03" > + domcaps = guest.lookup_domcaps() > + if domcaps.supports_sev_launch_security(check_es=True): > + self.policy = "0x07" > + else: > + self.policy = "0x03" > > def set_defaults(self, guest): > if self.type == "sev": > diff --git a/virtinst/domain/memorybacking.py > b/virtinst/domain/memorybacking.py > index c883c57d..4ddd3865 100644 > --- a/virtinst/domain/memorybacking.py > +++ b/virtinst/domain/memorybacking.py > @@ -27,6 +27,9 @@ class DomainMemoryBacking(XMLBuilder): > XML_NAME = "memoryBacking" > _XML_PROP_ORDER = ["hugepages", "nosharepages", "locked", "pages"] > > + def set_locked(self, value): > + self.locked = value > + > hugepages = XMLProperty("./hugepages", is_bool=True) > nosharepages = XMLProperty("./nosharepages", is_bool=True) > locked = XMLProperty("./locked", is_bool=True) > diff --git a/virtinst/domcapabilities.py b/virtinst/domcapabilities.py > index 3ebc409d..4d79c6d1 100644 > --- a/virtinst/domcapabilities.py > +++ b/virtinst/domcapabilities.py > @@ -93,6 +93,10 @@ def _make_capsblock(xml_root_name): > class _SEV(XMLBuilder): > XML_NAME = "sev" > supported = XMLProperty("./@supported", is_yesno=True) > + cbitpos = XMLProperty("./cbitpos") > + reducedPhysBits = XMLProperty("./reducedPhysBits") > + maxGuests = XMLProperty("./maxGuests") > + maxESGuests = XMLProperty("./maxESGuests") > > > ############################# > @@ -390,12 +394,14 @@ class DomainCapabilities(XMLBuilder): > # Misc support methods # > ######################## > > - def supports_sev_launch_security(self): > + def supports_sev_launch_security(self, check_es=False): > """ > Returns False if either libvirt doesn't advertise support for > SEV at > all (< libvirt-4.5.0) or if it explicitly advertises it as > unsupported > on the platform > """ > + if check_es: > + return bool(self.features.sev.supported and > self.features.sev.maxESGuests) > return bool(self.features.sev.supported) > > def supports_video_bochs(self): I pushed most of these bits. After playing with sev the past few months I've cooled a bit on the idea of adding this to the UI. I thought consumer AMD hardware supported SEV, but I found out it's only server class hardware. This drastically reduces the users that are ever going to see this option available. It was an 'advanced' usecase to begin with but with limited audience it's downright obscure in practice. Throw in the interconnected options, questions about what sev-snp support will look like, and I'm weary to maintain this. Personally I think we should just focus on making the `virt-xml $VMNAME --edit --launchSecurity sev && virt-xml $VMNAME --edit --memorybacking locked=on` case do the right thing, or simplify that, and suggest it to interested users. Thanks, Cole
