On 07/28/17 18:00, Brijesh Singh wrote:
> On 07/28/2017 08:38 AM, Laszlo Ersek wrote:
snip
>> (b) Plus, approaching the question from the Map() direction, we need
>> to consider two scenarios:
>>
>> - Client code calls AllocateBuffer(), then Map(), and it writes to
>> the buffer only then. This should be safe.
>> - client code calls AllocateBuffer(), writes to it, and then calls
>> Map(). This will result in memory contents that look like garbage
>> to the hypervisor. Bad.
>>
>> I can imagine the following to handle these cases: in the Map() and
>> Unmap() functions, we have to decrypt and encrypt the memory contents
>> in-place, after changing the C bit (without allocating additional
>> memory). Introduce a static UINT8 array with EFI_PAGE_SIZE bytes
>> (this will always remain in encrypted memory). Update the C bit with
>> a single function call for the entire range (like now) -- this will
>> not affect the guest-readability of the pages --, then bounce each
>> page within the range to the static buffer and back to its original
>> place. In effect this will in-place encrypt or decrypt the memory,
>> and will be faster than a byte-wise rewrite.
snip
>> * BusMasterCommonBuffer:
>> - Client calls AllocateBuffer(), and places some data in the
>> returned memory.
>> - Client calls Map(). Map() clears the C bit in one fell swoop,
>> and then decrypts the buffer in-place (by bouncing it page-wise
>> to the static array and back).
>> - Client communicates with the device.
>> - Client calls Unmap(). Unmap() restores the C bit in one fell
>> swoop, and encrypts the buffer in-place (by bouncing it
>> page-wise to the static array and back).
>> - Client reads some residual data from the buffer.
>> - Client calls FreeBuffer(). FreeBuffer() relases the pages.
>>
>
> Yes this works fine as long as the client uses
> EFI_PCI_IO_PROTOCOL.AllocateBuffer() to allocate the buffer.
Again, a performance-oriented thought:
Above I suggested using a statically allocated page-sized buffer, for
the in-place encryption/decryption. Ultimately this means *two*
CopyMem()s for the entire buffer (just executed page-wise), in *each* of
Map() and Unmap().
Maybe we can do better: what if you perform the CopyMem() from the
buffer right back to the same buffer? CopyMem() is *required* to work
with overlapping source and target areas (similarly to memmove() in
standard C).
This would result in *one* CopyMem (for in-place de-/encryption) in each
of Map() and Unmap(), and thereby it would have identical performance
impact to the BusMasterRead and BusMasterWrite Map() operations (where
copying / crypting takes place between distinct memory areas).
The OVMF DSC files resolve "BaseMemoryLib" -- which provides CopyMem()
-- to "MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf";
regardless of module type. The actual implementation appears to reside
in "MdePkg/Library/BaseMemoryLibRepStr/X64/CopyMem.nasm":
> global ASM_PFX(InternalMemCopyMem)
> ASM_PFX(InternalMemCopyMem):
> push rsi
> push rdi
> mov rsi, rdx ; rsi <- Source
> mov rdi, rcx ; rdi <- Destination
> lea r9, [rsi + r8 - 1] ; r9 <- End of Source
> cmp rsi, rdi
> mov rax, rdi ; rax <- Destination as return value
> jae .0
> cmp r9, rdi
> jae @CopyBackward ; Copy backward if overlapped
> .0:
> mov rcx, r8
> and r8, 7
> shr rcx, 3
> rep movsq ; Copy as many Qwords as possible
> jmp @CopyBytes
> @CopyBackward:
> mov rsi, r9 ; rsi <- End of Source
> lea rdi, [rdi + r8 - 1] ; esi <- End of Destination
> std ; set direction flag
> @CopyBytes:
> mov rcx, r8
> rep movsb ; Copy bytes backward
> cld
> pop rdi
> pop rsi
> ret
>
However, I'm afraid even if this works on SEV (which I certainly
expect!), this code won't be reached, due to the following CopyMem()
wrapper implementation in
"MdePkg/Library/BaseMemoryLibRepStr/CopyMemWrapper.c":
> VOID *
> EFIAPI
> CopyMem (
> OUT VOID *DestinationBuffer,
> IN CONST VOID *SourceBuffer,
> IN UINTN Length
> )
> {
> if (Length == 0) {
> return DestinationBuffer;
> }
> ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)DestinationBuffer));
> ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)SourceBuffer));
>
> if (DestinationBuffer == SourceBuffer) {
> return DestinationBuffer;
> }
> return InternalMemCopyMem (DestinationBuffer, SourceBuffer, Length);
> }
As you see, (DestinationBuffer == SourceBuffer) is handled as a no-op
(quite justifiedly, except in the case of SEV).
Personally I think it would be OK to copy the wrapper function and the
assembly code to OvmfPkg/IoMmuDxe/X64, under the names SevCopyMem() and
InternalSevCopyMem(), and call SevCopyMem() in the CommonBuffer cases of
Map() and Unmap(), for the in-place flipping.
For the 32-bit case (OvmfPkgIa32.dsc), my understanding is that guests
cannot control the C bit at all (there is no C bit in the PTEs), and
memory is always encrypted. Is that correct? If so, then we only need to
ensure that SevCopyMem() compile, as it will never be called -- in the
entry point function of OvmfPkg/IoMmuDxe, MemEncryptSevIsEnabled() will
return FALSE, and so the IOMMU protocol will not be installed. Therefore
the 32-bit version (under OvmfPkg/IoMmuDxe/Ia32) of SevCopyMem() can be
stubbed out as an ASSERT(FALSE)+CpuDeadLoop().
If you can think of a better location for SevCopyMem(), that's fine as
well. For example, you could add it to
"OvmfPkg/Library/BaseMemEncryptSevLib" as well.
... I don't think this functionality should be added under MdePkg,
because it is *very* special to the IOMMU implementation, and
practically no other module should use a "busy" in-place CopyMem().
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
