From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Heyi Guo <heyi.guo@linaro.org>, edk2-devel@lists.01.org
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Star Zeng <star.zeng@intel.com>, Eric Dong <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
Michael D Kinney <michael.d.kinney@intel.com>
Subject: Re: [RFC v2 1/2] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Thu, 22 Feb 2018 16:55:28 +0800 [thread overview]
Message-ID: <29d9d1d1-0cc3-6888-5fda-367bf9b2ef2d@Intel.com> (raw)
In-Reply-To: <1519282474-94811-2-git-send-email-heyi.guo@linaro.org>
On 2/22/2018 2:54 PM, Heyi Guo wrote:
> This is the draft patch for the discussion posted in edk2-devel
> mailing list:
> https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html
>
> As discussed in the mailing list, we'd like to add support for PCI
> address translation which is necessary for some non-x86 platforms. I
> also want to minimize the changes to the generic host bridge driver
> and platform PciHostBridgeLib implemetations, so additional two
> interfaces are added to expose translation information of the
> platform. To be generic, I add translation for each type of IO or
> memory resources.
>
> The patch is still a RFC, so I only passed the build for qemu64 and
> the function has not been tested yet.
>
> Please let me know your comments about it.
>
> Thanks.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 57 ++++++++----
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++++++++++++++++++---
> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 1 +
> 3 files changed, 131 insertions(+), 28 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index 1494848..fa22d8d 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -32,6 +32,29 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
> EFI_EVENT mIoMmuEvent;
> VOID *mIoMmuRegistration;
>
> +STATIC
> +UINT64
> +GetTranslationByResourceType (
> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> + IN PCI_RESOURCE_TYPE ResourceType
> + )
> +{
> + switch (ResourceType) {
> + case TypeIo:
> + return RootBridge->Io.Translation;
> + case TypeMem32:
> + return RootBridge->Mem.Translation;
> + case TypePMem32:
> + return RootBridge->PMem.Translation;
> + case TypeMem64:
> + return RootBridge->MemAbove4G.Translation;
> + case TypePMem64:
> + return RootBridge->PMemAbove4G.Translation;
> + default:
> + return 0;
> + }
> +}
> +
> /**
> Ensure the compatibility of an IO space descriptor with the IO aperture.
>
> @@ -412,7 +435,7 @@ InitializePciHostBridge (
>
> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> Status = AddIoSpace (
> - RootBridges[Index].Io.Base,
> + RootBridges[Index].Io.Base + RootBridges[Index].Io.Translation,
> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1
> );
> ASSERT_EFI_ERROR (Status);
> @@ -422,7 +445,7 @@ InitializePciHostBridge (
> EfiGcdIoTypeIo,
> 0,
> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1,
> - &RootBridges[Index].Io.Base,
> + &RootBridges[Index].Io.Base + RootBridges[Index].Io.Translation,
> gImageHandle,
> NULL
> );
> @@ -444,13 +467,13 @@ InitializePciHostBridge (
> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) {
> if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) {
> Status = AddMemoryMappedIoSpace (
> - MemApertures[MemApertureIndex]->Base,
> + MemApertures[MemApertureIndex]->Base + MemApertures[MemApertureIndex]->Translation,
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> EFI_MEMORY_UC
> );
> ASSERT_EFI_ERROR (Status);
> Status = gDS->SetMemorySpaceAttributes (
> - MemApertures[MemApertureIndex]->Base,
> + MemApertures[MemApertureIndex]->Base + MemApertures[MemApertureIndex]->Translation,
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> EFI_MEMORY_UC
> );
> @@ -463,7 +486,7 @@ InitializePciHostBridge (
> EfiGcdMemoryTypeMemoryMappedIo,
> 0,
> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1,
> - &MemApertures[MemApertureIndex]->Base,
> + &MemApertures[MemApertureIndex]->Base + MemApertures[MemApertureIndex]->Translation,
> gImageHandle,
> NULL
> );
> @@ -828,8 +851,8 @@ NotifyPhase (
> FALSE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (15, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1),
> - RootBridge->Io.Limit
> + ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) + RootBridge->Io.Translation,
> + RootBridge->Io.Limit + RootBridge->Io.Translation
> );
> break;
>
> @@ -838,8 +861,8 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (63, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1),
> - RootBridge->MemAbove4G.Limit
> + ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) + RootBridge->MemAbove4G.Translation,
> + RootBridge->MemAbove4G.Limit + RootBridge->MemAbove4G.Translation
> );
> if (BaseAddress != MAX_UINT64) {
> break;
> @@ -853,8 +876,8 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (31, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1),
> - RootBridge->Mem.Limit
> + ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) + RootBridge->Mem.Translation,
> + RootBridge->Mem.Limit + RootBridge->Mem.Translation
> );
> break;
>
> @@ -863,8 +886,8 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (63, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1),
> - RootBridge->PMemAbove4G.Limit
> + ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) + RootBridge->PMemAbove4G.Translation,
> + RootBridge->PMemAbove4G.Limit + RootBridge->PMemAbove4G.Translation
> );
> if (BaseAddress != MAX_UINT64) {
> break;
> @@ -877,8 +900,8 @@ NotifyPhase (
> TRUE,
> RootBridge->ResAllocNode[Index].Length,
> MIN (31, BitsOfAlignment),
> - ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1),
> - RootBridge->PMem.Limit
> + ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) + RootBridge->PMem.Translation,
> + RootBridge->PMem.Limit + RootBridge->PMem.Translation
> );
> break;
>
> @@ -1152,6 +1175,7 @@ StartBusEnumeration (
> Descriptor->AddrSpaceGranularity = 0;
> Descriptor->AddrRangeMin = RootBridge->Bus.Base;
> Descriptor->AddrRangeMax = 0;
> + // Ignore translation offset for bus
> Descriptor->AddrTranslationOffset = 0;
> Descriptor->AddrLen = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1;
>
> @@ -1421,7 +1445,8 @@ GetProposedResources (
> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
> Descriptor->GenFlag = 0;
> - Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base;
> + Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base -
> + GetTranslationByResourceType (RootBridge, Index);
> Descriptor->AddrRangeMax = 0;
> Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
> Descriptor->AddrLen = RootBridge->ResAllocNode[Index].Length;
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index dc06c16..bd3394a 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -86,12 +86,23 @@ CreateRootBridge (
> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"",
> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> ));
> + // We don't see any scenario for bus translation, so translation for bus is just ignored.
> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n", Bridge->Bus.Base, Bridge->Bus.Limit));
> - DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n", Bridge->Io.Base, Bridge->Io.Limit));
> - DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n", Bridge->Mem.Base, Bridge->Mem.Limit));
> - DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
> - DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base, Bridge->PMem.Limit));
> - DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
> + DEBUG ((DEBUG_INFO, " Io: %lx - %lx translation=%lx\n",
> + Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
> + ));
> + DEBUG ((DEBUG_INFO, " Mem: %lx - %lx translation=%lx\n",
> + Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
> + ));
> + DEBUG ((DEBUG_INFO, " MemAbove4G: %lx - %lx translation=%lx\n",
> + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Bridge->MemAbove4G.Translation
> + ));
> + DEBUG ((DEBUG_INFO, " PMem: %lx - %lx translation=%lx\n",
> + Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
> + ));
> + DEBUG ((DEBUG_INFO, " PMemAbove4G: %lx - %lx translation=%lx\n",
> + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Bridge->PMemAbove4G.Translation
> + ));
>
> //
> // Make sure Mem and MemAbove4G apertures are valid
> @@ -403,6 +414,28 @@ RootBridgeIoCheckParameter (
> return EFI_SUCCESS;
> }
>
> +EFI_STATUS
> +RootBridgeIoGetMemTranslationByAddress (
> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> + IN UINT64 Address,
> + IN OUT UINT64 *Translation
> + )
> +{
> + if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) {
> + *Translation = RootBridge->Mem.Translation;
> + } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) {
> + *Translation = RootBridge->PMem.Translation;
> + } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) {
> + *Translation = RootBridge->MemAbove4G.Translation;
> + } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) {
> + *Translation = RootBridge->PMemAbove4G.Translation;
> + } else {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Polls an address in memory mapped I/O space until an exit condition is met,
> or a timeout occurs.
> @@ -658,13 +691,22 @@ RootBridgeIoMemRead (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> + UINT64 Translation;
>
> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
> Count, Buffer);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> - return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> }
>
> /**
> @@ -705,13 +747,22 @@ RootBridgeIoMemWrite (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> + UINT64 Translation;
>
> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address,
> Count, Buffer);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> - return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + Translation, Count, Buffer);
> }
>
> /**
> @@ -746,6 +797,8 @@ RootBridgeIoIoRead (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +
> Status = RootBridgeIoCheckParameter (
> This, IoOperation, Width,
> Address, Count, Buffer
> @@ -753,7 +806,10 @@ RootBridgeIoIoRead (
> if (EFI_ERROR (Status)) {
> return Status;
> }
> - return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +
> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->Io.Translation, Count, Buffer);
> }
>
> /**
> @@ -788,6 +844,8 @@ RootBridgeIoIoWrite (
> )
> {
> EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +
> Status = RootBridgeIoCheckParameter (
> This, IoOperation, Width,
> Address, Count, Buffer
> @@ -795,7 +853,10 @@ RootBridgeIoIoWrite (
> if (EFI_ERROR (Status)) {
> return Status;
> }
> - return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> +
> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +
> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address + RootBridge->Io.Translation, Count, Buffer);
> }
>
> /**
> @@ -1615,25 +1676,41 @@ RootBridgeIoConfiguration (
>
> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> + // According to UEFI 2.7, RootBridgeIo::Configuration should return address
> + // range in CPU view, and TranslationOffset = PCI view - CPU view.
> Descriptor->AddrRangeMin = ResAllocNode->Base;
> Descriptor->AddrRangeMax = ResAllocNode->Base + ResAllocNode->Length - 1;
> Descriptor->AddrLen = ResAllocNode->Length;
> switch (ResAllocNode->Type) {
>
> case TypeIo:
> - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> + // According to UEFI 2.7, translation = PCI address - CPU address,
> + // so we change the sign here to make it consistent with UEFI spec.
> + // The other translations will be treated as the same.
> + Descriptor->AddrTranslationOffset = -RootBridge->Io.Translation;
> break;
>
> case TypePMem32:
> - Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> + Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> + Descriptor->AddrTranslationOffset = -RootBridge->PMem.Translation;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> + Descriptor->AddrSpaceGranularity = 32;
> + break;
> +
> case TypeMem32:
> + Descriptor->AddrTranslationOffset = -RootBridge->Mem.Translation;
> Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> Descriptor->AddrSpaceGranularity = 32;
> break;
>
> case TypePMem64:
> - Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> + Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> + Descriptor->AddrTranslationOffset = -RootBridge->PMemAbove4G.Translation;
> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> + Descriptor->AddrSpaceGranularity = 64;
> case TypeMem64:
> + Descriptor->AddrTranslationOffset = -RootBridge->MemAbove4G.Translation;
> Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> Descriptor->AddrSpaceGranularity = 64;
> break;
> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> index d42e9ec..b9e8c0f 100644
> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> @@ -22,6 +22,7 @@
> typedef struct {
> UINT64 Base;
> UINT64 Limit;
> + UINT64 Translation;
> } PCI_ROOT_BRIDGE_APERTURE;
>
> typedef struct {
>
Heyi,
The address stored in GCD database should only be in CPU-view.
I think you might improperly add the CPU-view address range to GCD.
Thanks,
Ray
--
Thanks,
Ray
next prev parent reply other threads:[~2018-02-22 8:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-22 6:54 [RFC v2 0/2] Add translation support to generic PCIHostBridge Heyi Guo
2018-02-22 6:54 ` [RFC v2 1/2] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-22 8:55 ` Ni, Ruiyu [this message]
2018-02-22 8:56 ` Ni, Ruiyu
2018-02-22 9:43 ` Laszlo Ersek
2018-02-22 6:54 ` [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
2018-02-22 10:41 ` Laszlo Ersek
2018-02-23 1:10 ` Guo Heyi
2018-02-22 10:06 ` [RFC v2 0/2] Add translation support to generic PCIHostBridge Laszlo Ersek
2018-02-22 10:32 ` Guo Heyi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=29d9d1d1-0cc3-6888-5fda-367bf9b2ef2d@Intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox