From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.120.1614879334853224728 for ; Thu, 04 Mar 2021 09:35:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Zw0s83R3; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614879334; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fX/bdartkh9QLc4IL/vvUeCcd6p+qPsMaXCO09Gp/gw=; b=Zw0s83R3UWvCXCOInFFztP5h7DN1dihahVk/Py9uyfG6YgE7ZB30Um4bRbO9Pzw+dipQmK R2qgOYuuPmN5xXE9zIR79II6QGG2GoXEfLYGZ3+R070vU2zxAlg1g+jqlgzWDfBgzXZuDH RJd7+N2WHe9HFpzgIJS9nsI5oqQ4/WA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-335-JIO3ZmR4MNWvG1VfEazaNQ-1; Thu, 04 Mar 2021 12:35:32 -0500 X-MC-Unique: JIO3ZmR4MNWvG1VfEazaNQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2DF1887A831; Thu, 4 Mar 2021 17:35:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-76.ams2.redhat.com [10.36.112.76]) by smtp.corp.redhat.com (Postfix) with ESMTP id 96E0A1412A; Thu, 4 Mar 2021 17:35:27 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH 00/14] Firmware Support for Fast Live Migration for AMD SEV To: Tobin Feldman-Fitzthum , devel@edk2.groups.io Cc: Dov Murik , Tobin Feldman-Fitzthum , James Bottomley , Hubertus Franke , Brijesh Singh , Ashish Kalra , Jon Grimm , Tom Lendacky References: <20210302204839.82042-1-tobin@linux.ibm.com> <9d7de545-7902-4d38-ba49-f084a750ee2a@redhat.com> <4869b086-f329-b79b-39ee-e21bfc5f95b5@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <0fc31304-98e0-1b66-3906-7b7f27f88e5e@redhat.com> Date: Thu, 4 Mar 2021 18:35:26 +0100 MIME-Version: 1.0 In-Reply-To: <4869b086-f329-b79b-39ee-e21bfc5f95b5@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 03/03/21 19:25, Tobin Feldman-Fitzthum wrote: >> Laszlo wrote: >> I'm quite uncomfortable with an attempt to hide a CPU from the OS via >> ACPI. The OS has other ways to learn (for example, a boot loader could >> use the MP services itself, stash the information, and hand it to the OS >> kernel -- this would minimally allow for detecting an inconsistency in >> the OS). What about "all-but-self" IPIs too -- the kernel might think >> all the processors it's poking like that were under its control. > > This might be the second most controversial piece. Here's a question: if > we could successfully hide the MH vCPU from the OS, would it still make > you uncomfortable? In other words, is the worry that there might be some > inconsistency or more generally that there is something hidden from the > OS? (1) My personal concern is the consistency aspect. In *some* parts of the firmware, we'd rely on the hidden CPU to behave as a "logical execution unit" (because we want it to run the MH), but in other parts of the firmware, we'd expect it to be hidden. (Consider what EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() should do while the MH is running!) And then the CPU should be hidden from the OS completely, even if the OS doesn't rely on ACPI, but massages LAPIC stuff that is architecturally specified. In other words, we'd have to treat this processor as a "service processor", outside of the "normal" (?) processor domain -- basically what the PSP is right now. I don't have the slightest idea how physical firmware deals with service processors in general. I'm really scared of the many possible corner cases (CPU hot(un)plug, NUMA proximity, ...) (2) I expect kernel developers to have concerns about a firmware-level "background job" at OS runtime. SMM does something similar (periodic or otherwise hardware-initiated async SMIs etc), and kernel developers already dislike those (latency spikes, messing with hardware state...). > One thing to think about is that the guest owner should generally be > aware that there is a migration handler running. The way I see it, a > guest owner of an SEV VM would need to opt-in to migration and should > then expect that there is an MH running even if they aren't able to see > it. Of course we need to be certain that the MH isn't going to break the > OS. I didn't think of the guest owner, but the developers that work on (possibly unrelated parts of) the guest kernel. > >> Also, as far as I can tell from patch #7, the AP seems to be >> busy-looping (with a CpuPause() added in), for the entire lifetime of >> the OS. Do I understand right? If so -- is it a temporary trait as well? > > In our approach the MH continuously checks for commands from the > hypervisor. There are potentially ways to optimize this, such as having > the hypervisor de-schedule the MH vCPU while not migrating. You could > potentially shut down down the MH on the target after receiving the > MH_RESET command (when the migration finishes), but what if you want to > migrate that VM somewhere else? I have no idea. In the current world, de-scheduling a particular VCPU for extended periods of time is a bad idea (stolen time goes up, ticks get lost, ...) So I guess this would depend on how well you could "hide" the service processor from the guest kernel. I'd really like if we could rely on an established "service processor" methodology, in the guest. Physical platform vendors have used service processors for ages, the firmwares on those platforms (on the main boards) do manage the service processors, and the service processors are hidden from the OS too (beyond specified access methods, if any). My understanding (or assumption) is that such a service processor is primarily a separate entity (you cannot talk to them "unintentionally", for example with an All-But-Self IPI), and that it's reachable only with specific access methods. I think the AMD PSP itself might follow this approach (AIUI it's an aarch64 CPU on an otherwise Intel/AMD arch platform). I'd like us to benefit from a crystallized "service processor" abstraction, if possible. I apologize that I'm this vague -- I've never seen such firmware code that deals with a service processor, I just assume it exists. Thanks Laszlo > >> >> Sorry if my questions are "premature", in the sense that I could get my >> own answers as well if I actually read the patches in detail -- however, >> I wouldn't like to do that at once, because then I'll be distracted by >> many style issues and other "trivial" stuff. Examples for the latter: > > Not premature at all. I think you hit the nail on the head with > everything you raised. > > -Tobin > >> >> - patch#1 calls SetMemoryEncDecHypercall3(), but there is no such >> function in edk2, so minimally it's a patch ordering bug in the series, >> >> - in patch#1, there's minimally one whitespace error (no whitespace >> right after "EFI_SIZE_TO_PAGES") >> >> - in patch#1, the alphabetical ordering in the [LibraryClasses] section, >> and in the matching #include directives, gets broken, >> >> - I'd prefer if the "SevLiveMigrationEnabled" UEFI variable were set in >> ConfidentialMigrationDxe, rather than PlatformDxe (patch #3), or at >> least another AMD SEV related DXE driver (OvmfPkg/AmdSevDxe etc). >> >> - any particular reasonf or making the UEFI variable non-volatile? I >> don't think it should survive any particular boot of the guest. >> >> - Why do we need a variable in the first place? >> >> etc etc >> >> Thanks! >> Laszlo >> >> >> >> >>> Ashish Kalra (2): >>>    OvmfPkg/PlatformPei: Mark SEC GHCB page in the page encrpytion >>> bitmap. >>>    OvmfPkg/PlatformDxe: Add support for SEV live migration. >>> >>> Brijesh Singh (1): >>>    OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall >>> >>> Dov Murik (1): >>>    OvmfPkg/AmdSev: Build page table for migration handler >>> >>> Tobin Feldman-Fitzthum (10): >>>    OvmfPkg/AmdSev: Base for Confidential Migration Handler >>>    OvmfPkg/PlatfomPei: Set Confidential Migration PCD >>>    OvmfPkg/AmdSev: Setup Migration Handler Mailbox >>>    OvmfPkg/AmdSev: MH support for mailbox protocol >>>    UefiCpuPkg/MpInitLib: temp removal of MpLib cleanup >>>    UefiCpuPkg/MpInitLib: Allocate MP buffer as runtime memory >>>    UefiCpuPkg/CpuExceptionHandlerLib: Exception handling as runtime >>>      memory >>>    OvmfPkg/AmdSev: Don't overwrite mailbox or pagetables >>>    OvmfPkg/AmdSev: Don't overwrite MH stack >>>    OvmfPkg/AmdSev: MH page encryption POC >>> >>>   OvmfPkg/OvmfPkg.dec                           |  11 + >>>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   2 + >>>   OvmfPkg/AmdSev/AmdSevX64.fdf                  |  13 +- >>>   .../ConfidentialMigrationDxe.inf              |  45 +++ >>>   .../ConfidentialMigrationPei.inf              |  35 ++ >>>   .../DxeMemEncryptSevLib.inf                   |   1 + >>>   .../PeiMemEncryptSevLib.inf                   |   1 + >>>   OvmfPkg/PlatformDxe/Platform.inf              |   2 + >>>   OvmfPkg/PlatformPei/PlatformPei.inf           |   2 + >>>   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   2 + >>>   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   2 + >>>   OvmfPkg/AmdSev/ConfidentialMigration/MpLib.h  | 235 +++++++++++++ >>>   .../ConfidentialMigration/VirtualMemory.h     | 177 ++++++++++ >>>   OvmfPkg/Include/Guid/MemEncryptLib.h          |  16 + >>>   OvmfPkg/PlatformDxe/PlatformConfig.h          |   5 + >>>   .../ConfidentialMigrationDxe.c                | 325 ++++++++++++++++++ >>>   .../ConfidentialMigrationPei.c                |  25 ++ >>>   .../X64/PeiDxeVirtualMemory.c                 |  18 + >>>   OvmfPkg/PlatformDxe/AmdSev.c                  |  99 ++++++ >>>   OvmfPkg/PlatformDxe/Platform.c                |   6 + >>>   OvmfPkg/PlatformPei/AmdSev.c                  |  10 + >>>   OvmfPkg/PlatformPei/Platform.c                |  10 + >>>   .../CpuExceptionHandlerLib/DxeException.c     |   8 +- >>>   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  21 +- >>>   UefiCpuPkg/Library/MpInitLib/MpLib.c          |   7 +- >>>   25 files changed, 1061 insertions(+), 17 deletions(-) >>>   create mode 100644 >>> OvmfPkg/AmdSev/ConfidentialMigration/ConfidentialMigrationDxe.inf >>>   create mode 100644 >>> OvmfPkg/AmdSev/ConfidentialMigration/ConfidentialMigrationPei.inf >>>   create mode 100644 OvmfPkg/AmdSev/ConfidentialMigration/MpLib.h >>>   create mode 100644 >>> OvmfPkg/AmdSev/ConfidentialMigration/VirtualMemory.h >>>   create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h >>>   create mode 100644 >>> OvmfPkg/AmdSev/ConfidentialMigration/ConfidentialMigrationDxe.c >>>   create mode 100644 >>> OvmfPkg/AmdSev/ConfidentialMigration/ConfidentialMigrationPei.c >>>   create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c >>> >