Hello Feng, John, On 11/03/15 02:48, Tian Feng wrote: > When executing ATAPI cmd at IDE mode, EFI_SUCCESS may be returned wrongly > with old logic but in fact DRQ is not ready and the transaction doesn't > get executed correctly at this time. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Feng Tian <[email protected]> > Cc: Star Zeng <[email protected]> > --- > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > index 4928ed5..f74e5ca 100644 > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > @@ -1949,7 +1949,7 @@ AtaPacketReadWrite ( > // > Status = DRQReady2 (PciIo, IdeRegisters, Timeout); > if (EFI_ERROR (Status)) { > - return CheckStatusRegister (PciIo, IdeRegisters); > + return Status; > } > > // >
Unfortunately, this patch breaks OVMF on QEMU's ide-cd device. The symptoms were reported in <https://github.com/tianocore/edk2/issues/43>; I reproduced the issue and bisected it to this patch (git commit 7cac2401 / SVN 19611). I also confirmed it by reverting the patch on top of current master, and the CD-ROM started to work. I'm adding John Snow, who maintains QEMU's IDE emulation. Can you guys please investigate this patch, and discuss why it breaks on QEMU's ide-cd device? John, if you'd like to browse the code, the following link highlights the line that the above patch changes: https://github.com/tianocore/edk2/blob/7cac2401635317360226a235d1f95f8347081cbb/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c#L1952 As far as I can see, this problem could be timing-sensitive. The DRQReady2() function polls for the DRQ bit to become set, in a certain amount of time. If that doesn't happen (or a definitive error emerges), the function fails. Pre-patch, such failure would not be decisive; the status register would be read and evaluated. Post-patch, the failure is decisive, even if it is just a timeout, and the CheckStatusRegister() call would otherwise return success. ... Hm, I added a debug log right after DRQReady2(), on the error branch (i.e., when it fails). The status code is *not* EFI_TIMEOUT, it is EFI_NOT_READY. Looking at DRQReady2(), this means: - BSY is clear - ERR is clear - DRQ is clear too Apparently, the DRQReady2() function expects that as soon as BSY is clear (and there is no error), DRQ must be immediately set (more or less: not busy, no error, so ready to transfer data). Is that a valid assumption to make? Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
