On 25 March 2016 at 09:27, Leendert van Doorn <leend...@paramecium.org> wrote: > Wow, this is a blast from the past. One of my Friday afternoon hacking > projects is seeing the light of day ☺ Not all of this patch is mine though, I > didn’t add the if (PciEmulation) clauses and they aren’t needed as far as I > remember. > > The problem that part of this code solves is that Map() can return a > different device address than the input host address. The Arm implementation > of DmaLib has cases where this can occur, specifically when I/O needs to use > uncached memory and the source is in cached memory. The original AHCI driver > however used the host address in places where it should be using the device > address. This patch fixes that. On x86 this bug wasn’t a problem because the > two are always the same but on ARM systems that may not be the case (See > ArmPkg/…/DmaLib). My original patch fixed this driver to consistently use > device address as returned by Map(). > > I’m not sure why the if (PciEmulation) clauses were added. Presumably someone > wanted to be cautious and not break the other uses of this driver. The way I > remember the driver and DmaLib logic that shouldn’t be necessary. >
Hello Leendert, Thanks for the explanation. This does mean that the patch in its current form does not make sense. PCI emulation is used in various ARM platforms to work around the [invalid] assumption on the part of the AHCI, EHCI and various other drivers in Tianocore that these host controllers are always on a PCI bus. This is often not the case on ARM platforms, and that is why many such platforms resort to some kind of PCI emulation, which implements the PCI I/O protocol on behalf of non-PCI hardware, and impersonates PCI versions of those devices. So there are two things to address: - we need a separate host controller driver, or an intermediate AHCI host protocol to support AHCI controllers on non-discoverable 'platform' buses - we need to fix (as this patch does) the AHCI PCI code to use the correct address field when it needs the device address -- Ard. > On 3/24/16, 5:50 PM, "Tian, Feng" <feng.t...@intel.com> wrote: > >>Duran >> >>Could you explain more about this fix? >> >>1) What's the meaning of PCI emulation here? >>2) What's the difference between normal case and this PCI emulation? >>3) Why this PCI emulation case needs to update driver to directly use PCI >>address for filling transfer descriptor content? >> >>Thanks >>Feng >> >>-----Original Message----- >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leo >>Duran >>Sent: Friday, March 25, 2016 4:30 AM >>To: edk2-devel@lists.01.org >>Cc: Leo Duran; Leendert van Doorn; leif.lindh...@linaro.org; >>ard.biesheu...@linaro.org >>Subject: [edk2] [PATCH] MdeModulePkg: support AHCI controller using PCI >>emulation >> >>From: Leendert van Doorn <leend...@paramecium.org> >> >>Contributed-under: TianoCore Contribution Agreement 1.0 >>Signed-off-by: Leo Duran <leo.du...@amd.com> >>--- >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 88 >> +++++++++++++++++----- >> .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf | 1 + >> MdeModulePkg/MdeModulePkg.dec | 6 ++ >> 3 files changed, 75 insertions(+), 20 deletions(-) >> >>diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >>b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >>index f64a340..0c8887c 100644 >>--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >>+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >>@@ -529,6 +529,7 @@ AhciBuildCommand ( >> UINTN MemAddr; >> DATA_64 Data64; >> UINT32 Offset; >>+ BOOLEAN PciEmulation = PcdGetBool (PcdAtaPassThruPciEmulation); >> >> // >> // Filling the PRDT >>@@ -542,22 +543,32 @@ AhciBuildCommand ( >> // >> ASSERT (PrdtNumber <= 65535); >> >>- Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFis) + sizeof >>(EFI_AHCI_RECEIVED_FIS) * Port; >>+ if (PciEmulation) >>+ Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFisPciAddr) + sizeof >>+ (EFI_AHCI_RECEIVED_FIS) * Port; else >>+ Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFis) + sizeof >>+ (EFI_AHCI_RECEIVED_FIS) * Port; >> >> BaseAddr = Data64.Uint64; >> >> ZeroMem ((VOID *)((UINTN) BaseAddr), sizeof (EFI_AHCI_RECEIVED_FIS)); >> >>- ZeroMem (AhciRegisters->AhciCommandTable, sizeof (EFI_AHCI_COMMAND_TABLE)); >>+ if (PciEmulation) >>+ ZeroMem (AhciRegisters->AhciCommandTablePciAddr, sizeof >>+ (EFI_AHCI_COMMAND_TABLE)); else >>+ ZeroMem (AhciRegisters->AhciCommandTable, sizeof >>+ (EFI_AHCI_COMMAND_TABLE)); >> >> CommandFis->AhciCFisPmNum = PortMultiplier; >> >>- CopyMem (&AhciRegisters->AhciCommandTable->CommandFis, CommandFis, sizeof >>(EFI_AHCI_COMMAND_FIS)); >>+ if (PciEmulation) >>+ CopyMem (&AhciRegisters->AhciCommandTablePciAddr->CommandFis, >>+ CommandFis, sizeof (EFI_AHCI_COMMAND_FIS)); else >>+ CopyMem (&AhciRegisters->AhciCommandTable->CommandFis, CommandFis, >>+ sizeof (EFI_AHCI_COMMAND_FIS)); >> >> Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + >> EFI_AHCI_PORT_CMD; >> if (AtapiCommand != NULL) { >> CopyMem ( >>- &AhciRegisters->AhciCommandTable->AtapiCmd, >>+ PciEmulation ? &AhciRegisters->AhciCommandTablePciAddr->AtapiCmd : >>+ &AhciRegisters->AhciCommandTable->AtapiCmd, >> AtapiCommand, >> AtapiCommandLength >> ); >>@@ -576,14 +587,26 @@ AhciBuildCommand ( >> >> for (PrdtIndex = 0; PrdtIndex < PrdtNumber; PrdtIndex++) { >> if (RemainedData < EFI_AHCI_MAX_DATA_PER_PRDT) { >>- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >>(UINT32)RemainedData - 1; >>+ if (PciEmulation) >>+ >>AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbc = >>(UINT32)RemainedData - 1; >>+ else >>+ >>+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >>+ (UINT32)RemainedData - 1; >> } else { >>- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >>EFI_AHCI_MAX_DATA_PER_PRDT - 1; >>+ if (PciEmulation) >>+ >>AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbc = >>EFI_AHCI_MAX_DATA_PER_PRDT - 1; >>+ else >>+ >>+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >>+ EFI_AHCI_MAX_DATA_PER_PRDT - 1; >> } >> >> Data64.Uint64 = (UINT64)MemAddr; >>- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDba = >>Data64.Uint32.Lower32; >>- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbau = >>Data64.Uint32.Upper32; >>+ if (PciEmulation) { >>+ >>AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDba = >>Data64.Uint32.Lower32; >>+ >>AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbau = >>Data64.Uint32.Upper32; >>+ } else { >>+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDba = >>Data64.Uint32.Lower32; >>+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbau = >>Data64.Uint32.Upper32; >>+ } >>+ >> RemainedData -= EFI_AHCI_MAX_DATA_PER_PRDT; >> MemAddr += EFI_AHCI_MAX_DATA_PER_PRDT; >> } >>@@ -592,19 +615,29 @@ AhciBuildCommand ( >> // Set the last PRDT to Interrupt On Complete >> // >> if (PrdtNumber > 0) { >>- AhciRegisters->AhciCommandTable->PrdtTable[PrdtNumber - 1].AhciPrdtIoc = >>1; >>+ if (PciEmulation) >>+ AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtNumber - >>1].AhciPrdtIoc = 1; >>+ else >>+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtNumber - >>+ 1].AhciPrdtIoc = 1; >> } >> >> CopyMem ( >>- (VOID *) ((UINTN) AhciRegisters->AhciCmdList + (UINTN) CommandSlotNumber >>* sizeof (EFI_AHCI_COMMAND_LIST)), >>+ PciEmulation ? (VOID *) ((UINTN) AhciRegisters->AhciCmdListPciAddr + >>(UINTN) CommandSlotNumber * sizeof (EFI_AHCI_COMMAND_LIST)) : >>+ (VOID *) ((UINTN) AhciRegisters->AhciCmdList + >>+ (UINTN) CommandSlotNumber * sizeof (EFI_AHCI_COMMAND_LIST)), >> CommandList, >> sizeof (EFI_AHCI_COMMAND_LIST) >> ); >> >> Data64.Uint64 = (UINT64)(UINTN) AhciRegisters->AhciCommandTablePciAddr; >>- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtba = >>Data64.Uint32.Lower32; >>- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtbau = >>Data64.Uint32.Upper32; >>- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdPmp = >>PortMultiplier; >>+ if (PciEmulation) { >>+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdCtba = >>Data64.Uint32.Lower32; >>+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdCtbau = >>Data64.Uint32.Upper32; >>+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdPmp = >>PortMultiplier; >>+ } else { >>+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtba = >>Data64.Uint32.Lower32; >>+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtbau = >>Data64.Uint32.Upper32; >>+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdPmp = >>PortMultiplier; >>+ } >> >> } >> >>@@ -707,6 +740,7 @@ AhciPioTransfer ( >> BOOLEAN InfiniteWait; >> BOOLEAN PioFisReceived; >> BOOLEAN D2hFisReceived; >>+ BOOLEAN PciEmulation = PcdGetBool >>(PcdAtaPassThruPciEmulation); >> >> if (Timeout == 0) { >> InfiniteWait = TRUE; >>@@ -774,7 +808,10 @@ AhciPioTransfer ( >> // >> // Check the status and wait the driver sending data >> // >>- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>(EFI_AHCI_RECEIVED_FIS); >>+ if (PciEmulation) >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); else >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); >> >> if (Read && (AtapiCommand == 0)) { >> // >>@@ -814,7 +851,10 @@ AhciPioTransfer ( >> break; >> } >> >>- PrdCount = *(volatile UINT32 *) >>(&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc)); >>+ if (PciEmulation) >>+ PrdCount = *(volatile UINT32 *) >>(&(AhciRegisters->AhciCmdListPciAddr[0].AhciCmdPrdbc)); >>+ else >>+ PrdCount = *(volatile UINT32 *) >>+ (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc)); >> if (PrdCount == DataCount) { >> Status = EFI_SUCCESS; >> break; >>@@ -930,7 +970,6 @@ AhciDmaTransfer ( >> EFI_AHCI_COMMAND_LIST CmdList; >> UINTN FisBaseAddr; >> UINT32 PortTfd; >>- >> EFI_PCI_IO_PROTOCOL *PciIo; >> EFI_TPL OldTpl; >> >>@@ -1025,9 +1064,12 @@ AhciDmaTransfer ( >> } >> >> // >>- // Wait for command compelte >>+ // Wait for command complete >> // >>- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>(EFI_AHCI_RECEIVED_FIS); >>+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); else >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); >> Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET; >> if (Task != NULL) { >> // >>@@ -1175,7 +1217,10 @@ AhciNonDataTransfer ( >> // >> // Wait device sends the Response Fis >> // >>- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>(EFI_AHCI_RECEIVED_FIS); >>+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); else >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); >> Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET; >> Status = AhciWaitMemSet ( >> Offset, >>@@ -1544,7 +1589,10 @@ AhciAtaSmartReturnStatusCheck ( >> (EFI_IO_BUS_ATA_ATAPI | EFI_IOB_ATA_BUS_SMART_ENABLE) >> ); >> >>- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>(EFI_AHCI_RECEIVED_FIS); >>+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); else >>+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >>+ (EFI_AHCI_RECEIVED_FIS); >> >> Value = *(UINT32 *) (FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET); >> >>diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >>b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >>index 82d5f7a..e999a14 100644 >>--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >>+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >>@@ -70,6 +70,7 @@ >> >> [Pcd] >> gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable ## SOMETIMES_CONSUMES >>+ gEfiMdeModulePkgTokenSpaceGuid.PcdAtaPassThruPciEmulation ## CONSUMES >> >> # [Event] >> # EVENT_TYPE_PERIODIC_TIMER ## SOMETIMES_CONSUMES diff --git >> a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index >> 61db352..5f96cf1 100644 >>--- a/MdeModulePkg/MdeModulePkg.dec >>+++ b/MdeModulePkg/MdeModulePkg.dec >>@@ -1401,6 +1401,12 @@ >> # @Prompt Enable ATA S.M.A.R.T feature. >> gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable|TRUE|BOOLEAN|0x00010065 >> >>+ ## Indicates if PCI emulation is enabled for ATA PassThru.<BR><BR> >>+ # TRUE - PCI emulation for ATA PassThru is enabled.<BR> >>+ # FALSE - PCI emulation for ATA PassThru is disabled.<BR> >>+ # @Prompt Enable PCI emulation for ATA PassThru. >>+ >>+ gEfiMdeModulePkgTokenSpaceGuid.PcdAtaPassThruPciEmulation|FALSE|BOOLEA >>+ N|0x00010080 >>+ >> ## Indicates if full PCI enumeration is disabled.<BR><BR> >> # TRUE - Full PCI enumeration is disabled.<BR> >> # FALSE - Full PCI enumeration is not disabled.<BR> >>-- >>1.9.1 >> >>_______________________________________________ >>edk2-devel mailing list >>edk2-devel@lists.01.org >>https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
