From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.114.1609794274981141881 for ; Mon, 04 Jan 2021 13:04:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=NA1iBQb2; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609794274; 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=8MPsk2ZUwXhO8zDzT3MaXp+GbtMa0uFmJc3rRlpPeQQ=; b=NA1iBQb2ExrEhBIpi9XDKMNu9wmuVNczzPzD8N3ssTHlK/Qgv2WD9EhUK9m0NOBwvcUB5W npOJmKlcYr3emkB3yIielZyY8o43zzriIMxnCMUGw6QkRRqJvewTJJDG6hSg1zDOoE2QdE CKB5QtE74kRX6/Ae25A7h3E4l1bwh+U= 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-332-e9ZDa8ZhP0u-iPLyqFuetw-1; Mon, 04 Jan 2021 16:04:30 -0500 X-MC-Unique: e9ZDa8ZhP0u-iPLyqFuetw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EA07C180A099; Mon, 4 Jan 2021 21:04:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-80.ams2.redhat.com [10.36.113.80]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4D6F65D9C6; Mon, 4 Jan 2021 21:04:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 06/12] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , James Bottomley , Jordan Justen , Ard Biesheuvel References: <90152d0505354d270cc3af9e5838010e4dcbe114.1608065471.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <5daddb82-ad8e-7de7-49df-f3d18907bfe1@redhat.com> Date: Mon, 4 Jan 2021 22:04:26 +0100 MIME-Version: 1.0 In-Reply-To: <90152d0505354d270cc3af9e5838010e4dcbe114.1608065471.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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: 7bit On 12/15/20 21:51, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3108 > > The PCIe MMCONFIG range should be treated as an MMIO range. However, > there is a comment in the code explaining why AddIoMemoryBaseSizeHob() > is not called. The AmdSevDxe walks the GCD map looking for MemoryMappedIo > or NonExistent type memory and will clear the encryption bit for these > ranges. > > Since the MMCONFIG range does not have one of these types, the encryption > bit is not cleared for this range. Add support to detect the presence of > the MMCONFIG range and clear the encryption bit. This will be needed for > follow-on support that will validate MMIO under SEV-ES. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Brijesh Singh > Signed-off-by: Tom Lendacky > --- > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 8 +++++++- > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 20 +++++++++++++++++++- > 2 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > index dd9ecc789a20..0676fcc5b6a4 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > @@ -2,7 +2,7 @@ > # > # Driver clears the encryption attribute from MMIO regions when SEV is enabled > # > -# Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.
> # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -39,3 +39,9 @@ [Depex] > > [FeaturePcd] > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > + > +[FixedPcd] > + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index 595586617882..ed516fcdf956 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -4,7 +4,7 @@ > in APRIORI. It clears C-bit from MMIO and NonExistent Memory space when SEV > is enabled. > > - Copyright (c) 2017, AMD Inc. All rights reserved.
> + Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include (1) Please keep the #include list alphabetically sorted. > > EFI_STATUS > EFIAPI > @@ -65,6 +66,23 @@ AmdSevDxeEntryPoint ( > FreePool (AllDescMap); > } > > + // > + // If PCI Express is enabled, the MMCONFIG area has been reserved, rather > + // than marked as MMIO, and so the C-bit won't be cleared by the above walk > + // through the GCD map. Check for the MMCONFIG area and clear the C-bit for > + // the range. > + // > + if (PcdGet16 (PcdOvmfHostBridgePciDevId) == INTEL_Q35_MCH_DEVICE_ID) { > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + FixedPcdGet64 (PcdPciExpressBaseAddress), > + EFI_SIZE_TO_PAGES (SIZE_256MB), > + FALSE > + ); > + > + ASSERT_EFI_ERROR (Status); > + } > + > // > // When SMM is enabled, clear the C-bit from SMM Saved State Area > // > Very interesting. One wonders why, without this change, MMCONFIG accesses work at all on SEV. But then... this guest phys area is not backed by RAM in the first place. Whenever the guest accesses it, we trap to QEMU unconditionally. And so memory encryption plays no role in practice, I must think. It's different for the flash, because the flash is backed by RAM, and whether an access to it traps to QEMU or not depends on both the access (r/w/x) and the mode the flash is in (programming mode on vs. off). I now wonder whether the comment in the leading context (not visible above), namely the one that references the root bridge MMIO aperture, from which the PCI MMIO BARs are allocated, is accurate. Perhaps that area would work in fact even if we didn't clear the C bit for them (considering just the accesses themselves under SEV; not SEV-ES). (2) Please include a sentence in the commit message about the fact that MMCONFIG is not backed by a KVM memory slot, and so actual memory encryption does not take place, and that's why MMCONFIG accesses do not break currently under SEV / SEV-ES. (This is at least what I think happens.) With (1) and (2) addressed: Reviewed-by: Laszlo Ersek Thanks Laszlo