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

["Ni, Ray" <ray.ni@intel.com>]

Ard,
Let me try to understand:

1. CPU does not support IO instructions to access IO ports.
2. PCI devices contain IO space.
3. A special PCI host bridge can be configured to map a range of MMIO space from CPU side to another range of IO space to PCI device side.

If above is right, I can imagine what problem Chao is trying to solve.
There are following facts:
4. MdeModulePkg/PciHostBridge driver produces PciRootBridgeIo->Io() service which directly calls to CpuIo->Io()

I guess Chao are using MdeModulePkg/PciHostBridge driver.

I agree with Ard that the proper place to change/hook the IO access is inside PciHostBridge driver instead of CpuIo2Dxe if the only IO access is from PCI device driver.

Then PciHostBridgeDxe driver could be enhanced to honor the MMIO-to-IO mapping and only translate the IO request to MMIO which belongs to the IO window.

Thanks,
Ray
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, December 20, 2023 3:41 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: lichao@loongson.cn; 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
> 
> 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 (#112758): https://edk2.groups.io/g/devel/message/112758
Mute This Topic: https://groups.io/mt/103261693/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-