Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 7/28/17 7:52 PM, Brijesh Singh wrote: [snip] > AMD APM document a procedure which must be used to perform in-place > encryption/decryption. We must follow those steps to ensure that data is > flush into memory using the correct C-bit. Not doing so may result in > unpredictable results. > > http://support.amd.com/TechDocs/24593.pdf (Section 7.10.8) I am wondering if UEFI provides APIs to get two linear mapping of the same physical address. The steps says we create two mapping of same physical address with different C-bits. I will look into UEFI docs to see if something like this exist otherwise we have to consider two memcpy. Since its just for CommandBuffers (which usually are smaller hence we may be okay from performance point of view. Also its just a boot time thing, does not get used when guest OS is takes over. I will investigate and see what works best without adding extra complexity in the code :) -Brijesh ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 7/28/17 2:59 PM, Laszlo Ersek wrote:
> On 07/28/17 18:00, Brijesh Singh wrote:
>> On 07/28/2017 08:38 AM, Laszlo Ersek wrote:
> snip
>
>>> (b) Plus, approaching the question from the Map() direction, we need
>>> to consider two scenarios:
>>>
>>> - Client code calls AllocateBuffer(), then Map(), and it writes to
>>> the buffer only then. This should be safe.
>>> - client code calls AllocateBuffer(), writes to it, and then calls
>>> Map(). This will result in memory contents that look like garbage
>>> to the hypervisor. Bad.
>>>
>>> I can imagine the following to handle these cases: in the Map() and
>>> Unmap() functions, we have to decrypt and encrypt the memory contents
>>> in-place, after changing the C bit (without allocating additional
>>> memory). 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. In effect this will in-place encrypt or decrypt the memory,
>>> and will be faster than a byte-wise rewrite.
> snip
>
>>> * BusMasterCommonBuffer:
>>>- Client calls AllocateBuffer(), and places some data in the
>>> returned memory.
>>>- Client calls Map(). Map() clears the C bit in one fell swoop,
>>> and then decrypts the buffer in-place (by bouncing it page-wise
>>> to the static array and back).
>>>- Client communicates with the device.
>>>- Client calls Unmap(). Unmap() restores the C bit in one fell
>>> swoop, and encrypts the buffer in-place (by bouncing it
>>> page-wise to the static array and back).
>>>- Client reads some residual data from the buffer.
>>>- Client calls FreeBuffer(). FreeBuffer() relases the pages.
>>>
>> Yes this works fine as long as the client uses
>> EFI_PCI_IO_PROTOCOL.AllocateBuffer() to allocate the buffer.
> Again, a performance-oriented thought:
>
> Above I suggested using a statically allocated page-sized buffer, for
> the in-place encryption/decryption. Ultimately this means *two*
> CopyMem()s for the entire buffer (just executed page-wise), in *each* of
> Map() and Unmap().
>
> Maybe we can do better: what if you perform the CopyMem() from the
> buffer right back to the same buffer? CopyMem() is *required* to work
> with overlapping source and target areas (similarly to memmove() in
> standard C).
>
> This would result in *one* CopyMem (for in-place de-/encryption) in each
> of Map() and Unmap(), and thereby it would have identical performance
> impact to the BusMasterRead and BusMasterWrite Map() operations (where
> copying / crypting takes place between distinct memory areas).
>
> The OVMF DSC files resolve "BaseMemoryLib" -- which provides CopyMem()
> -- to "MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf";
> regardless of module type. The actual implementation appears to reside
> in "MdePkg/Library/BaseMemoryLibRepStr/X64/CopyMem.nasm":
>
AMD APM document a procedure which must be used to perform in-place
encryption/decryption. We must follow those steps to ensure that data is
flush into memory using the correct C-bit. Not doing so may result in
unpredictable results.
http://support.amd.com/TechDocs/24593.pdf (Section 7.10.8)
>> global ASM_PFX(InternalMemCopyMem)
>> ASM_PFX(InternalMemCopyMem):
>> pushrsi
>> pushrdi
>> mov rsi, rdx; rsi <- Source
>> mov rdi, rcx; rdi <- Destination
>> lea r9, [rsi + r8 - 1] ; r9 <- End of Source
>> cmp rsi, rdi
>> mov rax, rdi; rax <- Destination as return value
>> jae .0
>> cmp r9, rdi
>> jae @CopyBackward ; Copy backward if overlapped
>> .0:
>> mov rcx, r8
>> and r8, 7
>> shr rcx, 3
>> rep movsq ; Copy as many Qwords as possible
>> jmp @CopyBytes
>> @CopyBackward:
>> mov rsi, r9 ; rsi <- End of Source
>> lea rdi, [rdi + r8 - 1] ; esi <- End of Destination
>> std ; set direction flag
>> @CopyBytes:
>> mov rcx, r8
>> rep movsb ; Copy bytes backward
>> cld
>> pop rdi
>> pop rsi
>> ret
>>
> However, I'm afraid even if this works on SEV (which I certainly
> expect!), this code won't be reached, due to the following CopyMem()
> wrapper implementation in
> "MdePkg/Library/BaseMemoryLibRepStr/CopyMemWrapper.c":
>
>> VOID *
>> EFIAPI
>> CopyMem (
>> OUT VOID *DestinationBuffer,
>> IN CONST VOID *SourceBuffer,
>> IN UINTN Length
>> )
>> {
>> if (Length == 0) {
>> return DestinationBuffer;
>> }
>> ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)DestinationBuffer));
>> ASSERT ((Leng
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 07/28/17 18:00, Brijesh Singh wrote:
> On 07/28/2017 08:38 AM, Laszlo Ersek wrote:
snip
>> (b) Plus, approaching the question from the Map() direction, we need
>> to consider two scenarios:
>>
>> - Client code calls AllocateBuffer(), then Map(), and it writes to
>> the buffer only then. This should be safe.
>> - client code calls AllocateBuffer(), writes to it, and then calls
>> Map(). This will result in memory contents that look like garbage
>> to the hypervisor. Bad.
>>
>> I can imagine the following to handle these cases: in the Map() and
>> Unmap() functions, we have to decrypt and encrypt the memory contents
>> in-place, after changing the C bit (without allocating additional
>> memory). 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. In effect this will in-place encrypt or decrypt the memory,
>> and will be faster than a byte-wise rewrite.
snip
>> * BusMasterCommonBuffer:
>>- Client calls AllocateBuffer(), and places some data in the
>> returned memory.
>>- Client calls Map(). Map() clears the C bit in one fell swoop,
>> and then decrypts the buffer in-place (by bouncing it page-wise
>> to the static array and back).
>>- Client communicates with the device.
>>- Client calls Unmap(). Unmap() restores the C bit in one fell
>> swoop, and encrypts the buffer in-place (by bouncing it
>> page-wise to the static array and back).
>>- Client reads some residual data from the buffer.
>>- Client calls FreeBuffer(). FreeBuffer() relases the pages.
>>
>
> Yes this works fine as long as the client uses
> EFI_PCI_IO_PROTOCOL.AllocateBuffer() to allocate the buffer.
Again, a performance-oriented thought:
Above I suggested using a statically allocated page-sized buffer, for
the in-place encryption/decryption. Ultimately this means *two*
CopyMem()s for the entire buffer (just executed page-wise), in *each* of
Map() and Unmap().
Maybe we can do better: what if you perform the CopyMem() from the
buffer right back to the same buffer? CopyMem() is *required* to work
with overlapping source and target areas (similarly to memmove() in
standard C).
This would result in *one* CopyMem (for in-place de-/encryption) in each
of Map() and Unmap(), and thereby it would have identical performance
impact to the BusMasterRead and BusMasterWrite Map() operations (where
copying / crypting takes place between distinct memory areas).
The OVMF DSC files resolve "BaseMemoryLib" -- which provides CopyMem()
-- to "MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf";
regardless of module type. The actual implementation appears to reside
in "MdePkg/Library/BaseMemoryLibRepStr/X64/CopyMem.nasm":
> global ASM_PFX(InternalMemCopyMem)
> ASM_PFX(InternalMemCopyMem):
> pushrsi
> pushrdi
> mov rsi, rdx; rsi <- Source
> mov rdi, rcx; rdi <- Destination
> lea r9, [rsi + r8 - 1] ; r9 <- End of Source
> cmp rsi, rdi
> mov rax, rdi; rax <- Destination as return value
> jae .0
> cmp r9, rdi
> jae @CopyBackward ; Copy backward if overlapped
> .0:
> mov rcx, r8
> and r8, 7
> shr rcx, 3
> rep movsq ; Copy as many Qwords as possible
> jmp @CopyBytes
> @CopyBackward:
> mov rsi, r9 ; rsi <- End of Source
> lea rdi, [rdi + r8 - 1] ; esi <- End of Destination
> std ; set direction flag
> @CopyBytes:
> mov rcx, r8
> rep movsb ; Copy bytes backward
> cld
> pop rdi
> pop rsi
> ret
>
However, I'm afraid even if this works on SEV (which I certainly
expect!), this code won't be reached, due to the following CopyMem()
wrapper implementation in
"MdePkg/Library/BaseMemoryLibRepStr/CopyMemWrapper.c":
> VOID *
> EFIAPI
> CopyMem (
> OUT VOID *DestinationBuffer,
> IN CONST VOID *SourceBuffer,
> IN UINTN Length
> )
> {
> if (Length == 0) {
> return DestinationBuffer;
> }
> ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)DestinationBuffer));
> ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)SourceBuffer));
>
> if (DestinationBuffer == SourceBuffer) {
> return DestinationBuffer;
> }
> return InternalMemCopyMem (DestinationBuffer, SourceBuffer, Length);
> }
As you see, (DestinationBuffer == SourceBuffer) is handled as a no-op
(quite justifiedly, except in the case of SEV).
Personally I think it would be OK to copy the wrapper function and the
assembly code to OvmfPkg/IoMmuDxe/X64, under the names SevCopyMem
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 07/28/2017 08:38 AM, Laszlo Ersek wrote: On 07/27/17 21:00, Brijesh Singh wrote: On 07/27/2017 12:56 PM, Ard Biesheuvel wrote: On 27 July 2017 at 18:16, Laszlo Ersek wrote: Normally, Map allocates the bounce buffer internally, and Unmap releases the bounce buffer internally (for BusMasterRead and BusMasterWrite), which is not right here. If you use AllocateBuffer() separately, then Map() -- with BusMasterCommonBuffer -- will not allocate internally, and Unmap() will also not deallocate internally. So, in the ExitBootServices() callback, you will be able to call Unmap() only, and then *not* call FreeBuffer() at all. This is why I suggest introducing all four functions (Allocate, Free, Map, Unmap) to the VIRTIO_DEVICE_PROTOCOL, and why all virtio drivers should use all four functions explicitly, not just Map and Unmap. ... Actually, I think there is a bug in "OvmfPkg/IoMmuDxe/AmdSevIoMmu.c". You have the following distribution of operations at the moment: - IoMmuAllocateBuffer() allocates pages and clears the memory encryption mask - IoMmuFreeBuffer() re-sets the memory encryption mask, and deallocates pages - IoMmuMap() does nothing at all when BusMasterCommonBuffer is requested (and IoMmuAllocateBuffer() was called previously). Otherwise, IoMmuMap() allocates pages, allocates MAP_INFO, and clears the memory encryption mask. - IoMmuUnmap() does nothing when cleaning up a BusMasterCommonBuffer operation (--> NO_MAPPING). Otherwise, IoMmuUnmap() clears the encryption mask, and frees both the pages and MAP_INFO. 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 (...)" This distribution of operations seems wrong. The key point is that AllocateBuffer() *need not* result in a buffer that is immediately usable, and that client code is required to call Map() *unconditionally*, even if BusMasterCommonBuffer is the desired operation. Therefore, the right distribution of operations is: - IoMmuAllocateBuffer() allocates pages and does not touch the encryption mask.. - IoMmuFreeBuffer() deallocates pages and does not touch the encryption mask. 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,
Re: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top]
On 07/28/17 12:19, Ni, Ruiyu wrote: > Laszlo, > Thanks for raising this questions. > Please see my embedded reply in below > > Thanks/Ray > >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, July 27, 2017 7:23 PM >> To: Ni, Ruiyu ; edk2-devel@lists.01.org >> Cc: Marcel Apfelbaum ; Aleksandr Bezzubikov >> ; Zeng, Star >> Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid >> hang when BUS pad resource is not in top] >> >> Hello Ray, >> >> please excuse me for hijacking this thread a little bit, but I'm very happy >> that >> you happen to be looking at this exact part of the code, which is also what >> I've been discussing with Marcel and Aleksandr (CC'd) due to a different >> reason. >> >> (The relevant sub-thread is at >> >> Allow RedHat PCI bridges reserve more buses than necessary during init >> >> http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f- >> 2ceb6180f...@redhat.com >> >> but it's very long, so I'm not asking you to read that -- instead, please >> let me >> ask you our question independently and in a self-contained >> form.) >> >> In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource >> paddings, from the >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() >> function. >> >> QEMU has no "root" hotplug controllers, only "non-root" hotplug controllers >> (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protocol" >> in >> the PI-1.6 spec). Therefore OVMF's >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty >> array. >> The BUS resource padding that we'd like to return from >> GetResourcePadding() would be for "non-root" hotplug controllers. >> >> According to the PI spec, this is a valid thing to do: >> >> * From "12.5 Sample Implementation for a Platform Containing PCI >> Hot Plug* Slots": >> >>> [...] For all the root HPCs and the nonroot HPCs, call >>> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() to obtain the >>> amount of overallocation and add that amount to the requests from the >>> physical devices. Reprogram the bus numbers by taking into account the >>> bus resource padding information. [...] >> >> * From "12.6 PCI Hot Plug PCI Initialization Protocol, >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": >> >>> [...] This member function is called for all the root HPCs and nonroot >>> HPCs that are detected by the PCI bus enumerator. [...] The PCI bus >>> driver is responsible for adding this resource request to the resource >>> requests by the physical PCI devices. [...] >> >> I searched PciBusDxe for GetResourcePadding() call sites, and there are >> two: >> >> (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function >> GetResourcePaddingForHpb(). This call site seems to work as follows, >> for our purposes: >> >> GetResourcePaddingPpb() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> GetResourcePaddingForHpb() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c] >> gPciHotPlugInit->GetResourcePadding() >> // >> // and then save the resource descriptors, including the bus >> // padding, in PciIoDevice->ResourcePaddingDescriptors >> // >> >> ResourcePaddingPolicy() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> ApplyResourcePadding() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> // >> // translate IO and MMIO resources from >> // PciDev->ResourcePaddingDescriptors to internal padding >> // representation >> // >> >> This call site and processing seem to be active for "non-root" >> hotplug controllers, but they do not consider bus numbers, only IO >> and MMIO. > [Ray] It's in bus allocation phase. So the PciBus driver only cares about the > bus padding. > PciBus driver firstly allocates the bus resource and programs the PCI-PCI > bridges > to set the bus numbers. > After that, PciBus allocates the IO/MMIO resources and program the BARs in > PCI devices & PCI-PCI bridges. > So it's good here. >> >> (2) In the PciScanBus() function, file >> "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another >> GetResourcePadding() call -- your present patch is updating this >> code part. >> >> From the resource padding list returned by this GetResourcePadding() >> invocation, PciBusDxe cares only about the bus numbers. I think >> that's probably fine. However, in this location, >> GetResourcePadding() is called *only* for "root" hotplug >> controllers, of which QEMU has none. >> >> The end result is that bus number paddings are not considered at all for >> "non-root" hotplug controllers: > [Ray] Bus number padding should be considered for "non-root" HPCs. > So I agree it's a bug/gap I need to fix. I noticed that already before your > question. > And I do have a plan to fix it though the priority is not very high. A
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 07/28/17 10:39, Ard Biesheuvel wrote: > On 27 July 2017 at 23:10, Brijesh Singh wrote: >> >> >> On 07/27/2017 04:31 PM, Ard Biesheuvel wrote: >>> >>> On 27 Jul 2017, at 21:55, Brijesh Singh wrote: On 07/27/2017 02:00 PM, Brijesh Singh wrote: >>> This distribution of operations seems wrong. The key point is that >>> AllocateBuffer() *need not* result in a buffer that is immediately >>> usable, and that client code is required to call Map() >>> *unconditionally*, even if BusMasterCommonBuffer is the desired >>> operation. Therefore, the right distribution of operations is: >>> >>> - IoMmuAllocateBuffer() allocates pages and does not touch the >>>encryption mask.. >>> >>> - IoMmuFreeBuffer() deallocates pages and does not touch the >>> encryption >>>mask. >>> > 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. > [1] > https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L159 > [2] > https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L197 >>> >>> - IoMmuMap() does not allocate pages when BusMasterCommonBuffer is >>>requested, and it allocates pages (bounce buffer) otherwise. >>> > I am trying to wrap my head around how we can support > BusMasterCommonBuffer > when buffer was not allocated by us. Changing the memory encryption mask > in > a page table will not update the contents. Also since the memory > encryption > mask works on PAGE_SIZE hence changing the encryption mask on not our > allocated > buffer could mess things up (e.g if NumberOfBytes is not PAGE_SIZE > aligned). I may be missing something in my understanding. Here is a flow I have in my mind, please correct me. OvmfPkg/VirtIoBlk.c: VirtioBlkInit() VirtioRingInit Virtio->AllocateSharedPages(RingSize, &Ring->Base) PciIo->AllocatePages(RingSize, &RingAddress) Virtio->MapSharedPages(...,BusMasterCommonBuffer, Ring->Base, RingSize, &RingDeviceAddress) . . This case is straight forward and we can easily maps. No need for bounce buffering. VirtioBlkReadBlocks(..., BufferSize, Buffer,) .. .. SynchronousRequest(..., BufferSize, Buffer) Virtio->MapSharedPages(..., BusMasterCommonBuffer, Buffer, BufferSize, &DeviceAddress) VirtioAppendDesc(DeviceAddress, BufferSize, ...) VirtioFlush (...) In the above case, "Buffer" was not allocated by us hence we will not able to change the memory encryption attributes. Am I missing something in the flow ? >>> >>> >>> Common buffer mappings may only be created from buffers that were >>> allocated by AllocateBuffer(). In fact, that is its main purpose >> >> >> Yes, that part is well understood. If the buffer was allocated by us (e.g >> vring, request/status >> structure etc) then those should be mapped as "BusMasterCommonBuffer". Brijesh, thanks for the clarification. Previously I replied (at length) to your paragraph that said "trying to wrap my head around...", and it wasn't clear what you meant by "allocated by us". In my previous response, I assumed that you meant a distinction between "allocated in Map()" vs. "allocated in AllocateBuffer()". I stand by my earlier answer for that (assumed) distinction, but now I see that you meant something else. >> >> But I am trying to figure out, how to map a data buffers before issuing a >> virtio request. e.g when >> VirtioBlkReadBlocks() is called, "Buffer" pointer is not a DMA address hence >> we need to map it. >> I think it should be mapped using "BusMasterWrite" not >> "BusMasterCommonBuffer" before adding into vring. >> > > If the transfer is strictly unidirectional, then that should work. If > the transfer goes both ways, you may need to map/unmap for read and > then map/unmap for write > You (Brijesh and Ard) are both right. This question depends on the outermost interface that the specific virtio driver provides. In this case, VirtioBlkReadBlocks() implements EFI_BLOCK_IO_PROTOCOL.ReadBlocks(). The buffer is owned by an independent agent, and it is guaranteed by the ReadBlocks() interface that the transfer is unidirectional. So a standalone Map() call with BusMasterWrite is appropriate, followed by a standalone Unmap() call *before* VirtioBlkReadBlocks() returns. In this sense, my earlier request to "*always* use AllocateBuffer + Map" was too strict. However, in the *general* case, the recommendation remains the same. For the virtio-net dri
Re: [edk2] [PATCH v3 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage
On Fri, Jul 28, 2017 at 10:18:49PM +0800, Jun Nie wrote:
> On 2017年07月28日 21:06, Leif Lindholm wrote:
> >On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
> >>On 2017年07月27日 22:09, Leif Lindholm wrote:
> >>>On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
> Add an android kernel loader that could load kernel from storage
> device. This patch is from Haojian's code as below link. The minor
> change is that alternative dtb is searched in second loader binary
> of Android bootimage if dtb is not found after Linux kernel.
> https://patches.linaro.org/patch/94683/
>
> This android boot image BDS add addtitional cmdline/dtb/ramfs
> support besides kernel that is introduced by Android boot header.
> >>>
> >>>Getting there. A few more comments below.
> >>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie
> ---
> ArmPkg/Include/Library/BdsLib.h| 3 +
> ArmPkg/Library/BdsLib/BdsFilePath.c| 3 -
> .../Application/AndroidBoot/AndroidBootApp.c | 127 +++
> .../Application/AndroidBoot/AndroidBootApp.inf | 64
> .../Application/AndroidFastboot/AndroidBootImg.c | 35 +-
> .../AndroidFastboot/AndroidFastbootApp.h | 1 +
> .../AndroidFastboot/Arm/BootAndroidBootImg.c | 2 +-
> EmbeddedPkg/Include/Library/AndroidBootImgLib.h| 67
> EmbeddedPkg/Include/Protocol/AndroidBootImg.h | 47 +++
> .../Library/AndroidBootImgLib/AndroidBootImgLib.c | 419
> +
> .../AndroidBootImgLib/AndroidBootImgLib.inf| 48 +++
> 11 files changed, 782 insertions(+), 34 deletions(-)
> create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> create mode 100644
> EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
> create mode 100644
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> create mode 100644
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
>
> >
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> new file mode 100644
> index 000..72c6322
> --- /dev/null
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -0,0 +1,419 @@
> +/** @file
> +
> + Copyright (c) 2013-2014, ARM Ltd. All rights reserved.
> + Copyright (c) 2017, Linaro. All rights reserved.
> +
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the
> BSD License
> + which accompanies this distribution. The full text of the license may
> be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +
> +**/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
> +
> +typedef struct {
> + MEMMAP_DEVICE_PATH Node1;
> + EFI_DEVICE_PATH_PROTOCOLEnd;
> +} MEMORY_DEVICE_PATH;
> +
> +STATIC ABOOTIMG_PROTOCOL *mAbootimg;
> >>>
> >>>mAndroidBootImg.
> >>
> >>Will do.
> >>>
> +
> +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
> >>>
> >>>Should also have an 'm'-prefix.
> >>
> >>Will do. But what 'm' prefix stand for?
> >>>
> +{
> + {
> +{
> + HARDWARE_DEVICE_PATH,
> + HW_MEMMAP_DP,
> + {
> +(UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
> +(UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
> + },
> +}, // Header
> +0, // StartingAddress (set at runtime)
> +0 // EndingAddress (set at runtime)
> + }, // Node1
> + {
> +END_DEVICE_PATH_TYPE,
> +END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +{ sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> + } // End
> +};
> +
> +EFI_STATUS
> +AbootimgGetImgSize (
> >>>
> >>>AndroidBootImgGetImageSize.
> >>
> >>Will do.
> >>>
> + IN VOID*BootImg,
> + OUT UINTN *ImgSize
> + )
> +{
> + ANDROID_BOOTIMG_HEADER *Header;
> +
> + Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> + if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> +return EFI_INVALID_PARAMETER;
> + }
> +
> +
Re: [edk2] [PATCH v3 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage
On 2017年07月28日 21:06, Leif Lindholm wrote:
On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
On 2017年07月27日 22:09, Leif Lindholm wrote:
On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
Add an android kernel loader that could load kernel from storage
device. This patch is from Haojian's code as below link. The minor
change is that alternative dtb is searched in second loader binary
of Android bootimage if dtb is not found after Linux kernel.
https://patches.linaro.org/patch/94683/
This android boot image BDS add addtitional cmdline/dtb/ramfs
support besides kernel that is introduced by Android boot header.
Getting there. A few more comments below.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie
---
ArmPkg/Include/Library/BdsLib.h| 3 +
ArmPkg/Library/BdsLib/BdsFilePath.c| 3 -
.../Application/AndroidBoot/AndroidBootApp.c | 127 +++
.../Application/AndroidBoot/AndroidBootApp.inf | 64
.../Application/AndroidFastboot/AndroidBootImg.c | 35 +-
.../AndroidFastboot/AndroidFastbootApp.h | 1 +
.../AndroidFastboot/Arm/BootAndroidBootImg.c | 2 +-
EmbeddedPkg/Include/Library/AndroidBootImgLib.h| 67
EmbeddedPkg/Include/Protocol/AndroidBootImg.h | 47 +++
.../Library/AndroidBootImgLib/AndroidBootImgLib.c | 419 +
.../AndroidBootImgLib/AndroidBootImgLib.inf| 48 +++
11 files changed, 782 insertions(+), 34 deletions(-)
create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
new file mode 100644
index 000..2de1d8a
--- /dev/null
+++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
@@ -0,0 +1,127 @@
+/** @file
+
+ Copyright (c) 2013-2014, ARM Ltd. All rights reserved.
+ Copyright (c) 2017, Linaro. All rights reserved.
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD
License
+ which accompanies this distribution. The full text of the license may be
found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+EFI_STATUS
+EFIAPI
+AndroidBootAppEntryPoint (
+ IN EFI_HANDLEImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+ CHAR16 *BootPathStr;
+ EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL *EfiDevicePathFromTextProtocol;
+ EFI_DEVICE_PATH *DevicePath;
+ EFI_DEVICE_PATH_PROTOCOL*Node, *NextNode;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;
+ UINT32 MediaId, BlockSize;
+ VOID*Buffer;
+ EFI_HANDLE Handle;
+ UINTN Size;
+
+ BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
+ ASSERT (BootPathStr != NULL);
+ Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
+(VOID **)&EfiDevicePathFromTextProtocol);
+ ASSERT_EFI_ERROR(Status);
+ DevicePath = (EFI_DEVICE_PATH
*)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
+ ASSERT (DevicePath != NULL);
+
+ /* Find DevicePath node of Partition */
+ NextNode = DevicePath;
+ while (1) {
Should this not be while (NextNode != NULL), with some check that the
node was found before progressing?
(NextNode != NULL) is valid check.
The code check node before progressing as below, doesn't it?
My point is that if you never match the "if (IS_DEVICE_PATH_NODE "
condition, this loop will never terminate.
And if we update the loop condition to fix that, we end up calling
LocateDevicePath with a known bad parameter.
Yeah, besides the check in while(), I should add check before while loop.
+Node = NextNode;
+if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
+ break;
+}
+NextNode = NextDevicePathNode (Node);
+ }
+
+ Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid,
+ &DevicePath, &Handle);
And should this not use &Node rather than &DevicePath?
I suppose we should return error if n
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 07/27/17 21:00, Brijesh Singh wrote: > On 07/27/2017 12:56 PM, Ard Biesheuvel wrote: >> On 27 July 2017 at 18:16, Laszlo Ersek wrote: >>> Normally, Map allocates the bounce buffer internally, and Unmap >>> releases the bounce buffer internally (for BusMasterRead and >>> BusMasterWrite), which is not right here. If you use >>> AllocateBuffer() separately, then Map() -- with >>> BusMasterCommonBuffer -- will not allocate internally, and Unmap() >>> will also not deallocate internally. So, in the ExitBootServices() >>> callback, you will be able to call Unmap() only, and then *not* call >>> FreeBuffer() at all. >>> >>> This is why I suggest introducing all four functions (Allocate, >>> Free, Map, Unmap) to the VIRTIO_DEVICE_PROTOCOL, and why all virtio >>> drivers should use all four functions explicitly, not just Map and >>> Unmap. >>> >>> ... Actually, I think there is a bug in >>> "OvmfPkg/IoMmuDxe/AmdSevIoMmu.c". You have the following >>> distribution of operations at the moment: >>> >>> - IoMmuAllocateBuffer() allocates pages and clears the memory >>> encryption mask >>> >>> - IoMmuFreeBuffer() re-sets the memory encryption mask, and >>> deallocates pages >>> >>> - IoMmuMap() does nothing at all when BusMasterCommonBuffer is >>> requested (and IoMmuAllocateBuffer() was called previously). >>> Otherwise, IoMmuMap() allocates pages, allocates MAP_INFO, and >>> clears the memory encryption mask. >>> >>> - IoMmuUnmap() does nothing when cleaning up a BusMasterCommonBuffer >>> operation (--> NO_MAPPING). Otherwise, IoMmuUnmap() clears the >>> encryption mask, and frees both the pages and MAP_INFO. >>> > > 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? > >>> This distribution of operations seems wrong. The key point is that >>> AllocateBuffer() *need not* result in a buffer that is immediately >>> usable, and that client code is required to call Map() >>> *unconditionally*, even if BusMasterCommonBuffer is the desired >>> operation. Therefore, the right distribution of operations is: >>> >>> - IoMmuAllocateBuffer() allocates pages and does not touch the >>> encryption mask.. >>> >>> - IoMmuFreeBuffer() deallocates pages and does not touch the >>> encryption mask. >>> > > 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 addr
Re: [edk2] [PATCH v3 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage
On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
> On 2017年07月27日 22:09, Leif Lindholm wrote:
> >On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
> >>Add an android kernel loader that could load kernel from storage
> >>device. This patch is from Haojian's code as below link. The minor
> >>change is that alternative dtb is searched in second loader binary
> >>of Android bootimage if dtb is not found after Linux kernel.
> >>https://patches.linaro.org/patch/94683/
> >>
> >>This android boot image BDS add addtitional cmdline/dtb/ramfs
> >>support besides kernel that is introduced by Android boot header.
> >
> >Getting there. A few more comments below.
> >
> >>Contributed-under: TianoCore Contribution Agreement 1.0
> >>Signed-off-by: Jun Nie
> >>---
> >> ArmPkg/Include/Library/BdsLib.h| 3 +
> >> ArmPkg/Library/BdsLib/BdsFilePath.c| 3 -
> >> .../Application/AndroidBoot/AndroidBootApp.c | 127 +++
> >> .../Application/AndroidBoot/AndroidBootApp.inf | 64
> >> .../Application/AndroidFastboot/AndroidBootImg.c | 35 +-
> >> .../AndroidFastboot/AndroidFastbootApp.h | 1 +
> >> .../AndroidFastboot/Arm/BootAndroidBootImg.c | 2 +-
> >> EmbeddedPkg/Include/Library/AndroidBootImgLib.h| 67
> >> EmbeddedPkg/Include/Protocol/AndroidBootImg.h | 47 +++
> >> .../Library/AndroidBootImgLib/AndroidBootImgLib.c | 419
> >> +
> >> .../AndroidBootImgLib/AndroidBootImgLib.inf| 48 +++
> >> 11 files changed, 782 insertions(+), 34 deletions(-)
> >> create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >> create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> >> create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> >> create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
> >> create mode 100644
> >> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >> create mode 100644
> >> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> >>
> >>diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>new file mode 100644
> >>index 000..2de1d8a
> >>--- /dev/null
> >>+++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>@@ -0,0 +1,127 @@
> >>+/** @file
> >>+
> >>+ Copyright (c) 2013-2014, ARM Ltd. All rights reserved.
> >>+ Copyright (c) 2017, Linaro. All rights reserved.
> >>+
> >>+ This program and the accompanying materials
> >>+ are licensed and made available under the terms and conditions of the
> >>BSD License
> >>+ which accompanies this distribution. The full text of the license may
> >>be found at
> >>+ http://opensource.org/licenses/bsd-license.php
> >>+
> >>+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >>+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> >>IMPLIED.
> >>+
> >>+**/
> >>+
> >>+#include
> >>+#include
> >>+#include
> >>+#include
> >>+#include
> >>+#include
> >>+#include
> >>+
> >>+#include
> >>+#include
> >>+
> >>+EFI_STATUS
> >>+EFIAPI
> >>+AndroidBootAppEntryPoint (
> >>+ IN EFI_HANDLEImageHandle,
> >>+ IN EFI_SYSTEM_TABLE *SystemTable
> >>+ )
> >>+{
> >>+ EFI_STATUS Status;
> >>+ CHAR16 *BootPathStr;
> >>+ EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL *EfiDevicePathFromTextProtocol;
> >>+ EFI_DEVICE_PATH *DevicePath;
> >>+ EFI_DEVICE_PATH_PROTOCOL*Node, *NextNode;
> >>+ EFI_BLOCK_IO_PROTOCOL *BlockIo;
> >>+ UINT32 MediaId, BlockSize;
> >>+ VOID*Buffer;
> >>+ EFI_HANDLE Handle;
> >>+ UINTN Size;
> >>+
> >>+ BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
> >>+ ASSERT (BootPathStr != NULL);
> >>+ Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
> >>+(VOID **)&EfiDevicePathFromTextProtocol);
> >>+ ASSERT_EFI_ERROR(Status);
> >>+ DevicePath = (EFI_DEVICE_PATH
> >>*)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
> >>+ ASSERT (DevicePath != NULL);
> >>+
> >>+ /* Find DevicePath node of Partition */
> >>+ NextNode = DevicePath;
> >>+ while (1) {
> >
> >Should this not be while (NextNode != NULL), with some check that the
> >node was found before progressing?
>
> (NextNode != NULL) is valid check.
> The code check node before progressing as below, doesn't it?
My point is that if you never match the "if (IS_DEVICE_PATH_NODE "
condition, this loop will never terminate.
And if we update the loop condition to fix that, we end up calling
LocateDevicePath with a known bad parameter.
> >
> >>+Node = NextNode;
> >>+if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, ME
Re: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top]
Laszlo, Thanks for raising this questions. Please see my embedded reply in below Thanks/Ray > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 27, 2017 7:23 PM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Marcel Apfelbaum ; Aleksandr Bezzubikov > ; Zeng, Star > Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid > hang when BUS pad resource is not in top] > > Hello Ray, > > please excuse me for hijacking this thread a little bit, but I'm very happy > that > you happen to be looking at this exact part of the code, which is also what > I've been discussing with Marcel and Aleksandr (CC'd) due to a different > reason. > > (The relevant sub-thread is at > > Allow RedHat PCI bridges reserve more buses than necessary during init > > http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f- > 2ceb6180f...@redhat.com > > but it's very long, so I'm not asking you to read that -- instead, please let > me > ask you our question independently and in a self-contained > form.) > > In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource > paddings, from the > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() > function. > > QEMU has no "root" hotplug controllers, only "non-root" hotplug controllers > (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protocol" in > the PI-1.6 spec). Therefore OVMF's > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty > array. > The BUS resource padding that we'd like to return from > GetResourcePadding() would be for "non-root" hotplug controllers. > > According to the PI spec, this is a valid thing to do: > > * From "12.5 Sample Implementation for a Platform Containing PCI > Hot Plug* Slots": > > > [...] For all the root HPCs and the nonroot HPCs, call > > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() to obtain the > > amount of overallocation and add that amount to the requests from the > > physical devices. Reprogram the bus numbers by taking into account the > > bus resource padding information. [...] > > * From "12.6 PCI Hot Plug PCI Initialization Protocol, > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": > > > [...] This member function is called for all the root HPCs and nonroot > > HPCs that are detected by the PCI bus enumerator. [...] The PCI bus > > driver is responsible for adding this resource request to the resource > > requests by the physical PCI devices. [...] > > I searched PciBusDxe for GetResourcePadding() call sites, and there are > two: > > (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function > GetResourcePaddingForHpb(). This call site seems to work as follows, > for our purposes: > > GetResourcePaddingPpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > GetResourcePaddingForHpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c] > gPciHotPlugInit->GetResourcePadding() > // > // and then save the resource descriptors, including the bus > // padding, in PciIoDevice->ResourcePaddingDescriptors > // > > ResourcePaddingPolicy() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > ApplyResourcePadding() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > // > // translate IO and MMIO resources from > // PciDev->ResourcePaddingDescriptors to internal padding > // representation > // > > This call site and processing seem to be active for "non-root" > hotplug controllers, but they do not consider bus numbers, only IO > and MMIO. [Ray] It's in bus allocation phase. So the PciBus driver only cares about the bus padding. PciBus driver firstly allocates the bus resource and programs the PCI-PCI bridges to set the bus numbers. After that, PciBus allocates the IO/MMIO resources and program the BARs in PCI devices & PCI-PCI bridges. So it's good here. > > (2) In the PciScanBus() function, file > "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another > GetResourcePadding() call -- your present patch is updating this > code part. > > From the resource padding list returned by this GetResourcePadding() > invocation, PciBusDxe cares only about the bus numbers. I think > that's probably fine. However, in this location, > GetResourcePadding() is called *only* for "root" hotplug > controllers, of which QEMU has none. > > The end result is that bus number paddings are not considered at all for > "non-root" hotplug controllers: [Ray] Bus number padding should be considered for "non-root" HPCs. So I agree it's a bug/gap I need to fix. I noticed that already before your question. And I do have a plan to fix it though the priority is not very high. > - site (1) is active for "non-root" HPCs, but it ignores bus number > paddings, > - site (2) processes bus number paddings, but it is inactive for >
Re: [edk2] [PATCH v3 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage
On 2017年07月27日 22:09, Leif Lindholm wrote:
On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
Add an android kernel loader that could load kernel from storage
device. This patch is from Haojian's code as below link. The minor
change is that alternative dtb is searched in second loader binary
of Android bootimage if dtb is not found after Linux kernel.
https://patches.linaro.org/patch/94683/
This android boot image BDS add addtitional cmdline/dtb/ramfs
support besides kernel that is introduced by Android boot header.
Getting there. A few more comments below.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie
---
ArmPkg/Include/Library/BdsLib.h| 3 +
ArmPkg/Library/BdsLib/BdsFilePath.c| 3 -
.../Application/AndroidBoot/AndroidBootApp.c | 127 +++
.../Application/AndroidBoot/AndroidBootApp.inf | 64
.../Application/AndroidFastboot/AndroidBootImg.c | 35 +-
.../AndroidFastboot/AndroidFastbootApp.h | 1 +
.../AndroidFastboot/Arm/BootAndroidBootImg.c | 2 +-
EmbeddedPkg/Include/Library/AndroidBootImgLib.h| 67
EmbeddedPkg/Include/Protocol/AndroidBootImg.h | 47 +++
.../Library/AndroidBootImgLib/AndroidBootImgLib.c | 419 +
.../AndroidBootImgLib/AndroidBootImgLib.inf| 48 +++
11 files changed, 782 insertions(+), 34 deletions(-)
create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
diff --git a/ArmPkg/Include/Library/BdsLib.h b/ArmPkg/Include/Library/BdsLib.h
index c58f47e..4528c2e 100644
--- a/ArmPkg/Include/Library/BdsLib.h
+++ b/ArmPkg/Include/Library/BdsLib.h
@@ -15,6 +15,9 @@
#ifndef __BDS_ENTRY_H__
#define __BDS_ENTRY_H__
+#define IS_DEVICE_PATH_NODE(node,type,subtype)\
+(((node)->Type == (type)) && ((node)->SubType == (subtype)))
+
/**
This is defined by the UEFI specs, don't change it
**/
diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c
b/ArmPkg/Library/BdsLib/BdsFilePath.c
index f9d8c4c..41557bb 100644
--- a/ArmPkg/Library/BdsLib/BdsFilePath.c
+++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
@@ -24,9 +24,6 @@
#include
#include
-
-#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) &&
((node)->SubType == (subtype)))
-
Could you break these bits of moving macros and definitions into
common header files as a separate patch, preceding the rest of the changes?
Will do.
/* Type and defines to set up the DHCP4 options */
typedef struct {
diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
new file mode 100644
index 000..2de1d8a
--- /dev/null
+++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
@@ -0,0 +1,127 @@
+/** @file
+
+ Copyright (c) 2013-2014, ARM Ltd. All rights reserved.
+ Copyright (c) 2017, Linaro. All rights reserved.
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD
License
+ which accompanies this distribution. The full text of the license may be
found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+EFI_STATUS
+EFIAPI
+AndroidBootAppEntryPoint (
+ IN EFI_HANDLEImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+ CHAR16 *BootPathStr;
+ EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL *EfiDevicePathFromTextProtocol;
+ EFI_DEVICE_PATH *DevicePath;
+ EFI_DEVICE_PATH_PROTOCOL*Node, *NextNode;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;
+ UINT32 MediaId, BlockSize;
+ VOID*Buffer;
+ EFI_HANDLE Handle;
+ UINTN Size;
+
+ BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
+ ASSERT (BootPathStr != NULL);
+ Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
+(VOID **)&EfiDevicePathFromTextProtocol);
+ ASSERT_EFI_ERROR(Status);
+ DevicePath = (EFI_DEVICE_PATH
*)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
+ ASSERT (DevicePath != NULL);
+
+ /* Find DevicePath node of Partition */
[edk2] [PATCH 2/2] MdeModulePkg PeiCore: Handle notification PPI from SEC
InstallPpi() will be used for normal PPI in PPI list from SEC,
and NotifyPpi() will be used for notification PPI in PPI list from SEC.
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng
---
MdeModulePkg/Core/Pei/PeiMain.h | 14
MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 7 +-
MdeModulePkg/Core/Pei/Ppi/Ppi.c | 140
3 files changed, 142 insertions(+), 19 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index 8b58916e65b3..e95b1c3d8cd7 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -557,6 +557,20 @@ DispatchNotify (
IN INTNNotifyStopIndex
);
+/**
+ Process PpiList from SEC phase.
+
+ @param PeiServicesAn indirect pointer to the EFI_PEI_SERVICES table
published by the PEI Foundation.
+ @param PpiListPoints to a list of one or more PPI descriptors to be
installed initially by the PEI core.
+These PPI's will be installed and/or immediately
signaled if they are notification type.
+
+**/
+VOID
+ProcessPpiListFromSec (
+ IN CONST EFI_PEI_SERVICES **PeiServices,
+ IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList
+ );
+
//
// Boot mode support functions
//
diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
index 27484bafc575..556c40a3035c 100644
--- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
+++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
@@ -1,7 +1,7 @@
/** @file
Pei Core Main Entry Point
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -380,11 +380,10 @@ PeiCore (
);
//
-// If SEC provided any PPI services to PEI, install them.
+// If SEC provided the PpiList, process it.
//
if (PpiList != NULL) {
- Status = PeiServicesInstallPpi (PpiList);
- ASSERT_EFI_ERROR (Status);
+ ProcessPpiListFromSec (&PrivateData.Ps, PpiList);
}
} else {
//
diff --git a/MdeModulePkg/Core/Pei/Ppi/Ppi.c b/MdeModulePkg/Core/Pei/Ppi/Ppi.c
index db6eded6d6ed..b2ab3fbd2086 100644
--- a/MdeModulePkg/Core/Pei/Ppi/Ppi.c
+++ b/MdeModulePkg/Core/Pei/Ppi/Ppi.c
@@ -1,7 +1,7 @@
/** @file
EFI PEI Core PPI services
-Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -171,6 +171,9 @@ ConvertPpiPointers (
@param PeiServicesAn indirect pointer to the
EFI_PEI_SERVICES table published by the PEI Foundation.
@param PpiListPointer to a list of PEI PPI Descriptors.
+ @param Single TRUE if only single entry in the PpiList.
+FALSE if the PpiList is ended with an
entry which has the
+EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST flag
set in its Flags field.
@retval EFI_SUCCESS if all PPIs in PpiList are successfully
installed.
@retval EFI_INVALID_PARAMETERif PpiList is NULL pointer
@@ -179,10 +182,10 @@ ConvertPpiPointers (
**/
EFI_STATUS
-EFIAPI
-PeiInstallPpi (
+InternalPeiInstallPpi (
IN CONST EFI_PEI_SERVICES**PeiServices,
- IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList
+ IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList,
+ IN BOOLEAN Single
)
{
PEI_CORE_INSTANCE *PrivateData;
@@ -229,11 +232,16 @@ PeiInstallPpi (
PrivateData->PpiData.PpiListPtrs[Index].Ppi = (EFI_PEI_PPI_DESCRIPTOR*)
PpiList;
PrivateData->PpiData.PpiListEnd++;
-//
-// Continue until the end of the PPI List.
-//
-if ((PpiList->Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) ==
-EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) {
+if (Single) {
+ //
+ // Only single entry in the PpiList.
+ //
+ break;
+} else if ((PpiList->Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) ==
+ EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) {
+ //
+ // Continue until the end of the PPI List.
+ //
break;
}
PpiList++;
@@ -258,6 +266,31 @@ PeiInstallPpi (
/**
+ This function installs an interface in the PEI PPI database by GUID.
+ The purpose of the service is to publish an interface that other parties
+ can use to call additional PEIMs.
+
+ @param PeiServicesAn indirect pointer to the
EFI_PEI_SERVICES table published by the PEI Foundation.
+
[edk2] [PATCH 0/2] Handle notification PPI from SEC
This patch series is to follow latest (>= 1.5) PI spec to handle notification PPI from SEC. Star Zeng (2): MdePkg PiPeiCis.h: Add description for notification PPI from SEC MdeModulePkg PeiCore: Handle notification PPI from SEC MdeModulePkg/Core/Pei/PeiMain.h | 14 MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 7 +- MdeModulePkg/Core/Pei/Ppi/Ppi.c | 140 MdePkg/Include/Pi/PiPeiCis.h| 17 ++-- 4 files changed, 151 insertions(+), 27 deletions(-) -- 2.7.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ShellPkg: Avoid buffer out-of-bound access
Reviewed-by: Steven Shi
Thanks
Steven Shi
> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, July 26, 2017 4:22 PM
> To: edk2-devel@lists.01.org
> Cc: Shi, Steven
> Subject: [PATCH] ShellPkg: Avoid buffer out-of-bound access
>
> PathSize is the number of bytes in PathForReturn buffer so
> PathForReturn[PathSize - 1] incorrectly accesses the last
> character in the buffer,
> PathForReturn[PathSize / sizeof (CHAR16) - 1] should be used.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni
> Cc: Steven Shi
> ---
> ShellPkg/Application/Shell/ShellProtocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index b3b8acc0d0..991fb58ca7 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -477,7 +477,7 @@ EfiShellGetFilePathFromDevicePath(
> // UEFI Shell spec section 3.7)
> if ((PathSize != 0)&&
> (PathForReturn != NULL)&&
> -(PathForReturn[PathSize - 1] != L'\\') &&
> +(PathForReturn[PathSize / sizeof (CHAR16) - 1] != L'\\') &&
> (AlignedNode->PathName[0]!= L'\\')) {
>PathForReturn = StrnCatGrow (&PathForReturn, &PathSize, L"\\", 1);
> }
> --
> 2.12.2.windows.2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 1/2] MdePkg PiPeiCis.h: Add description for notification PPI from SEC
This patch is to follow latest (>= 1.5) PI spec to add description
for notification PPI from SEC
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng
---
MdePkg/Include/Pi/PiPeiCis.h | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/MdePkg/Include/Pi/PiPeiCis.h b/MdePkg/Include/Pi/PiPeiCis.h
index 92d6f0641ede..eda814a8701a 100644
--- a/MdePkg/Include/Pi/PiPeiCis.h
+++ b/MdePkg/Include/Pi/PiPeiCis.h
@@ -230,7 +230,7 @@ EFI_STATUS
@retval EFI_SUCCESS The interface was successfully installed.
@retval EFI_INVALID_PARAMETER The PpiList pointer is NULL, or any of the PEI
PPI descriptors in the
-list do not have the
EFI_PEI_PPI_DESCRIPTOR_PPI bit set in the Flags field.
+list do not have the
EFI_PEI_PPI_DESCRIPTOR_NOTIFY_TYPES bit set in the Flags field.
@retval EFI_OUT_OF_RESOURCES There is no additional space in the PPI
database.
**/
@@ -1004,13 +1004,14 @@ typedef struct _EFI_SEC_PEI_HAND_OFF {
allows the SEC phase to pass information about the stack,
temporary RAM and the Boot Firmware Volume. In addition, it also
allows the SEC phase to pass services and data forward for use
- during the PEI phase in the form of one or more PPIs. There is
- no limit to the number of additional PPIs that can be passed
- from SEC into the PEI Foundation. As part of its initialization
- phase, the PEI Foundation will add these SEC-hosted PPIs to its
- PPI database such that both the PEI Foundation and any modules
- can leverage the associated service calls and/or code in these
- early PPIs.
+ during the PEI phase in the form of one or more PPIs. These PPI's
+ will be installed and/or immediately signaled if they are
+ notification type. There is no limit to the number of additional
+ PPIs that can be passed from SEC into the PEI Foundation. As part
+ of its initialization phase, the PEI Foundation will add these
+ SEC-hosted PPIs to its PPI database such that both the PEI
+ Foundation and any modules can leverage the associated service
+ calls and/or code in these early PPIs.
@param SecCoreDataPoints to a data structure containing
information about the PEI core's
--
2.7.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
On 27 July 2017 at 23:10, Brijesh Singh wrote: > > > On 07/27/2017 04:31 PM, Ard Biesheuvel wrote: >> >> >>> On 27 Jul 2017, at 21:55, Brijesh Singh wrote: >>> >>> >>> >>> On 07/27/2017 02:00 PM, Brijesh Singh wrote: >>> >> This distribution of operations seems wrong. The key point is that >> AllocateBuffer() *need not* result in a buffer that is immediately >> usable, and that client code is required to call Map() >> *unconditionally*, even if BusMasterCommonBuffer is the desired >> operation. Therefore, the right distribution of operations is: >> >> - IoMmuAllocateBuffer() allocates pages and does not touch the >>encryption mask.. >> >> - IoMmuFreeBuffer() deallocates pages and does not touch the >> encryption >>mask. >> 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. [1] https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L159 [2] https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L197 >> >> - IoMmuMap() does not allocate pages when BusMasterCommonBuffer is >>requested, and it allocates pages (bounce buffer) otherwise. >> I am trying to wrap my head around how we can support BusMasterCommonBuffer when buffer was not allocated by us. Changing the memory encryption mask in a page table will not update the contents. Also since the memory encryption mask works on PAGE_SIZE hence changing the encryption mask on not our allocated buffer could mess things up (e.g if NumberOfBytes is not PAGE_SIZE aligned). >>> >>> >>> I may be missing something in my understanding. Here is a flow I have in >>> my >>> mind, please correct me. >>> >>> OvmfPkg/VirtIoBlk.c: >>> >>> VirtioBlkInit() >>> >>> >>> VirtioRingInit >>> Virtio->AllocateSharedPages(RingSize, &Ring->Base) >>> PciIo->AllocatePages(RingSize, &RingAddress) >>> Virtio->MapSharedPages(...,BusMasterCommonBuffer, Ring->Base, >>> RingSize, &RingDeviceAddress) >>> . >>> . >>> >>> This case is straight forward and we can easily maps. No need for bounce >>> buffering. >>> >>> VirtioBlkReadBlocks(..., BufferSize, Buffer,) >>> .. >>> .. >>> SynchronousRequest(..., BufferSize, Buffer) >>> >>> Virtio->MapSharedPages(..., BusMasterCommonBuffer, Buffer, >>> BufferSize, &DeviceAddress) >>> VirtioAppendDesc(DeviceAddress, BufferSize, ...) >>> VirtioFlush (...) >>> In the above case, "Buffer" was not allocated by us hence we will not >>> able to change the >>> memory encryption attributes. Am I missing something in the flow ? >>> >> >> >> Common buffer mappings may only be created from buffers that were >> allocated by AllocateBuffer(). In fact, that is its main purpose > > > Yes, that part is well understood. If the buffer was allocated by us (e.g > vring, request/status > structure etc) then those should be mapped as "BusMasterCommonBuffer". > > But I am trying to figure out, how to map a data buffers before issuing a > virtio request. e.g when > VirtioBlkReadBlocks() is called, "Buffer" pointer is not a DMA address hence > we need to map it. > I think it should be mapped using "BusMasterWrite" not > "BusMasterCommonBuffer" before adding into vring. > If the transfer is strictly unidirectional, then that should work. If the transfer goes both ways, you may need to map/unmap for read and then map/unmap for write ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
