Re: [edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method

["Nikita Leshenko" <nikita.leshchenko@oracle.com>]

> On 20 Apr 2020, at 20:30, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 04/14/20 19:38, Nikita Leshenko wrote:
>> Machines should be able to boot after this commit. Tested with different
>> Linux distributions (Ubuntu, CentOS) and different Windows
>> versions (Windows 7, Windows 10, Server 2016).
>> 
>> Ref: https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2390__;!!GqivPVa7Brio!JfeG4MsveGF5K9VB4618njXWmMprNv9SIfBg5g6ZHp5jolyqIheckAunOt0SAymJ0j1Ywg$ 
>> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> ---
>> .../Include/IndustryStandard/FusionMptScsi.h  |  19 +-
>> OvmfPkg/MptScsiDxe/MptScsi.c                  | 369 +++++++++++++++++-
>> OvmfPkg/MptScsiDxe/MptScsiDxe.inf             |   3 +
>> OvmfPkg/OvmfPkg.dec                           |   3 +
>> 4 files changed, 390 insertions(+), 4 deletions(-)
>> 
> [...]
> 
> (12) Does this device support 64-bit DMA?
Yes it does, but in an inconvenient way. The high 32 bits are passed in the
handshake (SenseBufferHighAddr, HostMfaHighAddr) and the low 32 bits are passed
on the request queue/SenseBufferLowAddress. Now that I think about it again,
since we have only one request, reply and buffer, I'll just enable dual-cycle
and pass the high addresses on initialization.

> [...]
> (15) I don't see why hoisting MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE
> before the "switch" is helpful. That value can only take effect if the
> XxxTransferLength field (matching the direction) is zero. But I don't
> think those explicit zero checks buy us much.
> 
> I would simply remove the zero comparisons, and always go with either
> TXDIR_READ or TXDIR_WRITE (dependent on Packet->DataDirection). And, in
> case the data size is zero, then just set / copy zero bytes.
We can't just set/copy zero bytes. If transfer size is zero, we must set
MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE, otherwise the device (at least under
QEMU) will complain. But I'll simplify the code by setting TXDIR_NONE only if
the transfer size is zero.

> [...]
> (19) Assuming we can expose the reply frame, but not the request, will
> the driver continue to behave fine (without explicit rollback actions
> for the reply frame)?
> 
> Does "IoReplyEnqueued" help with handling this?
Yes, IoReply is put on a queue and the device may use it to communicate errors
in case the request fails. If the request fails and we get an IoReply, we set
IoReplyEnqueued to FALSE so that we know to enqueue it on the next request. If
we fail while *submitting* the request, the reply frame will remain enqueued
(IoReplyEnqueued == TRUE) and we'll be able to use it for a following request.

> 
> (Just a question, not necessarily a code change request.)
> 
> [...]
>> +  if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
>> +    //
>> +    // This is a turbo reply, everything is good
>> +    //
>> +    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>> +    Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> 
> (20) Does "turbo reply" mean that you have to update neither
> XxxTransferLength, nor SenseDataLength, on output?
Turbo reply means that the command was successful, which means that we don't
need to update XxxTransferLength. I think we need to zero SenseDataLength
since successful commands don't generate SENSE data.

> [...]
> (23) Would it make sense to check that the device only *lowers*
> SenseDataLength with the above assignment?
Yes, will add that.

> [...]
> I think TransferCount should be set in all cases. Does the device really
> only set TransferCount for TARGET_GOOD?
I assumed that it only updates it when TARGET_GOOD for some reason, but I
checked again and I was wrong. I will read TransferCount unconditionally.

> [...]
>> +[Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
> 
> (36) General -- can you run "BaseTools/Scripts/SetupGit.py" in your edk2
> clone? It will improve the file order in which changes are formatted
> into patch files. A patch is easier to read if the INF, DEC, and header
> file changes come first.
Sorry, I'm pretty sure I ran it in the past, I'll run it again...

ACK on the rest of the comments (also in the other patches).
Nikita
>