Re: [PATCH RFC 18/18] OvmfPkg/XenPvBlkDxe: Add BlockIo.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jul 16, 2014 at 04:15:47PM +0100, Anthony PERARD wrote:
> Install the BlockIo protocol.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenPvBlkDxe/BlockIo.c       | 256 ++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenPvBlkDxe/BlockIo.h       | 109 +++++++++++++++
>  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c   |  59 +++++++++
>  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h   |   1 +
>  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf |   2 +
>  5 files changed, 427 insertions(+)
>  create mode 100644 OvmfPkg/XenPvBlkDxe/BlockIo.c
>  create mode 100644 OvmfPkg/XenPvBlkDxe/BlockIo.h
> 
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockIo.c b/OvmfPkg/XenPvBlkDxe/BlockIo.c
> new file mode 100644
> index 0000000..4ac2411
> --- /dev/null
> +++ b/OvmfPkg/XenPvBlkDxe/BlockIo.c
> @@ -0,0 +1,256 @@
> +
> +/** @file
> +  TODO: Brief Description of UEFI Driver XenPvBlkDxe
> +
> +  TODO: Detailed Description of UEFI Driver XenPvBlkDxe
> +
> +  TODO: Copyright for UEFI Driver XenPvBlkDxe
> +
> +  TODO: License for UEFI Driver XenPvBlkDxe
> +
> +**/
> +
> +#include "XenPvBlkDxe.h"
> +
> +#include "BlockFront.h"
> +
> +///
> +/// Block I/O Media structure
> +///
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +EFI_BLOCK_IO_MEDIA  gXenPvBlkDxeBlockIoMedia = {
> +  0,      // MediaId
> +  FALSE,  // RemovableMedia
> +  FALSE,  // MediaPresent
> +  FALSE,  // LogicalPartition
> +  TRUE,   // ReadOnly
> +  FALSE,  // WriteCaching
> +  512,    // BlockSize
> +  512,    // IoAlign
> +  0,      // LastBlock
> +  0,      // LowestAlignedLba
> +  0,      // LogicalBlocksPerPhysicalBlock
> +  0       // OptimalTransferLengthGranularity
> +};
> +
> +///
> +/// Block I/O Protocol instance
> +///
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +EFI_BLOCK_IO_PROTOCOL  gXenPvBlkDxeBlockIo = {
> +  EFI_BLOCK_IO_PROTOCOL_REVISION3,             // Revision
> +  &gXenPvBlkDxeBlockIoMedia,                // Media
> +  (EFI_BLOCK_RESET)XenPvBlkDxeBlockIoReset, // Reset
> +  XenPvBlkDxeBlockIoReadBlocks,             // ReadBlocks
> +  XenPvBlkDxeBlockIoWriteBlocks,            // WriteBlocks
> +  XenPvBlkDxeBlockIoFlushBlocks             // FlushBlocks
> +};
> +
> +
> +
> +
> +STATIC
> +EFI_STATUS
> +XenPvBlkDxeBlockIoReadWriteBlocks (
> +  IN     EFI_BLOCK_IO_PROTOCOL  *This,
> +  IN     UINT32                 MediaId,
> +  IN     EFI_LBA                Lba,
> +  IN     UINTN                  BufferSize,
> +  IN OUT VOID                   *Buffer,
> +  IN     BOOLEAN                IsWrite
> +  )
> +{
> +  XEN_BLOCK_FRONT_IO IoData;
> +  EFI_BLOCK_IO_MEDIA *Media;
> +  UINTN NextOffset;
> +
> +  if (Buffer == NULL) {
> +    DEBUG ((EFI_D_ERROR, "%a %d, buffer null\n", __func__, __LINE__));

Heheh.

> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (BufferSize == 0) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  Media = This->Media;
> +
> +  if (BufferSize % Media->BlockSize != 0) {
> +    DEBUG ((EFI_D_ERROR, "%a %d, bad buffer size\n", __func__, __LINE__));
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  if (Lba > Media->LastBlock ||
> +      (BufferSize / Media->BlockSize) - 1 > Media->LastBlock - Lba) {
> +    DEBUG ((EFI_D_ERROR, "%a %d, read too large\n", __func__, __LINE__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (IsWrite && Media->ReadOnly) {
> +    DEBUG ((EFI_D_ERROR, "%a %d, read only\n", __func__, __LINE__));
> +    return EFI_WRITE_PROTECTED;
> +  }
> +
> +  if ((Media->IoAlign > 1) && (UINTN)Buffer & (Media->IoAlign - 1)) {
> +    // This happen often when using grub2
> +    VOID *NewBuffer;
> +    EFI_STATUS Status;
> +
> +    /* Try again with a properly aligned buffer. */
> +    NewBuffer = AllocateAlignedPages((BufferSize + EFI_PAGE_SIZE) / EFI_PAGE_SIZE,
> +                                     Media->IoAlign);
> +    if (!IsWrite) {
> +      Status = XenPvBlkDxeBlockIoReadBlocks (This, MediaId,
> +                                             Lba, BufferSize, NewBuffer);
> +      CopyMem (Buffer, NewBuffer, BufferSize);
> +    } else {
> +      CopyMem (NewBuffer, Buffer, BufferSize);
> +      Status = XenPvBlkDxeBlockIoWriteBlocks (This, MediaId,
> +                                              Lba, BufferSize, NewBuffer);
> +    }
> +    FreeAlignedPages (NewBuffer, (BufferSize + EFI_PAGE_SIZE) / EFI_PAGE_SIZE);
> +    return Status;
> +  }
> +
> +
> +  // XXX Check MediaId
> +
> +  IoData.Dev = XEN_BLOCK_FRONT_FROM_BLOCK_IO (This);
> +  IoData.Offset = Lba * Media->BlockSize;
> +  IoData.Callback = NULL;
> +  IoData.Data = NULL;
> +  IoData.Size = BufferSize;
> +  IoData.Buffer = Buffer;
> +
> +  NextOffset = Lba * Media->BlockSize;

Or just = IoData.Offset ?

> +  while (BufferSize > 0) {
> +    // This should work if every Buffer is page aligned
> +    // but block accept sector aligned, so FIXME

Isn't the work-around for GRUB2 above doing this?

> +    IoData.Size = MIN(EFI_PAGE_SIZE * 8,//BLKIF_MAX_SEGMENTS_PER_REQUEST,
> +                           BufferSize);

Why 8 ?

> +    IoData.Buffer = Buffer;
> +    IoData.Offset = NextOffset;
> +    BufferSize -= IoData.Size;
> +    Buffer = (VOID*) ((UINTN) Buffer + IoData.Size);
> +    NextOffset += IoData.Size;
> +    XenPvBlockIo (&IoData, IsWrite);
> +    if (!IoData.Data) {

Well that wouldn't be possible as the XenPvBlockIo loops forever until that
is done.

However, you should probably have a comment here:

/* XXX If we move to a different mechanism for CallBack this might
   go away */

> +      DEBUG ((EFI_D_ERROR, "%a %d, XenPvBlockIo no data...\n", __func__, __LINE__));
> +      return EFI_DEVICE_ERROR;
> +    }
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Read BufferSize bytes from Lba into Buffer.
> +
> +  @param  This       Indicates a pointer to the calling context.
> +  @param  MediaId    Id of the media, changes every time the media is replaced.
> +  @param  Lba        The starting Logical Block Address to read from
> +  @param  BufferSize Size of Buffer, must be a multiple of device block size.
> +  @param  Buffer     A pointer to the destination buffer for the data. The caller is
> +                     responsible for either having implicit or explicit ownership of the buffer.
> +
> +  @retval EFI_SUCCESS           The data was read correctly from the device.
> +  @retval EFI_DEVICE_ERROR      The device reported an error while performing the read.
> +  @retval EFI_NO_MEDIA          There is no media in the device.
> +  @retval EFI_MEDIA_CHANGED     The MediaId does not matched the current device.
> +  @retval EFI_BAD_BUFFER_SIZE   The Buffer was not a multiple of the block size of the device.
> +  @retval EFI_INVALID_PARAMETER The read request contains LBAs that are not valid,
> +                                or the buffer is not on proper alignment.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoReadBlocks (
> +  IN  EFI_BLOCK_IO_PROTOCOL         *This,
> +  IN  UINT32                        MediaId,
> +  IN  EFI_LBA                       Lba,
> +  IN  UINTN                         BufferSize,
> +  OUT VOID                          *Buffer
> +  )
> +{
> +  return XenPvBlkDxeBlockIoReadWriteBlocks (This,
> +      MediaId, Lba, BufferSize, Buffer, FALSE);
> +}
> +
> +/**
> +  Write BufferSize bytes from Lba into Buffer.
> +
> +  @param  This       Indicates a pointer to the calling context.
> +  @param  MediaId    The media ID that the write request is for.
> +  @param  Lba        The starting logical block address to be written. The caller is
> +                     responsible for writing to only legitimate locations.
> +  @param  BufferSize Size of Buffer, must be a multiple of device block size.
> +  @param  Buffer     A pointer to the source buffer for the data.
> +
> +  @retval EFI_SUCCESS           The data was written correctly to the device.
> +  @retval EFI_WRITE_PROTECTED   The device can not be written to.
> +  @retval EFI_DEVICE_ERROR      The device reported an error while performing the write.
> +  @retval EFI_NO_MEDIA          There is no media in the device.
> +  @retval EFI_MEDIA_CHNAGED     The MediaId does not matched the current device.
> +  @retval EFI_BAD_BUFFER_SIZE   The Buffer was not a multiple of the block size of the device.
> +  @retval EFI_INVALID_PARAMETER The write request contains LBAs that are not valid,
> +                                or the buffer is not on proper alignment.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoWriteBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL          *This,
> +  IN UINT32                         MediaId,
> +  IN EFI_LBA                        Lba,
> +  IN UINTN                          BufferSize,
> +  IN VOID                           *Buffer
> +  )
> +{
> +  return XenPvBlkDxeBlockIoReadWriteBlocks (This,
> +      MediaId, Lba, BufferSize, Buffer, TRUE);
> +}
> +
> +/**
> +  Flush the Block Device.
> +
> +  @param  This              Indicates a pointer to the calling context.
> +
> +  @retval EFI_SUCCESS       All outstanding data was written to the device
> +  @retval EFI_DEVICE_ERROR  The device reported an error while writting back the data
> +  @retval EFI_NO_MEDIA      There is no media in the device.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoFlushBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL  *This
> +  )
> +{
> +  XenPvBlockSync (XEN_BLOCK_FRONT_FROM_BLOCK_IO (This));
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reset the block device hardware.
> +
> +  @param[in]  This                 Indicates a pointer to the calling context.
> +  @param[in]  ExtendedVerification Indicates that the driver may perform a more
> +                                   exhausive verfication operation of the device
> +                                   during reset.
> +
> +  @retval EFI_SUCCESS          The device was reset.
> +  @retval EFI_DEVICE_ERROR     The device is not functioning properly and could
> +                               not be reset.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoReset (
> +  IN EFI_BLOCK_IO2_PROTOCOL  *This,
> +  IN BOOLEAN                 ExtendedVerification
> +  )
> +{
> +  // Is there anything to do ?
> +  // since initialisation is done in Start (driver binding)

Won't we leak memory? Or will it after an Reset do Stop, then Start?


> +  DEBUG ((EFI_D_INFO, "%a %d\n", __func__, __LINE__));
> +  return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockIo.h b/OvmfPkg/XenPvBlkDxe/BlockIo.h
> new file mode 100644
> index 0000000..ec0fe2b
> --- /dev/null
> +++ b/OvmfPkg/XenPvBlkDxe/BlockIo.h
> @@ -0,0 +1,109 @@
> +
> +/** @file
> +  TODO: Brief Description of UEFI Driver XenPvBlkDxe
> +
> +  TODO: Detailed Description of UEFI Driver XenPvBlkDxe
> +
> +  TODO: Copyright for UEFI Driver XenPvBlkDxe
> +
> +  TODO: License for UEFI Driver XenPvBlkDxe
> +
> +**/
> +
> +/**
> +  Read BufferSize bytes from Lba into Buffer.
> +
> +  @param  This       Indicates a pointer to the calling context.
> +  @param  MediaId    Id of the media, changes every time the media is replaced.
> +  @param  Lba        The starting Logical Block Address to read from
> +  @param  BufferSize Size of Buffer, must be a multiple of device block size.
> +  @param  Buffer     A pointer to the destination buffer for the data. The caller is
> +                     responsible for either having implicit or explicit ownership of the buffer.
> +
> +  @retval EFI_SUCCESS           The data was read correctly from the device.
> +  @retval EFI_DEVICE_ERROR      The device reported an error while performing the read.
> +  @retval EFI_NO_MEDIA          There is no media in the device.
> +  @retval EFI_MEDIA_CHANGED     The MediaId does not matched the current device.
> +  @retval EFI_BAD_BUFFER_SIZE   The Buffer was not a multiple of the block size of the device.
> +  @retval EFI_INVALID_PARAMETER The read request contains LBAs that are not valid,
> +                                or the buffer is not on proper alignment.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoReadBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL          *This,
> +  IN UINT32                         MediaId,
> +  IN EFI_LBA                        Lba,
> +  IN UINTN                          BufferSize,
> +  OUT VOID                          *Buffer
> +  );
> +
> +/**
> +  Write BufferSize bytes from Lba into Buffer.
> +
> +  @param  This       Indicates a pointer to the calling context.
> +  @param  MediaId    The media ID that the write request is for.
> +  @param  Lba        The starting logical block address to be written. The caller is
> +                     responsible for writing to only legitimate locations.
> +  @param  BufferSize Size of Buffer, must be a multiple of device block size.
> +  @param  Buffer     A pointer to the source buffer for the data.
> +
> +  @retval EFI_SUCCESS           The data was written correctly to the device.
> +  @retval EFI_WRITE_PROTECTED   The device can not be written to.
> +  @retval EFI_DEVICE_ERROR      The device reported an error while performing the write.
> +  @retval EFI_NO_MEDIA          There is no media in the device.
> +  @retval EFI_MEDIA_CHNAGED     The MediaId does not matched the current device.
> +  @retval EFI_BAD_BUFFER_SIZE   The Buffer was not a multiple of the block size of the device.
> +  @retval EFI_INVALID_PARAMETER The write request contains LBAs that are not valid,
> +                                or the buffer is not on proper alignment.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoWriteBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL          *This,
> +  IN UINT32                         MediaId,
> +  IN EFI_LBA                        Lba,
> +  IN UINTN                          BufferSize,
> +  IN VOID                           *Buffer
> +  );
> +
> +/**
> +  Flush the Block Device.
> +
> +  @param  This              Indicates a pointer to the calling context.
> +
> +  @retval EFI_SUCCESS       All outstanding data was written to the device
> +  @retval EFI_DEVICE_ERROR  The device reported an error while writting back the data
> +  @retval EFI_NO_MEDIA      There is no media in the device.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoFlushBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL  *This
> +  );
> +
> +/**
> +  Reset the block device hardware.
> +
> +  @param[in]  This                 Indicates a pointer to the calling context.
> +  @param[in]  ExtendedVerification Indicates that the driver may perform a more
> +                                   exhausive verfication operation of the device
> +                                   during reset.
> +
> +  @retval EFI_SUCCESS          The device was reset.
> +  @retval EFI_DEVICE_ERROR     The device is not functioning properly and could
> +                               not be reset.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenPvBlkDxeBlockIoReset (
> +  IN EFI_BLOCK_IO2_PROTOCOL  *This,
> +  IN BOOLEAN                 ExtendedVerification
> +  );
> +
> +extern EFI_BLOCK_IO_MEDIA  gXenPvBlkDxeBlockIoMedia;
> +extern EFI_BLOCK_IO_PROTOCOL  gXenPvBlkDxeBlockIo;
> diff --git a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
> index 7ec7d60..3991e83 100644
> --- a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
> +++ b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
> @@ -266,6 +266,7 @@ XenPvBlkDxeDriverBindingStart (
>    EFI_STATUS Status;
>    XENBUS_PROTOCOL *XenbusIo;
>    XEN_BLOCK_FRONT_DEVICE *Dev;
> +  EFI_BLOCK_IO_MEDIA *Media;
>  
>    Status = gBS->OpenProtocol (
>                  ControllerHandle,
> @@ -284,6 +285,37 @@ XenPvBlkDxeDriverBindingStart (
>      return Status;
>    }
>  
> +  CopyMem (&Dev->BlockIo, &gXenPvBlkDxeBlockIo, sizeof (EFI_BLOCK_IO_PROTOCOL));
> +  Media = AllocateCopyPool (sizeof (EFI_BLOCK_IO_MEDIA),
> +                            &gXenPvBlkDxeBlockIoMedia);
> +  if (Dev->MediaInfo.VDiskInfo & VDISK_REMOVABLE) {
> +    Media->RemovableMedia = TRUE;
> +  }
> +  Media->MediaPresent = TRUE;
> +  Media->ReadOnly = !Dev->MediaInfo.ReadWrite;
> +  if (Dev->MediaInfo.CdRom) {
> +    // If it's a cdrom, the blocksize value need to be 2048 for OVMF to
> +    // recognize it as a cdrom:
> +    //    MdeModulePkg/Universal/Disk/PartitionDxe/ElTorito.c
> +    Media->BlockSize = 2048;
> +  } else {
> +    Media->BlockSize = Dev->MediaInfo.SectorSize;
> +  }
> +  Media->LastBlock = Dev->MediaInfo.Sectors - 1;
> +  // XXX What about LowestAlignedLba, LogicalBlocksPerPhysicalBlock and OptimalTransferLengthGranularity?

I think you can skip  LogicalBlocksPerPhysicalBlock and OptimalTransferLengthGranularity
as they are used for discard operations.

The LowestAlignedLba .. no idea.

> +  Dev->BlockIo.Media = Media;
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (
> +                    &ControllerHandle,
> +                    &gEfiBlockIoProtocolGuid, &Dev->BlockIo,
> +                    NULL
> +                    );
> +  if (EFI_ERROR (Status)) {
> +    // XXX: uninit everythings

 XenPvBlockFrontShutdown (Dev); ?
 FreePool(Media) ?

> +    DEBUG ((EFI_D_ERROR, "XenPvBlk: install protocol fail: %r\n", Status));
> +    return Status;
> +  }
> +
>    return EFI_SUCCESS;
>  }
>  
> @@ -322,6 +354,33 @@ XenPvBlkDxeDriverBindingStop (
>    IN EFI_HANDLE                   *ChildHandleBuffer OPTIONAL
>    )
>  {
> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
> +  XEN_BLOCK_FRONT_DEVICE *Dev;
> +  EFI_BLOCK_IO_MEDIA *Media;
> +  EFI_STATUS Status;
> +
> +  Status = gBS->OpenProtocol (
> +                  ControllerHandle, &gEfiBlockIoProtocolGuid,
> +                  (VOID **)&BlockIo,
> +                  This->DriverBindingHandle, ControllerHandle,
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gBS->UninstallProtocolInterface (ControllerHandle,
> +                  &gEfiBlockIoProtocolGuid, BlockIo);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Media = BlockIo->Media;
> +  Dev = XEN_BLOCK_FRONT_FROM_BLOCK_IO (BlockIo);
> +  XenPvBlockFrontShutdown (Dev);
> +
> +  FreePool (Media);
> +
>    gBS->CloseProtocol (ControllerHandle, &gXenbusProtocolGuid,
>           This->DriverBindingHandle, ControllerHandle);
>  
> diff --git a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> index 186d0ee..3120137 100644
> --- a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> +++ b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> @@ -76,6 +76,7 @@ extern EFI_COMPONENT_NAME_PROTOCOL  gXenPvBlkDxeComponentName;
>  //
>  #include "DriverBinding.h"
>  #include "ComponentName.h"
> +#include "BlockIo.h"
>  
>  
>  
> diff --git a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> index a43d006..b379020 100644
> --- a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +++ b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> @@ -32,6 +32,8 @@
>    ComponentName.h
>    BlockFront.c
>    BlockFront.h
> +  BlockIo.c
> +  BlockIo.h
>  
>  
>  [LibraryClasses]
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel