On 06/01/15 14:08, Heyi Guo wrote:
> Fix SCT test errors for VirtioBlkDxe with ReadBlocks interface:
>
> 1. Media present and media ID should be checked first according to
> UEFI spec: "The function must return EFI_NO_MEDIA or
> EFI_MEDIA_CHANGED even if LBA, BufferSize, or Buffer are invalid
> so the caller can probe for changes in media state".
(*)
> 2. Check Buffer to be not NULL, or we will get below error from QEMU
> and the emulation will exit abnormally:
> qemu-system-aarch64: virtio: error trying to map MMIO memory
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi....@linaro.org>
> ---
> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 862957c..36f0fa5 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -363,10 +363,31 @@ VirtioBlkReadBlocks (
> {
> VBLK_DEV *Dev;
> EFI_STATUS Status;
> + EFI_BLOCK_IO_MEDIA *Media;
> +
> + if (This == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
This is a bug in the caller. According to the UEFI specification,
release 2.5, EFI_BLOCK_IO_PROTOCOL.ReadBlocks(), there is no requirement
to check for This==NULL.
EFI_INVALID_PARAMETER is documented for:
The read request contains LBAs that are not valid, or the buffer is
not on proper alignment.
(If a caller cannot be bothered to ensure the following minimum sanity:
Status = BlockIo->ReadBlocks (BlockIo, ...
and instead writes
Status = BlockIo->ReadBLocks (RandomCrapThatCanBeNull
then returning EFI_INVALID_PARAMETER to it won't help.)
In any case, please show me a *normative* reference that requires this
check. (The SCT is not normative, as far as I know.)
> +
> + Media = This->Media;
> +
> + // Check media first according to UEFI spec
> + if (!Media) {
> + return EFI_INVALID_PARAMETER;
> + }
Again, not required by the spec.
More importantly, this condition is impossible in VirtioBlk.c. See the
following assignment in VirtioBlkInit():
Dev->BlockIo.Media = &Dev->BlockIoMedia;
> + if (!Media->MediaPresent) {
> + return EFI_NO_MEDIA;
> + }
Impossible. Same function,
Dev->BlockIoMedia.MediaPresent = TRUE;
(There is no support for media absence; it is always present.)
> + if (Media->MediaId != MediaId) {
> + return EFI_MEDIA_CHANGED;
> + }
Impossible. Same function,
Dev->BlockIoMedia.MediaId = 0;
There is no support for media change.
Regarding the quote I marked with (*), the full paragraph goes:
> If there is no media in the device, the function returns
> EFI_NO_MEDIA. If the MediaId is not the ID for the current media in
> the device, the function returns EFI_MEDIA_CHANGED. The function
> must return EFI_NO_MEDIA or EFI_MEDIA_CHANGED even if LBA,
> BufferSize, or Buffer are invalid so the caller can probe for changes
> in media state.
Clearly, a caller is supposed to pass only such MediaId parameters to
EFI_BLOCK_IO_PROTOCOL.ReadBlocks() that it has earlier retrieved from
.Media.MediaId. For VirtioBlk.c, that implies the caller can only pass
zero as MediaId, and that will always match.
(This can also be expressed as "the driver already returns
EFI_MEDIA_CHANGED in all cases when the caller makes a valid call, *and*
the media has changed" -- which is never. In VirtioBlk.c, either you
make a valid call (and then the media is the same) or the call is invalid.)
So, unless a user of the protocol instance deliberately changes fields
of the protocol instance (which is obviously forbidden), or passes
garbage parameters intentionally, these things can't happen.
>
> if (BufferSize == 0) {
> return EFI_SUCCESS;
> }
> + if (Buffer == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
The specification expressly says,
The caller is responsible for either having implicit or explicit
ownership of the buffer.
>
> Dev = VIRTIO_BLK_FROM_BLOCK_IO (This);
> Status = VerifyReadWriteRequest (
>
If Jordan or someone else gives you an R-b for this, I won't NAK the
patch. However, I certainly won't ACK it either.
I understand that you're just trying to clean up SCT test errors, but
the SCT is not normative. It has bugs (apparently). Trying to be
defensive *via error retvals* in the face of obvious caller
responsibility is futile.
If you added ASSERT()s instead, I might ACK that, since they would state
invariants. (It won't help you though work around the SCT bugs; the
ASSERT()s will just report that the SCT violates those invariants.)
Please file bugs against the SCT, or else convince me that VirtioBlk.c
is the culprit. (Or, get someone else to ACK your patch.)
Thanks
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
