Re: [edk2-devel] [PATCH v4 19/37] UefiCpuPkg: Add MMIO method in CpuIo2Dxe

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 20 Dec 2023 at 02:57, Ni, Ray <ray.ni@intel.com> wrote:
>
> It’s new to me that PCI IO (not MMIO) space is accessed through MMIO.
>
> PCIE spec defines different TLP types for Memory and I/O request in Transaction Layer chapter.
>
> If CpuIoDxe driver issues Memory request to a IO space inside a PCIE device, how does PCIE device claim the TPL packet and response?
>

Hello Ray,

On the opposite side of the PCI host bridge, these are all port I/O
transactions, and the endpoint is not aware of the distinction between
native port I/O and translated port I/O.

ARM CPUs do not implement port I/O at all, and so every host bridge
that implements port I/O support on the PCI side does so by exposing a
special MMIO resource window in the CPU physical address space that
gets translated to port I/O accesses at the opposite side.

This means that an implementation of the CpuIo2 protocol can only be
provided in a meaningful way if there are port I/O capable PCI host
bridges in the system, and some bookkeeping is needed to keep track of
the mapping between the special MMIO ranges on the CPU side and the
port I/O ranges on the PCI side. Note that this is not so different
from MMIO translation, where the mapping between MMIO is not 1:1
between CPU and PCI.

That said, I am not sure I follow why PcdPciIoTranslationIsEnabled is
needed. MMIO translation and IO translation are both properties of the
PCI host bridge implementation, so having a system wide PCD for this
seems unnecessary to me. But perhaps I missed something?



