From: Laszlo Ersek <lersek@redhat.com>
To: Brijesh Singh <brijesh.singh@amd.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Tom Lendacky <thomas.lendacky@amd.com>,
Jordan Justen <jordan.l.justen@intel.com>,
Jason Wang <jasowang@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
Date: Fri, 28 Jul 2017 21:21:12 +0200 [thread overview]
Message-ID: <2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com> (raw)
In-Reply-To: <217545ac-962d-089f-9c9a-d2bbfca6427e@amd.com>
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
next prev parent reply other threads:[~2017-07-28 19:19 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-19 22:09 [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support Brijesh Singh
2017-07-19 22:09 ` [RFC v1 1/3] OvmfPkg/Include/Virtio10: Define VIRTIO_F_IOMMU_PLATFORM feature bit Brijesh Singh
2017-07-19 22:09 ` [RFC v1 2/3] OvmfPkg/VirtioLib: Add IOMMU_PLATFORM support Brijesh Singh
2017-07-19 22:09 ` [RFC v1 3/3] OvmfPkg/VirtioBlkDxe: Add VIRITO_F_IOMMU_PLATFORM support Brijesh Singh
[not found] ` <62320c1a-0cec-947c-8c63-5eb0416e4e33@redhat.com>
2017-07-21 11:17 ` [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support Brijesh Singh
[not found] ` <20170722024318-mutt-send-email-mst@kernel.org>
2017-07-24 8:25 ` Gerd Hoffmann
2017-07-25 18:17 ` Laszlo Ersek
2017-07-25 23:42 ` Brijesh Singh
[not found] ` <904dae9f-e515-01ba-e16f-6561616c78af@redhat.com>
2017-07-26 15:30 ` Laszlo Ersek
2017-07-27 14:21 ` Brijesh Singh
2017-07-27 17:16 ` Laszlo Ersek
2017-07-27 17:56 ` Ard Biesheuvel
2017-07-27 19:00 ` Brijesh Singh
2017-07-27 20:55 ` Brijesh Singh
2017-07-27 21:31 ` Ard Biesheuvel
2017-07-27 21:38 ` Andrew Fish
2017-07-27 22:13 ` Brijesh Singh
2017-07-27 22:10 ` Brijesh Singh
2017-07-28 8:39 ` Ard Biesheuvel
2017-07-28 15:27 ` Laszlo Ersek
2017-07-28 13:38 ` Laszlo Ersek
2017-07-28 16:00 ` Brijesh Singh
2017-07-28 16:16 ` Laszlo Ersek
2017-07-28 19:21 ` Laszlo Ersek [this message]
2017-07-28 19:59 ` Laszlo Ersek
2017-07-29 0:52 ` Brijesh Singh
2017-07-29 1:37 ` Brijesh Singh
2017-07-31 18:20 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox