From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A2A40211C6063 for ; Fri, 1 Feb 2019 04:55:34 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C19B22CD804; Fri, 1 Feb 2019 12:55:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8A86319742; Fri, 1 Feb 2019 12:55:32 +0000 (UTC) To: Nikita Leshenko , edk2-devel@lists.01.org Cc: liran.alon@oracle.com References: <20190131100724.20907-1-nikita.leshchenko@oracle.com> <20190131100724.20907-13-nikita.leshchenko@oracle.com> From: Laszlo Ersek Message-ID: <3f977365-0c31-9e68-b85a-ca47544c348c@redhat.com> Date: Fri, 1 Feb 2019 13:55:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190131100724.20907-13-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 01 Feb 2019 12:55:33 +0000 (UTC) Subject: Re: [PATCH 12/14] OvmfPkg/MptScsiDxe: Implement the PassThru method X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Feb 2019 12:55:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/31/19 11:07, 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). > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Nikita Leshenko > Reviewed-by: Aaron Young > Reviewed-by: Liran Alon > --- > OvmfPkg/MptScsiDxe/MptScsi.c | 255 +++++++++++++++++++++++++++++- > OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 + > OvmfPkg/OvmfPkg.dec | 2 + > 3 files changed, 256 insertions(+), 4 deletions(-) So, this is the patch where you pass pointers (DRAM addresses, as expressed from the CPU's point of view) to the PCI device: > +STATIC > +EFI_STATUS > +MptScsiPopulateRequest ( > + IN UINT8 Target, > + IN UINT64 Lun, > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet, > + OUT MPT_SCSI_REQUEST_WITH_SG *Request > + ) > +{ > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL || > + Packet->CdbLength > sizeof (Request->Header.CDB)) { > + return EFI_UNSUPPORTED; > + } > + > + if (Target > 0 || Lun > 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (Packet->InTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) { > + Packet->InTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH; > + return EFI_BAD_BUFFER_SIZE; > + } > + if (Packet->OutTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) { > + Packet->OutTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH; > + return EFI_BAD_BUFFER_SIZE; > + } > + > + ZeroMem (Request, sizeof (*Request)); > + Request->Header.TargetID = Target; > + // It's 1 and not 0, for some reason... > + Request->Header.LUN[1] = Lun; > + Request->Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST; > + Request->Header.MessageContext = 1; // Hardcoded, we handle one request at a time > + > + Request->Header.CDBLength = Packet->CdbLength; > + CopyMem (Request->Header.CDB, Packet->Cdb, Packet->CdbLength); > + > + Request->Header.SenseBufferLength = Packet->SenseDataLength; > + ASSERT ((UINTN) Packet->SenseData <= MAX_UINT32); > + Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData; > + > + Request->Sg.EndOfList = 1; > + Request->Sg.EndOfBuffer = 1; > + Request->Sg.LastElement = 1; > + Request->Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE; > + > + switch (Packet->DataDirection) > + { > + case EFI_EXT_SCSI_DATA_DIRECTION_READ: > + Request->Header.DataLength = Packet->InTransferLength; > + Request->Sg.Length = Packet->InTransferLength; > + ASSERT ((UINTN) Packet->InDataBuffer <= MAX_UINT32); > + Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer; > + Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ; > + break; > + case EFI_EXT_SCSI_DATA_DIRECTION_WRITE: > + Request->Header.DataLength = Packet->OutTransferLength; > + Request->Sg.Length = Packet->OutTransferLength; > + ASSERT ((UINTN) Packet->OutDataBuffer <= MAX_UINT32); > + Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer; > + Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE; > + Request->Sg.BufferContainsData = 1; > + break; > + } > + > + return EFI_SUCCESS; > +} and > +STATIC > +EFI_STATUS > +MptScsiSendRequest ( > + IN MPT_SCSI_DEV *Dev, > + IN MPT_SCSI_REQUEST_WITH_SG *Request, > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > + ) > +{ > + EFI_STATUS Status; > + > + // Make sure Request is fully written > + MemoryFence (); > + > + ASSERT ((UINTN) Request <= MAX_UINT32); > + Status = Out32 (Dev, MPT_REG_REQ_Q, (UINT32)(UINTN) Request); > + if (EFI_ERROR (Status)) { > + // We couldn't enqueue the request, report it as an adapter error > + Packet->InTransferLength = 0; > + Packet->OutTransferLength = 0; > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > + Packet->SenseDataLength = 0; > + return EFI_DEVICE_ERROR; > + } > + > + return EFI_SUCCESS; > +} and > > @@ -291,10 +496,52 @@ MptScsiPassThru ( > IN EFI_EVENT Event OPTIONAL > ) > { > - DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n", > - Packet->DataDirection, > - Packet->InTransferLength, Packet->OutTransferLength)); > - return EFI_UNSUPPORTED; > + EFI_STATUS Status; > + MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This); > + > + // Noisy print, but useful when debugging this area > + // DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n", > + // Packet->DataDirection, > + // Packet->InTransferLength, Packet->OutTransferLength)); > + > + Status = MptScsiPopulateRequest (*Target, Lun, Packet, &Dev->IoRequest); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = MptScsiSendRequest (Dev, &Dev->IoRequest, Packet); > + if (EFI_ERROR (Status)) { > + return Status; > + } You will have to map each such memory transfer explicitly, with the PciIo->Map(), PciIo->Unmap(), PciIo->AllocateBuffer() and PciIo->FreeBuffer() functions. Otherwise, if there is an IOMMU in the system, or the CPU view of memory addresses is not 1:1 identical with the PCI device view of memory addresses for some other reason, then this driver will fail. And, this is a practical concern at this point, not a theoretical one. If OVMF runs in an AMD SEV (Secure Encrypted Virtualization) guest, then the PciIo Map / Unmap operations handle the memory decryption / re-encryption that's necessary for QEMU to see the data in guest RAM. In brief: (1) Use PciIo->Map() with BusMasterRead for a single-shot transfer from the CPU to the PCI device through DRAM. (2) Use PciIo->Map() with BusMasterWrite for a single-shot transfer from the PCI device to the CPU through DRAM. (3) For bi-directional communication (such as request/response within the same buffer), use PciIo->AllocateBuffer(), plus PciIo->Map() with BusMasterCommonBuffer. (4) If the Fusion MPT SCSI controller can handle 64-bit addresses (*after* mapping; that is, in the device address space), then you might want to set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in [PATCH 10/14] OvmfPkg/MptScsiDxe: Set and restore PCI attributes but in a *separate* call from setting (EFI_PCI_IO_ATTRIBUTE_IO | EFI_PCI_IO_ATTRIBUTE_BUS_MASTER). If setting DUAL_ADDRESS_CYCLE fails, that in itself won't prevent the driver from working. If the controller can only handle 32-bit device addresses (or you only want to deal with 32-bit device addresses), then do not set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE. All of the above is described (much better) in the UEFI spec. See EFI_PCI_IO_PROTOCOL | Description, in UEFI-2.7, pages 832-833 for example (those are the page numbers printed on the pages, in reality they are pages 902-903 in the PDF file). Thanks! Laszlo