Thanks Laszlo.
On 8/1/17 4:59 PM, Laszlo Ersek wrote:
> On 07/31/17 21:31, Brijesh Singh wrote:
> +
[Snip]
>> The function is used for mapping and unmapping the Host buffer with
>> + BusMasterCommonBuffer. Since the buffer can be accessed equally by the
>> + processor and the DMA bus master hence we can not use the bounce buffer.
>> +
>> + The function changes the underlying encryption mask of the pages that
>> maps the
>> + host buffer. It also ensures that buffer contents are updated with the
>> desired
>> + state.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetBufferAsEncDec (
>> + IN MAP_INFO *MapInfo,
>> + IN BOOLEAN Enc
>> + )
>> +{
>> + EFI_STATUS Status;
>> + EFI_PHYSICAL_ADDRESS TempBuffer;
>> +
>> + //
>> + // Allocate an intermediate buffer to hold the host buffer contents
>> + //
>> + Status = gBS->AllocatePages (
>> + AllocateAnyPages,
>> + EfiBootServicesData,
>> + MapInfo->NumberOfPages,
>> + &TempBuffer
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
> This is not right. This function is called (indirectly) from
> IoMmuUnmap(), which is the function that we'll call (also indirectly)
> from the ExitBootServices() callbacks. That means we cannot allocate or
> release dynamic memory here. This is why I suggested the page-sized
> static buffer, and page-wise copying.
>
> More on this later, below the end of this function.
Agree with your point, see more below
>> +
>> + //
>> + // If the host buffer has C-bit cleared, then make sure the intermediate
>> + // buffer matches with same encryption mask.
>> + //
>> + if (!Enc) {
>> + Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
>> + MapInfo->NumberOfPages, TRUE);
>> + ASSERT_EFI_ERROR (Status);
>> + }
> As a separate issue, I don't think this is right. The auxiliary buffer
> that we use for in-place encryption or decryption should *always* be
> encrypted. (I mentioned this in my email, "Introduce a static UINT8
> array with EFI_PAGE_SIZE bytes (this will always remain in encrypted
> memory).", but I guess it was easy to miss.)
See more below
>> +
>> + //
>> + // Copy the data from host buffer into a temporary buffer. At this
>> + // time both host and intermediate buffer will have same encryption
>> + // mask.
>> + //
> The comment should say "copy original buffer into temporary buffer".
> "host" buffer is very confusing here, as the virtualization host is
> exactly what may not yet have access to the buffer. In PCI bus master
> terminology, "host buffer" is valid, I think, but saying just "host
> buffer" is very confusing.
Will do.
> Also, "same encryption mask" is not desirable (see above), so please
> remove that sentence.
>
>> + CopyMem (
>> + (VOID *) (UINTN) TempBuffer,
>> + (VOID *) (UINTN)MapInfo->HostAddress,
>> + MapInfo->NumberOfBytes);
>> +
>> + //
>> + // Now change the encryption mask of the host buffer
>> + //
>> + if (Enc) {
>> + Status = MemEncryptSevSetPageEncMask (0, MapInfo->HostAddress,
>> + MapInfo->NumberOfPages, TRUE);
>> + ASSERT_EFI_ERROR (Status);
> What guarantees that this function call will never fail?
>
> If it fails, we should propagate the error (the function prototype
> allows for that), and roll back partial changes along the way.
>
> If there is no way to undo partial actions when
> MemEncryptSevSetPageEncMask() fails, then the ASSERT() is fine, but we
> need an additional CpuDeadLoop() as well.
I will propagate the error.
>> + } else {
>> + Status = MemEncryptSevClearPageEncMask (0, MapInfo->HostAddress,
>> + MapInfo->NumberOfPages, TRUE);
>> + ASSERT_EFI_ERROR (Status);
>> + }
>> +
>> + //
>> + // Copy the data from intermediate buffer into host buffer. At this
>> + // time encryption masks will be different on host and intermediate
>> + // buffer and the hardware will perform encryption/decryption on
>> + // accesses.
>> + //
>> + CopyMem (
>> + (VOID *) (UINTN)MapInfo->HostAddress,
>> + (VOID *) (UINTN)TempBuffer,
>> + MapInfo->NumberOfBytes);
>> +
>> + //
>> + // Restore the encryption mask of the intermediate buffer
>> + //
>> + if (!Enc) {
>> + Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
>> + MapInfo->NumberOfPages, TRUE);
>> + ASSERT_EFI_ERROR (Status);
>> + }
> Again, the intermediate buffer should always remain encrypted.
>
>> +
>> + //
>> + // Free the intermediate buffer
>> + //
>> + gBS->FreePages (TempBuffer, MapInfo->NumberOfPages);
>> + return EFI_SUCCESS;
>> +}
> In summary for this function: I've now read section "7.10.8
> Encrypt-in-Place" of AMD pub #24593. Earlier I wrote,
>
> 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.
>
> With regard to 7.10.8., this appears to be wrong. My idea was based on
> the expectation that changing the C bit will not affect *guest reads*
> from the same area. According to 7.10.8, my assumption was wrong: the
> algorithm in 7.10.8 uses a cache-line sized bounce buffer and *two*
> separate mappings for the area under in-place encryption. The reading
> always occurs through the original mapping.
>
> Now, this requires us to do one of two things:
>
> - Alternative 1: we can use the static, single page-sized buffer, in a
> loop. But then we must also break up the central
> MemEncryptSevSetPageEncMask() / MemEncryptSevClearPageEncMask() calls to
> small, page-sized calls. Copy out a page, change the C bit for that one
> page, copy back the page. Lather, rinse, repeat. This would likely be
> slow, and would guarantee that 2M pages would be split into 4K pages.
Yes, while implementing the code I had similar concern in my mind hence
I dropped the looping per page idea. So far, I have not seen really huge
request for BusMasterCommonBuffer Map(). The max I remember was 16 pages
when using the Ata drivers. It may not be as bad as we are thinking.
> - Alternative 2: we can stick with the large (unified) CopyMem() and
> C-bit changing function calls, but we still can't allocate memory for
> that in the SetBufferAsEncDec() function. Therefore you will have to
> allocate *twice* the requested number of pages in AllocateBuffer(),
> return the address of the lower half to the caller, and use the upper
> half as intermediate buffer in SetBufferAsEncDec(). This is safe because
> CommonBuffer[64] Map/Unmap requires the caller to pass in allocations
> from AllocateBuffer().
Yes, we could do something like this. thank for tip. This will certainly
will not affect the perform. I may give this as a first try in next rev.
>> +
>> +/**
>> + This function will be called by Map() when mapping the buffer buffer to
>> + BusMasterCommonBuffer type.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetHostBufferAsEncrypted (
>> + IN MAP_INFO *MapInfo
>> + )
>> +{
>> + return SetBufferAsEncDec (MapInfo, TRUE);
>> +}
>> +
>> +/**
>> + This function will be called by Unmap() when unmapping host buffer
>> + from the BusMasterCommonBuffer type.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetHostBufferAsDecrypted (
>> + IN MAP_INFO *MapInfo
>> + )
>> +{
>> + return SetBufferAsEncDec (MapInfo, FALSE);
>> +}
>>
>> /**
>> Provides the controller-specific addresses required to access system
>> memory from a
>> @@ -113,18 +233,6 @@ IoMmuMap (
>> }
>>
>> //
>> - // CommandBuffer was allocated by us (AllocateBuffer) and is already in
>> - // unencryted buffer so no need to create bounce buffer
>> - //
>> - if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>> - Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>> - *Mapping = NO_MAPPING;
>> - *DeviceAddress = PhysicalAddress;
>> -
>> - return EFI_SUCCESS;
>> - }
>> -
>> - //
>> // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>> // called later.
>> //
> Please implement the free-list idea that I outlined earlier. Unmap()
> must not call FreePool() on the MAP_INFO structures.
Sorry it was my bad. I missed implementing that feedback. I will fix it
in next rev. As you have outlined in previous feedback that Unmap() can
be called from ExitBootService() hence i will refrain from using
FreePool() or MemEncryptSev*() functions in this sequence (except when
freeing the actual bounce buffer).
>> @@ -144,6 +252,25 @@ IoMmuMap (
>> MapInfo->DeviceAddress = DmaMemoryTop;
>>
>> //
>> + // If the requested Map() operation is BusMasterCommandBuffer then map
>> + // using internal function otherwise allocate a bounce buffer to map
>> + // the host buffer to device buffer
>> + //
>> + if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>> + Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>> +
>> + Status = SetHostBufferAsDecrypted (MapInfo);
>> + if (EFI_ERROR (Status)) {
>> + FreePool (MapInfo);
>> + *NumberOfBytes = 0;
>> + return Status;
>> + }
>> +
>> + MapInfo->DeviceAddress = MapInfo->HostAddress;
>> + goto Done;
>> + }
> Please use a regular "else" branch here; we should use "goto" only for
> jumping to error handling code.
Will do thanks
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
