Re: [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD)

[Laszlo Ersek <lersek@redhat.com>]

(looks like I was the one to comment as second reviewer after all :) )

On 05/26/17 23:05, Jordan Justen wrote:
> On 2017-05-26 07:43:48, Brijesh Singh wrote:
>> Changes since v4:
>>  - decouple IoMmu protocol implementation from AmdSevDxe into a seperate
>>    IoMmuDxe driver. And introduce a placeholder protocol to provide the
>>    dependency support for the dependent modules.
> 
> I think you split IoMmuDxe out from AmdSevDxe based on my feedback
> regarding APRIORI, but I don't think this helped.
> 
> Ideally I would like to see one driver named IoMmuDxe that is *not* in
> APRIORI.

There are two separate goals here:

(1) Make sure that any driver that adds MMIO ranges will automatically
add those ranges with the C bit cleared in the PTEs, without actually
knowing about SEV.

(2) Provide an IOMMU protocol for those drivers that explicitly map /
unmap memory for DMA(-like) transfers.

Goal (2) is covered by the IOMMU protocol, and dependent drivers in
category (2) can be nicely ordered against IoMmuDxe with the OR depex (=
depend on the IOMMU protocol or the placeholder).

Goal (1) is a separate question. For covering goal (1), there are two
options:

(1a) modify all MMIO-adding drivers individually, to clear the C-bit.
(1b) do something general that will affect all MMIO additions at once,
without modifying individual drivers.

(1a) is ugly and it doesn't scale.

For (1b), there are again two possibilities:

(1b1) modify the GCD memory space map gDS services, so they handle SEV
internally,

(1b2) right after we enter the DXE phase (before any MMIO-adding driver
gets a chance to be dispatched), clear the C bit for all NonExistent
ranges (where MMIO might be added later) and for all currently existing
MMIO ranges (which come from the DXE core initialization, according to
MMIO memory descriptor HOBs from PEI).

I don't think we can express "right after we enter the DXE phase"
without an APRIORI entry; DEPEXes don't seem realistic for this.

Option (1b1) could still work, but as far as I remember (I could be
wrong!), Jiewen didn't like that, and he suggested (1b2) as the alternative.

> I think because of QemuFlashFvbServicesRuntimeDxe in APRIORI, we also
> need IoMmuDxe in the APRIORI.

QemuFlashFvbServicesRuntimeDxe adds the flash range as runtime,
uncacheable, system memory to the GCD memory space map (and then
allocates it at once). In practice it is used like MMIO, and therefore
the driver falls under category (1). Consequently, it should be
addressed with:
- either (1a) -- clear the C bit manually,
- or (1b1) -- modify the gDS services --> automatic C-bit clearing,
- or (1b2) -- assign the area with the C-bit pre-cleared.
- (or even (2) -- rework the driver to consume the IOMMU protocol
explicitly.)

Are you suggesting the last option -- rework
QemuFlashFvbServicesRuntimeDxe so that it consumes the IOMMU protocol?

In my opinion, that option is conceptually not much different from (1a)
-- which I don't like --, because it again implements a case-by-case
modification of an MMIO driver. Instead, I prefer SEV to be transparent
to drivers that don't already do *explicit* DMA.

> Hopefully we can figure our how to
> remove QemuFlashFvbServicesRuntimeDxe and then be able to remove
> IoMmuDxe.

The reason why QemuFlashFvbServicesRuntimeDxe is in APRIORI, and the
reason why AmdSevDxe is in APRIORI, are different.

* QemuFlashFvbServicesRuntimeDxe is in APRIORI -- and that's only when
SMM_REQUIRE is FALSE -- because it competes with
EmuVariableFvbRuntimeDxe, and QemuFlashFvbServicesRuntimeDxe takes
priority. EmuVariableFvbRuntimeDxe is only part of the build when
SMM_REQUIRE is false. When SMM_REQUIRE is TRUE, there is no competition,
and QemuFlashFvbServicesRuntimeDxe is not in APRIORI.

* AmdSevDxe is part of APRIORI because it must pre-clear the C bit for
*all* MMIO(-like) drivers.

If you move QemuFlashFvbServicesRuntimeDxe out of APRIORI (for example,
either by defining SMM_REQUIRE, or by inventing a different
serialization vehicle against EmuVariableFvbRuntimeDxe), AmdSevDxe
*still* has to remain in APRIORI, because it still must clear the C bit
for *all* MMIO(-like) drivers.

In my opinion, the current version of the patch set (v6) accurately
reflects option (1b2). And option (1b2) requires APRIORI usage.

If you dislike that, I think we'll have to go back to another option:
(1a), which I dislike; (1b1), which Jiewen dislikes (IIRC); or (2),
which I again dislike for all MMIO drivers that don't already use
explicit DMA.

Thanks
Laszlo

>>  - update debug messages to use gEfiCallerBaseName where applicable.
>>  - fix QemuFwCfgSecLib build errors and simplify SEV support
>>  - update QemuFwCfgDxeLib to assert when failed to locate IOMMU
>>  - update comments "host buffer" to " host buffer"
>>
>> Changes since v3:
>>  - update AmdSevDxe driver to produce IOMMU protocol
>>  - remove BmDmaLib dependency
>>  - update QemuFwCfgLib to use IOMMU protocol to allocate SEV DMA buffer
>>
>> Changes since v2:
>>  - move memory encryption CPUID and MSR definition into UefiCpuPkg
>>  - fix the argument order for SUB instruction in ResetVector and add more
>>    comments
>>  - update PlatformPei to use BaseMemEncryptSevLib
>>  - break the overlong comment lines to 79 chars
>>  - variable aligment and other formating fixes
>>  - split the SEV DMA support patch for QemuFwCfgLib into multiple patches as
>>    recommended by Laszlo
>>  - add AmdSevDxe driver which runs very early in DXE phase and clear the C-bit
>>    from MMIO memory region
>>  - drop 'QemuVideoDxe: Clear C-bit from framebuffer' patch since AmdSevDxe
>>    driver takes care of clearing the C-bit from MMIO region
>>  - Verified that Qemu PFLASH works fine with SEV guest, Found a KVM driver issue
>>    which was causing #PF when PFLASH was enabled. I have submitted patch to
>>    fix it in upstream http://marc.info/?l=kvm&m=149304930814202&w=2
>>
>> Changes since v1:
>>  - bug fixes in OvmfPkg/ResetVector (pointed by Tom Lendacky)
>>  - add SEV CPUID and MSR register definition in standard include file
>>  - remove the MemEncryptLib dependency from PlatformPei. Move AmdSevInitialize()
>>    implementation in local file inside the PlatformPei package
>>  - rename MemCryptSevLib to MemEncryptSevLib and add functions to set or
>>    clear memory encryption attribute on memory region
>>  - integerate SEV support in BmDmaLib
>>  - split QemuFwCfgDxePei.c into QemuFwCfgDxe.c and QemuFwCfgPei.c to
>>    allow building seperate QemuFwCfgLib for Dxe and Pei phase
>>    (recommended by Laszlo Ersek)
>>  - add SEV support in QemuFwCfgLib
>>  - clear the memory encryption attribute from framebuffer memory region
>>
>>
>> TODO:
>> (Will add these features after basic SEV support patches are accepted in upstream)
>>  - add support for DMA operation in QemuFwCfgS3Lib when SEV is enabled
>>  - investigate SMM/SMI support
>>
>> Cc: Jeff Fan <jeff.fan@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>
>> Brijesh Singh (17):
>>   UefiCpuPkg: Define AMD Memory Encryption specific CPUID and MSR
>>   OvmfPkg/ResetVector: Set C-bit when building initial page table
>>   OvmfPkg: Update dsc to use IoLib from BaseIoLibIntrinsicSev.inf
>>   OvmfPkg/BaseMemcryptSevLib: Add SEV helper library
>>   OvmfPkg/PlatformPei: Set memory encryption PCD when SEV is enabled
>>   OvmfPkg: Add AmdSevDxe driver
>>   OvmfPkg: Introduce IoMmuAbsent Protocol GUID
>>   OvmfPkg: Add PlatformHasIoMmuLib
>>   OvmfPkg: Add IoMmuDxe driver
>>   OvmfPkg/QemuFwCfgLib: Provide Pei and Dxe specific library
>>   OvmfPkg/QemuFwCfgLib: Prepare for SEV support
>>   OvmfPkg/QemuFwCfgLib: Implement SEV internal function for SEC phase
>>   OvmfPkg/QemuFwCfgLib: Implement SEV internal functions for PEI phase
>>   OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase
>>   OvmfPkg/QemuFwCfgLib: Add option to dynamic alloc FW_CFG_DMA Access
>>   OvmfPkg/QemuFwCfgLib: Add SEV support
>>   OvmfPkg: update PciHostBridgeDxe to use PlatformHasIoMmuLib
>>
>>  OvmfPkg/OvmfPkg.dec                                                    |   1 +
>>  OvmfPkg/OvmfPkgIa32.dsc                                                |  11 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                             |  12 +-
>>  OvmfPkg/OvmfPkgX64.dsc                                                 |  12 +-
>>  OvmfPkg/OvmfPkgIa32.fdf                                                |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.fdf                                             |   3 +
>>  OvmfPkg/OvmfPkgX64.fdf                                                 |   3 +
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf                                        |  43 ++
>>  OvmfPkg/IoMmuDxe/IoMmuDxe.inf                                          |  49 +++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf          |  50 +++
>>  OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf            |  37 ++
>>  OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgDxeLib.inf} |  15 +-
>>  OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgPeiLib.inf} |   9 +-
>>  OvmfPkg/PlatformPei/PlatformPei.inf                                    |   3 +
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h                             |  81 ++++
>>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.h                                         |  43 ++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h               | 184 ++++++++
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h                    |  37 ++
>>  OvmfPkg/PlatformPei/Platform.h                                         |   5 +
>>  UefiCpuPkg/Include/Register/Amd/Cpuid.h                                | 162 +++++++
>>  UefiCpuPkg/Include/Register/Amd/Fam17Msr.h                             |  62 +++
>>  UefiCpuPkg/Include/Register/Amd/Msr.h                                  |  29 ++
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.c                                          |  75 ++++
>>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                                         | 459 ++++++++++++++++++++
>>  OvmfPkg/IoMmuDxe/IoMmuDxe.c                                            |  53 +++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c           |  84 ++++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c        |  90 ++++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c            |  84 ++++
>>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c               | 439 +++++++++++++++++++
>>  OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.c              |  32 ++
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c                            | 230 ++++++++++
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c                            |  67 ++-
>>  OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgPeiDxe.c => QemuFwCfgPei.c}     |  72 ++-
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c                            |  57 +++
>>  OvmfPkg/PlatformPei/AmdSev.c                                           |  62 +++
>>  OvmfPkg/PlatformPei/Platform.c                                         |   1 +
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm                              |  70 ++-
>>  37 files changed, 2703 insertions(+), 24 deletions(-)
>>  create mode 100644 OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  create mode 100644 OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>>  create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>>  copy OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgDxeLib.inf} (71%)
>>  rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgPeiLib.inf} (80%)
>>  create mode 100644 OvmfPkg/Include/Library/MemEncryptSevLib.h
>>  create mode 100644 OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
>>  create mode 100644 UefiCpuPkg/Include/Register/Amd/Cpuid.h
>>  create mode 100644 UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>>  create mode 100644 UefiCpuPkg/Include/Register/Amd/Msr.h
>>  create mode 100644 OvmfPkg/AmdSevDxe/AmdSevDxe.c
>>  create mode 100644 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>  create mode 100644 OvmfPkg/IoMmuDxe/IoMmuDxe.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
>>  create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.c
>>  create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
>>  rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgPeiDxe.c => QemuFwCfgPei.c} (61%)
>>  create mode 100644 OvmfPkg/PlatformPei/AmdSev.c
>>
>> -- 
>> 2.7.4
>>