On 08/14/17 13:36, Brijesh Singh wrote:
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
(1) Where did the commit message go? :) It was pretty good. Here's my
suggestion for reinstating it:
The VRING buffer is a communication area between guest and
hypervisor. Allocate it using
VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() so that it can be
mapped later with VirtioRingMap() for bi-directional access.
(This is a VirtioLib patch so we can refer to VirtioRingMap() -- after
the reordering that I requested under [PATCH v2 12/23], point (2), the
VirtioRingMap() function will be introduced just before this patch.)
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 7d07dcc09d3d..8010b5cf4735 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -61,6 +61,7 @@ VirtioRingInit (
> OUT VRING *Ring
> )
> {
> + EFI_STATUS Status;
> UINTN RingSize;
> volatile UINT8 *RingPagesPtr;
>
> @@ -79,9 +80,16 @@ VirtioRingInit (
> sizeof *Ring->Used.AvailEvent,
> EFI_PAGE_SIZE);
>
> + //
> + // Allocate a shared ring buffer
> + //
> Ring->NumPages = EFI_SIZE_TO_PAGES (RingSize);
> - Ring->Base = AllocatePages (Ring->NumPages);
> - if (Ring->Base == NULL) {
> + Status = VirtIo->AllocateSharedPages (
> + VirtIo,
> + Ring->NumPages,
> + &Ring->Base
> + );
> + if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
> SetMem (Ring->Base, RingSize, 0x00);
> @@ -143,7 +151,7 @@ VirtioRingUninit (
> IN OUT VRING *Ring
> )
> {
> - FreePages (Ring->Base, Ring->NumPages);
> + VirtIo->FreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
> SetMem (Ring, sizeof *Ring, 0x00);
> }
>
>
You seem to have missed my points (2), (3) and (4) from my previous
review
<http://mid.mail-archive.com/26a6be37-9983-7a34-afbf-d25e5501b138@redhat.com>:
> (2) Please update the documentation of "@retval EFI_OUT_OF_RESOURCES"
> -- replace it with
>
> @return Status codes propagated from VirtIo->AllocateSharedPages().
>
> (3) Accordingly, please forward the Status received, on the error
> branch.
>
> (4) Please remove MemoryAllocationLib from the #includes and also from
> [LibraryClasses].
(5) In addition, the indentation of the AllocateSharedPages() arguments
is incorrect.
To be continued.
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel