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>, edk2-devel@lists.01.org
Cc: Jordan Justen <jordan.l.justen@intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers
Date: Sun, 27 Aug 2017 22:40:26 +0200	[thread overview]
Message-ID: <61e3119c-0e51-ea05-96e6-55c4908dadd4@redhat.com> (raw)
In-Reply-To: <1503697414-6830-3-git-send-email-brijesh.singh@amd.com>

This patch is functionally correct. I'd like to request three stylistic
improvements:

On 08/25/17 23:43, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of virtio request, response and any memory referenced by those
> request/response to the bus master.
> 
> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to
> map request and response buffers system physical address to the device
> address.
> 
> - If the buffer need to be accessed by both the processor and a bus
>   master then map with BusMasterCommonBuffer.
> 
> - If the buffer need to be accessed for a write operation by a bus master
>   then map with BusMasterWrite.
> 
> - If the buffer need to be accessed for a read  operation by a bus master
>   then map with BusMasterRead.
> 
> 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/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++--
>  1 file changed, 143 insertions(+), 14 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 663ba281ab73..ab69cb08a625 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -232,7 +232,8 @@ VerifyReadWriteRequest (
>  
>    @retval EFI_DEVICE_ERROR     Failed to notify host side via VirtIo write, or
>                                 unable to parse host response, or host response
> -                               is not VIRTIO_BLK_S_OK.
> +                               is not VIRTIO_BLK_S_OK or failed to map Buffer
> +                               for a bus master operation.
>  
>  **/
>  
> @@ -249,8 +250,16 @@ SynchronousRequest (
>  {
>    UINT32                  BlockSize;
>    volatile VIRTIO_BLK_REQ Request;
> -  volatile UINT8          HostStatus;
> +  volatile UINT8          *HostStatus;
> +  VOID                    *HostStatusBuffer;
>    DESC_INDICES            Indices;
> +  VOID                    *RequestMapping;
> +  VOID                    *StatusMapping;
> +  VOID                    *BufferMapping;
> +  EFI_PHYSICAL_ADDRESS    BufferDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
> +  EFI_STATUS              Status, Ret;

(1) Please rename the "Ret" variable to "UnmapStatus", and define it
separately.

>  
>    BlockSize = Dev->BlockIoMedia.BlockSize;
>  
> @@ -278,9 +287,89 @@ SynchronousRequest (
>    VirtioPrepare (&Dev->Ring, &Indices);
>  
>    //
> +  // Host status is bi-directional (we preset with a value and expect the
> +  // device to update it). Allocate a host status buffer which can be mapped
> +  // to access equally by both processor and the device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *HostStatus),
> +                          &HostStatusBuffer
> +                          );
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  //
> +  // Map virtio-blk request header (must be done after request header is
> +  // populated)
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto FreeHostStatusBuffer;
> +  }
> +
> +  //
> +  // Map data buffer
> +  //
> +  if (BufferSize > 0) {
> +    if (RequestIsWrite) {
> +      Status = VirtioMapAllBytesInSharedBuffer (
> +                 Dev->VirtIo,
> +                 VirtioOperationBusMasterRead,
> +                 (VOID *) Buffer,
> +                 BufferSize,
> +                 &BufferDeviceAddress,
> +                 &BufferMapping
> +                 );
> +    } else {
> +      Status = VirtioMapAllBytesInSharedBuffer (
> +                 Dev->VirtIo,
> +                 VirtioOperationBusMasterWrite,
> +                 (VOID *) Buffer,
> +                 BufferSize,
> +                 &BufferDeviceAddress,
> +                 &BufferMapping
> +                 );
> +    }

(2) Please squash these two branches into one, and determine the
Operation argument like this:

  (RequestIsWrite ?
   VirtioOperationBusMasterRead :
   VirtioOperationBusMasterWrite),

(The conditional operator (? :) should be used sparingly, but when it
improves readability, it should be used.)

> +
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapRequestBuffer;
> +    }
> +  }
> +
> +  //
> +  // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that
> +  // both processor and device can access it.
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             HostStatusBuffer,
> +             sizeof *HostStatus,
> +             &HostStatusDeviceAddress,
> +             &StatusMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapDataBuffer;
> +  }
> +
> +  HostStatus = HostStatusBuffer;
> +
> +  //
>    // preset a host status for ourselves that we do not accept as success
>    //
> -  HostStatus = VIRTIO_BLK_S_IOERR;
> +  *HostStatus = VIRTIO_BLK_S_IOERR;
>  
>    //
>    // ensured by VirtioBlkInit() -- this predicate, in combination with the
> @@ -291,8 +380,13 @@ SynchronousRequest (
>    //
>    // virtio-blk header in first desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> -    VRING_DESC_F_NEXT, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    RequestDeviceAddress,
> +    sizeof Request,
> +    VRING_DESC_F_NEXT,
> +    &Indices
> +    );
>  
>    //
>    // data buffer for read/write in second desc
> @@ -311,27 +405,62 @@ SynchronousRequest (
>      //
>      // VRING_DESC_F_WRITE is interpreted from the host's point of view.
>      //
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      BufferDeviceAddress,
> +      (UINT32) BufferSize,
>        VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
> -      &Indices);
> +      &Indices
> +      );
>    }
>  
>    //
>    // host status in last (second or third) desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
> -    VRING_DESC_F_WRITE, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    HostStatusDeviceAddress,
> +    sizeof *HostStatus,
> +    VRING_DESC_F_WRITE,
> +    &Indices
> +    );
>  
>    //
>    // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D).
>    //
> -  if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices,
> -        NULL) == EFI_SUCCESS &&
> -      HostStatus == VIRTIO_BLK_S_OK) {
> -    return EFI_SUCCESS;
> +  Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL);
> +
> +  //
> +  // Unmap the HostStatus buffer before accessing it
> +  //
> +  Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping);

(3) Okay, so here you will assign UnmapStatus. And then, the rest:

> +  if (EFI_ERROR (Ret)) {
> +    Status = EFI_DEVICE_ERROR;
> +  }
> +
> +  if (!EFI_ERROR (Status) &&
> +      *HostStatus == VIRTIO_BLK_S_OK) {
> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = EFI_DEVICE_ERROR;
>    }

should be written like this:

  if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) ||
      *HostStatus != VIRTIO_BLK_S_OK) {
    Status = EFI_DEVICE_ERROR;
  }

Namely,

- this block will ensure that we only look at *HostStatus when we're
allowed to (i.e., after both VirtioFlush() and Unmap succeeded),

- If VirtioFlush() fails and sets Status to some error code, then we
coerce Status to EFI_DEVICE_ERROR,

- If the entire condition evaluates to FALSE, then Status is already
EFI_SUCCESS, so no need to touch it.


Regarding the VirtioScsiDxe driver... in this patch, we get away with
the above shortcut (without making a mess of the code), but in the
VirtioScsiDxe driver, we won't. In that driver, the bus master device
can produce *two* output buffers, so you will have to unmap at least one
of those under an error-handling label -- when you mapped the first
successfully, and failed to map the second.

(In this patch you managed to unmap StatusMapping in shared code, but
that only worked because you had only one output buffer, and you could
put its mapping last.)

And once you unmap an output buffer on both the success path and the
failure path, things get more complex. I think that's OK, we should just
be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I
laid out in
<http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>.

Thank you,
Laszlo

>  
> -  return EFI_DEVICE_ERROR;
> +UnmapDataBuffer:
> +  if (BufferSize > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +FreeHostStatusBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *HostStatus),
> +                 HostStatusBuffer
> +                 );
> +
> +  return Status;
>  }
>  
>  
> 



  reply	other threads:[~2017-08-27 20:37 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-25 21:43 [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address Brijesh Singh
2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh
2017-08-27 18:58   ` Laszlo Ersek
2017-08-25 21:43 ` [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers Brijesh Singh
2017-08-27 20:40   ` Laszlo Ersek [this message]
2017-08-27 22:05     ` Laszlo Ersek
2017-08-27 23:18       ` Brijesh Singh
2017-08-25 21:43 ` [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
2017-08-27 19:01   ` 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=61e3119c-0e51-ea05-96e6-55c4908dadd4@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