public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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


  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