On Wed, 20 Dec 2023 at 02:57, Ni, Ray <ray...@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 <lic...@loongson.cn> > Sent: Tuesday, December 19, 2023 9:04 PM > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com> > Cc: Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann > <kra...@redhat.com>; Leif Lindholm <quic_llind...@quicinc.com>; Ard > Biesheuvel <ardb+tianoc...@kernel.org>; Sami Mujawar <sami.muja...@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...@intel.com> > > Cc: Rahul Kumar <rahul1.ku...@intel.com> > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Leif Lindholm <quic_llind...@quicinc.com> > > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > > Cc: Sami Mujawar <sami.muja...@arm.com> > > Signed-off-by: Chao Li <lic...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
