On 07/28/17 18:00, Brijesh Singh wrote:
>
>
> On 07/28/2017 08:38 AM, Laszlo Ersek wrote:
>> On 07/27/17 21:00, Brijesh Singh wrote:
>>> Actually one of main reason why we cleared and restored the memory
>>> encryption mask during allocate/free is because we also consume the
>>> IOMMU protocol in QemuFwCfgLib as a method to allocate and free a
>>> DMA buffer. I am certainly open to suggestions.
>>
>> Argh. That's again my fault then, I should have noticed it. :( I
>> apologize, the Map/Unmap/AllocateBuffer/FreeBuffer APIs are a "first"
>> for me as well.
>>
>> As discussed earlier (and confirmed by Ard), calling *just*
>> AllocateBuffer() is never enough, Map()/Unmap() are always necessary.
>>
>> So here's what I suggest (to keep the changes localized):
>>
>> - Extend the prototype of InternalQemuFwCfgSevDmaAllocateBuffer()
>> function to output a "VOID *Mapping" parameter as well, in addition
>> to the address.
>>
>> - Modify the prototype of the InternalQemuFwCfgSevDmaFreeBuffer()
>> function to take a "VOID *Mapping" parameter in addition to the
>> buffer address.
>>
>> - In the DXE implementation of
>> InternalQemuFwCfgSevDmaAllocateBuffer(), keep the current IOMMU
>> AllocateBuffer() call, but also call IOMMU Map(), with
>> CommonBuffer. Furthermore, propagate the Mapping output of Map()
>> outwards. (Note that Map() may have to be called in a loop,
>> dependent on "NumberOfBytes".)
>>
>> - In the DXE implementation of InternalQemuFwCfgSevDmaFreeBuffer(),
>> call the IOMMU Unmap() function first (using the new Mapping
>> parameter).
>>
>
> I will update the code and send patch for review. thanks
Here's an alternative, given that you mentioned being
performance-conscious. I'm not "suggesting" this as preferred, just
offering it for your consideration. Pick whichever you like more.
In this approach, I'm going to go back on my original request, namely to
keep the InternalQemuFwCfgDmaBytes() implementation centralized, in the
"QemuFwCfgLib.c" file. I now realize that the differences are large
enough that this may not have been a good idea. So here goes:
* Internal APIs ("QemuFwCfgLibInternal.h"):
- Remove the InternalQemuFwCfgSev*() function prototypes.
- Introduce the InternalQemuFwCfgDmaBytes() function prototype.
* SEC instance ("QemuFwCfgSec.c"):
- Remove the InternalQemuFwCfgSev*() function definitions.
- Add the InternalQemuFwCfgDmaBytes() function definition with
ASSERT(FALSE) + CpuDeadLoop().
* PEI instance ("QemuFwCfgPei.c):
- Remove the InternalQemuFwCfgSev*() function definitions.
- Copy the InternalQemuFwCfgDmaBytes() function definition from the
currently shared code ("QemuFwCfgLib.c"), and remove all branches
that are related to the SEV enabled case. IOW, specialize the
implementation to the plaintext case.
- In QemuFwCfgInitialize(), replace the
InternalQemuFwCfgSevIsEnabled() function call with a direct call to
MemEncryptSevIsEnabled().
* DXE instance ("QemuFwCfgDxe.c"):
- Remove the InternalQemuFwCfgSev*() function definitions.
- Copy the InternalQemuFwCfgDmaBytes() function definition from the
currently shared code ("QemuFwCfgLib.c"), as a starting point.
- Replace the InternalQemuFwCfgSevIsEnabled() call with a direct call
to MemEncryptSevIsEnabled().
- When SEV is enabled, split the control area ("FW_CFG_DMA_ACCESS") to
a separate buffer. This control area should be allocated with IOMMU
AllocateBuffer(), and Map()ped separately, as
BusMasterCommonBuffer64.
- For the data transfer buffer, use *only* Map() and Unmap(), without
AllocateBuffer() and FreeBuffer(). Use BusMasterRead64 and
BusMasterWrite64, dependent on FW_CFG_DMA_CTL_WRITE /
FW_CFG_DMA_CTL_READ. The point is that the potentially large data
area will be bounced only once, because Map()/Unmap() will own the
bounce buffer, and the in-place (de)crypting can be avoided. (The
fw_cfg DMA transfer is completed in one synchronous operation, as
witnessed by the library client.) The explicit CopyMem() calls can
be removed.
- Remove the InternalQemuFwCfgSevDmaAllocateBuffer() and
InternalQemuFwCfgSevDmaFreeBuffer() calls.
* shared code ("QemuFwCfgLib.c"):
- remove the InternalQemuFwCfgDmaBytes() function definition.
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