>
> From: Chao Li <lichao@loongson.cn>
> Sent: Tuesday, December 19, 2023 9:04 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
> Subject: Re: [edk2-devel] [PATCH v4 19/37] UefiCpuPkg: Add MMIO method in CpuIo2Dxe
>
>
>
> Hi Ray,
>
> Can you please review this patch? Thank you!
>
>
>
> Thanks,
> Chao
>
> On 2023/12/12 21:12, Chao Li wrote:
>
> CpuIo2Dxe only supports IO to access PCI IO. Some ARCH requires
>
> MMIO to access PCI IO, add the MMIO access method in CpuIo2Dxe.
>
>
>
> The MMIO methods depend on PcdPciIoTranslationIsEnabled and
>
> PcdPciIoTransLation. The code is referenced from ArmPkg.
>
>
>
> Build-tested only (with "OvmfPkgX64.dsc").
>
>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584
>
>
>
> Cc: Ray Ni <ray.ni@intel.com>
>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Signed-off-by: Chao Li <lichao@loongson.cn>
>
> ---
>
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c   | 149 +++++++++++++++++++----------
>
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h   |   2 +
>
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf |   8 +-
>
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.uni |   2 +
>
>  4 files changed, 111 insertions(+), 50 deletions(-)
>
>
>
> diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c
>
> index 87f4f805ca..cd31977af2 100644
>
> --- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c
>
> +++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c
>
> @@ -3,6 +3,8 @@
>
>
>
>  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>
> +Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> +Copyright (c) 2023 Loongson Technology Corporation Limited. All rights reserved.<BR>
>
>
>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -149,7 +151,7 @@ CpuIoCheckParameter (
>
>    //
>
>    // Since MAX_ADDRESS can be the maximum integer value supported by the CPU and Count
>
>    // can also be the maximum integer value supported by the CPU, this range
>
> -  // check must be adjusted to avoid all oveflow conditions.
>
> +  // check must be adjusted to avoid all overflow conditions.
>
>    //
>
>    // The following form of the range check is equivalent but assumes that
>
>    // MAX_ADDRESS and MAX_IO_PORT_ADDRESS are of the form (2^n - 1).
>
> @@ -398,6 +400,18 @@ CpuIoServiceRead (
>
>    EFI_CPU_IO_PROTOCOL_WIDTH  OperationWidth;
>
>    UINT8                      *Uint8Buffer;
>
>
>
> +  UINT8 EFIAPI  (*CpuIoRead8) (
>
> +    UINTN
>
> +    );
>
> +
>
> +  UINT16 EFIAPI  (*CpuIoRead16) (
>
> +    UINTN
>
> +    );
>
> +
>
> +  UINT32 EFIAPI  (*CpuIoRead32) (
>
> +    UINTN
>
> +    );
>
> +
>
>    Status = CpuIoCheckParameter (FALSE, Width, Address, Count, Buffer);
>
>    if (EFI_ERROR (Status)) {
>
>      return Status;
>
> @@ -410,37 +424,48 @@ CpuIoServiceRead (
>
>    OutStride      = mOutStride[Width];
>
>    OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH)(Width & 0x03);
>
>
>
> -  //
>
> -  // Fifo operations supported for (mInStride[Width] == 0)
>
> -  //
>
> -  if (InStride == 0) {
>
> -    switch (OperationWidth) {
>
> -      case EfiCpuIoWidthUint8:
>
> -        IoReadFifo8 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      case EfiCpuIoWidthUint16:
>
> -        IoReadFifo16 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      case EfiCpuIoWidthUint32:
>
> -        IoReadFifo32 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      default:
>
> -        //
>
> -        // The CpuIoCheckParameter call above will ensure that this
>
> -        // path is not taken.
>
> -        //
>
> -        ASSERT (FALSE);
>
> -        break;
>
> +  if (FeaturePcdGet (PcdPciIoTranslationIsEnabled) == FALSE) {
>
> +    //
>
> +    // Fifo operations supported for (mInStride[Width] == 0)
>
> +    //
>
> +    if (InStride == 0) {
>
> +      switch (OperationWidth) {
>
> +        case EfiCpuIoWidthUint8:
>
> +          IoReadFifo8 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        case EfiCpuIoWidthUint16:
>
> +          IoReadFifo16 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        case EfiCpuIoWidthUint32:
>
> +          IoReadFifo32 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        default:
>
> +          //
>
> +          // The CpuIoCheckParameter call above will ensure that this
>
> +          // path is not taken.
>
> +          //
>
> +          ASSERT (FALSE);
>
> +          break;
>
> +      }
>
>      }
>
> +
>
> +    CpuIoRead8  = IoRead8;
>
> +    CpuIoRead16 = IoRead16;
>
> +    CpuIoRead32 = IoRead32;
>
> +  } else {
>
> +    Address    += PcdGet64 (PcdPciIoTranslation);
>
> +    CpuIoRead8  = MmioRead8;
>
> +    CpuIoRead16 = MmioRead16;
>
> +    CpuIoRead32 = MmioRead32;
>
>    }
>
>
>
>    for (Uint8Buffer = Buffer; Count > 0; Address += InStride, Uint8Buffer += OutStride, Count--) {
>
>      if (OperationWidth == EfiCpuIoWidthUint8) {
>
> -      *Uint8Buffer = IoRead8 ((UINTN)Address);
>
> +      *Uint8Buffer = CpuIoRead8 ((UINTN)Address);
>
>      } else if (OperationWidth == EfiCpuIoWidthUint16) {
>
> -      *((UINT16 *)Uint8Buffer) = IoRead16 ((UINTN)Address);
>
> +      *((UINT16 *)Uint8Buffer) = CpuIoRead16 ((UINTN)Address);
>
>      } else if (OperationWidth == EfiCpuIoWidthUint32) {
>
> -      *((UINT32 *)Uint8Buffer) = IoRead32 ((UINTN)Address);
>
> +      *((UINT32 *)Uint8Buffer) = CpuIoRead32 ((UINTN)Address);
>
>      }
>
>    }
>
>
>
> @@ -502,6 +527,21 @@ CpuIoServiceWrite (
>
>    EFI_CPU_IO_PROTOCOL_WIDTH  OperationWidth;
>
>    UINT8                      *Uint8Buffer;
>
>
>
> +  UINT8 EFIAPI  (*CpuIoWrite8) (
>
> +    UINTN,
>
> +    UINT8
>
> +    );
>
> +
>
> +  UINT16 EFIAPI  (*CpuIoWrite16) (
>
> +    UINTN,
>
> +    UINT16
>
> +    );
>
> +
>
> +  UINT32 EFIAPI  (*CpuIoWrite32) (
>
> +    UINTN,
>
> +    UINT32
>
> +    );
>
> +
>
>    //
>
>    // Make sure the parameters are valid
>
>    //
>
> @@ -517,37 +557,48 @@ CpuIoServiceWrite (
>
>    OutStride      = mOutStride[Width];
>
>    OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH)(Width & 0x03);
>
>
>
> -  //
>
> -  // Fifo operations supported for (mInStride[Width] == 0)
>
> -  //
>
> -  if (InStride == 0) {
>
> -    switch (OperationWidth) {
>
> -      case EfiCpuIoWidthUint8:
>
> -        IoWriteFifo8 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      case EfiCpuIoWidthUint16:
>
> -        IoWriteFifo16 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      case EfiCpuIoWidthUint32:
>
> -        IoWriteFifo32 ((UINTN)Address, Count, Buffer);
>
> -        return EFI_SUCCESS;
>
> -      default:
>
> -        //
>
> -        // The CpuIoCheckParameter call above will ensure that this
>
> -        // path is not taken.
>
> -        //
>
> -        ASSERT (FALSE);
>
> -        break;
>
> +  if (FeaturePcdGet (PcdPciIoTranslationIsEnabled) == FALSE) {
>
> +    //
>
> +    // Fifo operations supported for (mInStride[Width] == 0)
>
> +    //
>
> +    if (InStride == 0) {
>
> +      switch (OperationWidth) {
>
> +        case EfiCpuIoWidthUint8:
>
> +          IoWriteFifo8 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        case EfiCpuIoWidthUint16:
>
> +          IoWriteFifo16 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        case EfiCpuIoWidthUint32:
>
> +          IoWriteFifo32 ((UINTN)Address, Count, Buffer);
>
> +          return EFI_SUCCESS;
>
> +        default:
>
> +          //
>
> +          // The CpuIoCheckParameter call above will ensure that this
>
> +          // path is not taken.
>
> +          //
>
> +          ASSERT (FALSE);
>
> +          break;
>
> +      }
>
>      }
>
> +
>
> +    CpuIoWrite8  = IoWrite8;
>
> +    CpuIoWrite16 = IoWrite16;
>
> +    CpuIoWrite32 = IoWrite32;
>
> +  } else {
>
> +    Address     += PcdGet64 (PcdPciIoTranslation);
>
> +    CpuIoWrite8  = MmioWrite8;
>
> +    CpuIoWrite16 = MmioWrite16;
>
> +    CpuIoWrite32 = MmioWrite32;
>
>    }
>
>
>
>    for (Uint8Buffer = (UINT8 *)Buffer; Count > 0; Address += InStride, Uint8Buffer += OutStride, Count--) {
>
>      if (OperationWidth == EfiCpuIoWidthUint8) {
>
> -      IoWrite8 ((UINTN)Address, *Uint8Buffer);
>
> +      CpuIoWrite8 ((UINTN)Address, *Uint8Buffer);
>
>      } else if (OperationWidth == EfiCpuIoWidthUint16) {
>
> -      IoWrite16 ((UINTN)Address, *((UINT16 *)Uint8Buffer));
>
> +      CpuIoWrite16 ((UINTN)Address, *((UINT16 *)Uint8Buffer));
>
>      } else if (OperationWidth == EfiCpuIoWidthUint32) {
>
> -      IoWrite32 ((UINTN)Address, *((UINT32 *)Uint8Buffer));
>
> +      CpuIoWrite32 ((UINTN)Address, *((UINT32 *)Uint8Buffer));
>
>      }
>
>    }
>
>
>
> diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h
>
> index 7ebde0759b..5256a583e1 100644
>
> --- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h
>
> +++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h
>
> @@ -2,6 +2,8 @@
>
>    Internal include file for the CPU I/O 2 Protocol.
>
>
>
>  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> +Copyright (c) 2023 Loongson Technology Corporation Limited. All rights reserved.<BR>
>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
>  **/
>
> diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>
> index 499258491f..271c47371b 100644
>
> --- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>
> +++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>
> @@ -3,6 +3,8 @@
>
>  #
>
>  # Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>
>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>
> +# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> +# Copyright (c) 2023 Loongson Technology Corporation Limited. All rights reserved.<BR>
>
>  #
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #
>
> @@ -20,7 +22,7 @@
>
>  #
>
>  # The following information is for reference only and not required by the build tools.
>
>  #
>
> -#  VALID_ARCHITECTURES           = IA32 X64 EBC
>
> +#  VALID_ARCHITECTURES           = IA32 X64 EBC ARM AARCH64 LOONGARCH64
>
>  #
>
>
>
>  [Sources]
>
> @@ -37,6 +39,10 @@
>
>    IoLib
>
>    UefiBootServicesTableLib
>
>
>
> +[Pcd]
>
> +  gEfiMdePkgTokenSpaceGuid.PcdPciIoTranslationIsEnabled
>
> +  gEfiMdePkgTokenSpaceGuid.PcdPciIoTranslation
>
> +
>
>  [Protocols]
>
>    gEfiCpuIo2ProtocolGuid                         ## PRODUCES
>
>
>
> diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.uni b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.uni
>
> index 8d4e5dd6b4..14a36ff888 100644
>
> --- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.uni
>
> +++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.uni
>
> @@ -4,6 +4,8 @@
>
>  // Produces the CPU I/O 2 Protocol by using the services of the I/O Library.
>
>  //
>
>  // Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> +// Copyright (c) 2023 Loongson Technology Corporation Limited. All rights reserved.<BR>
>
>  //
>
>  // SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  //
>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112749): https://edk2.groups.io/g/devel/message/112749
Mute This Topic: https://groups.io/mt/103261693/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-