Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support

[Laszlo Ersek <lersek@redhat.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:

>>> I agree with you, there is a bug in AmdSevIoMmu.c. I will fix it. And
>>> introduce list to track if the buffer was allocated by us. If buffer
>>> was allocated by us then we can safely say that it does not require a
>>> bounce buffer. While implementing the initial code I was not able to
>>> see BusMasterCommonBuffer usage. But with virtio things are getting a
>>> bit more clear in my mind.
>>
>> The purpose of the internal list is a little bit different -- it is a
>> "free list", not a tracker list.
>>
>> Under the proposed scheme, a MAP_INFO structure will have to be
>> allocated for *all* mappings: both for CommonBuffer (where the actual
>> pages come from the caller, who retrieved them earlier with an
>> AllocateBuffer() call), and for Read/Write (where the actual pages will
>> be allocated internally to Map). Allocating the MAP_INFO structure in
>> Map() is necessary in *both* cases because the Unmap() function can
>> *only* consult this MAP_INFO structure to learn the address and the size
>> at which it has to re-set the memory encryption mask. This is because
>> the Unmap() function gets no other input parameter.
>>
>> Then, regardless of the original operation (CommonBuffer vs.
>> Read/Write), the MAP_INFO structure has to be recycled in Unmap(). (For
>> CommonBuffer, the actual pages are owned by the caller, so we don't free
>> them here; for Read/Write, the actual pages are owned by Map/Unmap, so
>> we free them in Unmap() *in addition* to recycling MAP_INFO.) The main
>> point is that MAP_INFO cannot be released with FreePool() because that
>> could change the UEFI memmap during ExitBootServices() processing. So
>> MAP_INFO has to be "released" to an internal *free* list.
>>
>> And since we have an internal free list, the original MAP_INFO
>> allocation in Map() should consult the free list first, and only if it
>> is empty should it fall back to AllocatePool().
>>
>> Whether the actual pages tracked by MAP_INFO were allocated internally
>> by Map(), or externally by the caller (via AllocateBuffer()) is an
>> independent question. (And it can be seen from the MAP_INFO.Operation
>> field.)
>>
>> Does this make it clearer?
>>
> 
> It makes sense. thanks
> 
> One question, do we need to return EFI_NOT_SUPPORTED error when we get
> request to map a buffer with CommonBuffer but the buffer was not
> allocated using "EFI_PCI_IO_PROTOCOL->AllocateBuffer (...)"?
> 
> At least as per spec, it seems if caller wants to map a buffer with
> CommonBuffer then it must have called
> "EFI_PCI_IO_PROTOCOL->AllocateBuffer (...)"

Yes, that is it, exactly: if the caller violates a requirement in the
UEFI spec, covering the use of the APIs, then the firmware *may* make an
attempt to detect that (and to reject it), but the firmware is not
*required* to do so.

A much simpler example: on a double-free programming error, the second
gBS->FreePool() call is not *required* to detect the problem. ("The
Buffer that is freed must have been allocated by AllocatePool().")

So, I don't think we need to implement measures for checking that a
CommonBuffer Map() actually refers to a previously returned, active,
AllocateBuffer() address.

(Snipping the rest, I've read it all, thanks for the answers. Nothing to
add this time :) )

Cheers!
Laszlo