[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 18/18] OvmfPkg/XenPvBlkDxe: Add BlockIo.
On Wed, Jul 16, 2014 at 03:57:23PM -0400, Konrad Rzeszutek Wilk wrote: > 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@xxxxxxxxxx> > > --- > > 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 ? Yes. > > + 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? No, GRUB2 is worth, it's not even sector aligned... I think my comment is about why a Size of EFI_PAGE_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST did not work and that I should find a better value than page_size * 8. > > + IoData.Size = MIN(EFI_PAGE_SIZE * 8,//BLKIF_MAX_SEGMENTS_PER_REQUEST, > > + BufferSize); > > Why 8 ? I don't remember, but maybe the first value that works, or just a random value that I've seen somewhere else ... anyway, that need to be change. > > + 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 */ That right. > > + 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? > No, I don't think so. Reset is probably to reset the hardware. That a function that I need to take a closer look at. > > + 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. Ok. > The LowestAlignedLba .. no idea. I think in pv block case, LowestAlignedLba will always be 0. > > + Dev->BlockIo.Media = Media; > > + > > + Status = gBS->InstallMultipleProtocolInterfaces ( > > + &ControllerHandle, > > + &gEfiBlockIoProtocolGuid, &Dev->BlockIo, > > + NULL > > + ); > > + if (EFI_ERROR (Status)) { > > + // XXX: uninit everythings > > XenPvBlockFrontShutdown (Dev); ? > FreePool(Media) ? Yes. > > + 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@xxxxxxxxxxxxx > > http://lists.xen.org/xen-devel -- Anthony PERARD _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |