Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Brijesh Singh <brijesh.singh@amd.com>]

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