Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI Express feature AtomicOp

["Javeed, Ashraf" <ashraf.javeed@intel.com>]

Link correction made below.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Javeed,
> Ashraf
> Sent: Saturday, February 8, 2020 1:56 AM
> To: devel@edk2.groups.io; Javeed, Ashraf <ashraf.javeed@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12]
> PciBusDxe: New PCI Express feature AtomicOp
> 
> This patch can also be viewed in the following repo:-
https://github.com/ashrafj/edk2-staging/commit/176fccc85714fbbae616821ba355cd4306cecb87
> 
> Thanks
> Ashraf
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Javeed,
> > Ashraf
> > Sent: Saturday, February 8, 2020 1:35 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12]
> > PciBusDxe: New PCI Express feature AtomicOp
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2499
> >
> > This code change handles two PCI Express features related to AtomicOp,
> > in compliance with the PCI Express Base SPecification 5:
> > (1) configuring PCI function as an AtomicOp Requester
> > (2) Enabling of Port Egress blocking depending on AtomicOp Routing
> >     Capability
> >
> > These is programmed based on the following criteria:- For a PCI Bridge device:
> >  - enables as AtomicOp Requester solely based on platform provided device
> >    policy
> >  - enabling of Port Egress blocking is based on platform device policy;
> >    only if its device capability register's AtomicOp Routing bit is 1
> >
> > For an PCI EndPoint (EP) device:
> >  - if the platform's device policy wants to enable the AtomicOp Requester
> >    function for this device, than this device is enabled if all its bridge
> >    devices have the AtomicOp Routing capability bit set
> >  - Enabling of Port Egress disable is not applicable to PCI EP device
> >
> > For an PCI RCiEP device:
> >  - the AtomicOp Requester functionality is enabled solely based on the
> >    device policy provided by platform
> >  - similar to PCI EP device, the enabling of port Egress blocking is not
> >    applicable
> >
> > Signed-off-by: Ashraf Javeed <ashraf.javeed@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h             |   1 +
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.c | 161
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > +++++++++++++++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.h |  35
> > +++++++++++++++++++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c  |  23
> > ++++++++++++++++++++++-
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h  |   5 +++++
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c |  23
> > +++++++++++++++++++++++
> >  6 files changed, 247 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> > index 9b03c12..57ef1b2 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> > @@ -295,6 +295,7 @@ struct _PCI_IO_DEVICE {
> >    PCI_FEATURE_POLICY                        SetupRO;
> >    PCI_FEATURE_POLICY                        SetupNS;
> >    PCI_FEATURE_POLICY                        SetupCTO;
> > +  EFI_PCI_EXPRESS_ATOMIC_OP                 SetupAtomicOp;
> >  };
> >
> >  #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \ diff --git
> > a/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.c
> > index f3f4d39..5c76ba4 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.c
> > @@ -908,3 +908,164 @@ ProgramCompletionTimeout (
> >    return Status;
> >  }
> >
> > +/**
> > +  Routine to setup the AtomicOp Requester in the PCI device, verifies
> > +the routing
> > +  support in the bridge devices, to be complaint as per the PCI Base
> > specification.
> > +
> > +  @param PciDevice                      A pointer to the PCI_IO_DEVICE.
> > +  @param PciExFeatureConfiguration      pointer to common configuration
> > table to
> > +                                        initialize the PCI Express
> > + feature
> > +
> > +  @retval EFI_SUCCESS                   bridge device routing capability is
> successful.
> > +          EFI_INVALID_PARAMETER         input parameter is NULL
> > +**/
> > +EFI_STATUS
> > +SetupAtomicOpRoutingSupport (
> > +  IN PCI_IO_DEVICE                              *PciDevice,
> > +  IN PCI_EXPRESS_FEATURES_CONFIGURATION_TABLE
> > *PciExFeatureConfiguration
> > +  )
> > +{
> > +  //
> > +  // to enable the AtomicOp Requester in the PCI EP device; its Root
> > +Port (bridge),
> > +  // and its PCIe switch upstream & downstream ports (if present)
> > +needs to support
> > +  // the AtomicOp Routing capability.
> > +  //
> > +  if (IS_PCI_BRIDGE (&PciDevice->Pci)) {
> > +    if (!PciDevice-
> > >PciExpressCapabilityStructure.DeviceCapability2.Bits.AtomicOpRouting)
> > >{
> > +      //
> > +      // since the AtomicOp Routing support flag is initialized as TRUE, negate
> > +      // in case if any of the PCI Bridge device in the PCI tree does not support
> > +      // the AtomicOp Routing capability
> > +      //
> > +      if (PciExFeatureConfiguration == NULL) {
> > +        return EFI_INVALID_PARAMETER;
> > +      }
> > +      PciExFeatureConfiguration->AtomicOpRoutingSupported = FALSE;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Overrides the PCI Device Control 2 register AtomicOp Requester
> > +enable field; if
> > +  the hardware value is different than the intended value.
> > +
> > +  @param  PciDevice             A pointer to the PCI_IO_DEVICE instance.
> > +
> > +  @retval EFI_SUCCESS           The data was read from or written to the PCI
> > device.
> > +  @retval EFI_UNSUPPORTED       The address range specified by Offset,
> Width,
> > and Count is not
> > +                                valid for the PCI configuration header of the PCI controller.
> > +  @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invalid.
> > +
> > +**/
> > +EFI_STATUS
> > +ProgramAtomicOp (
> > +  IN PCI_IO_DEVICE                            *PciDevice,
> > +  IN PCI_EXPRESS_FEATURES_CONFIGURATION_TABLE
> > +*PciExFeatureConfiguration
> > +  )
> > +{
> > +  PCI_REG_PCIE_DEVICE_CONTROL2  PcieDev;
> > +  UINT32                        Offset;
> > +  EFI_STATUS                    Status;
> > +  EFI_TPL                       OldTpl;
> > +
> > +  PcieDev.Uint16 = 0;
> > +  Offset = PciDevice->PciExpressCapabilityOffset +
> > +               OFFSET_OF (PCI_CAPABILITY_PCIEXP, DeviceControl2);
> > + Status = PciDevice->PciIo.Pci.Read (
> > +                                  &PciDevice->PciIo,
> > +                                  EfiPciIoWidthUint16,
> > +                                  Offset,
> > +                                  1,
> > +                                  &PcieDev.Uint16
> > +                                  );
> > +  ASSERT (Status == EFI_SUCCESS);
> > +
> > +  if (PciDevice->SetupAtomicOp.Override) {
> > +    //
> > +    // override AtomicOp requester device control bit of the device based on
> the
> > +    // platform request
> > +    //
> > +    if (IS_PCI_BRIDGE (&PciDevice->Pci)) {
> > +      //
> > +      // for a bridge device as AtomicOp Requester function; only
> > + platform
> > override
> > +      // request is used to set the device control register
> > +      //
> > +      if (PcieDev.Bits.AtomicOpRequester != PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester) {
> > +        PcieDev.Bits.AtomicOpRequester = PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester;
> > +      }
> > +      //
> > +      // if platform also request its AtomicOp Egress blocking to be enabled; set
> > +      // only if its device capability's AtomicOpRouting bit is 1.
> > +      // applicable to only the bridge devices
> > +      //
> > +      if (PciDevice->SetupAtomicOp.Enable_AtomicOpEgressBlocking) {
> > +        if (PciDevice-
> > >PciExpressCapabilityStructure.DeviceCapability2.Bits.AtomicOpRouting)
> > >{
> > +          PcieDev.Bits.AtomicOpEgressBlocking = 1;
> > +        }
> > +      }
> > +    } else {
> > +      //
> > +      // in the case of non-bridge device
> > +      //
> > +      if (PciExFeatureConfiguration) {
> > +        //
> > +        // for a device as AtomicOp Requester function; its bridge devices should
> > +        // support the AtomicOp Routing capability to enable the device's as a
> > +        // requester function
> > +        //
> > +        if (PciExFeatureConfiguration->AtomicOpRoutingSupported) {
> > +          if (PcieDev.Bits.AtomicOpRequester != PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester) {
> > +            PcieDev.Bits.AtomicOpRequester = PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester;
> > +          }
> > +        }
> > +      } else {
> > +        //
> > +        // for the RCiEP device or the bridge device without any
> > + child, setup
> > AtomicOp
> > +        // Requester as per platform's device policy
> > +        //
> > +        if (PcieDev.Bits.AtomicOpRequester != PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester) {
> > +          PcieDev.Bits.AtomicOpRequester = PciDevice-
> > >SetupAtomicOp.Enable_AtomicOpRequester;
> > +        }
> > +      }
> > +      //
> > +      // the enabling of AtomicOp Egress Blocking is not applicable
> > + to a non-
> > bridge
> > +      // device
> > +      //
> > +    }
> > +    DEBUG ((
> > +      DEBUG_INFO,
> > +      "AtomicOp=%d,",
> > +      PcieDev.Bits.AtomicOpRequester
> > +      ));
> > +
> > +    //
> > +    // Raise TPL to high level to disable timer interrupt while the
> > + write operation
> > completes
> > +    //
> > +    OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> > +
> > +    Status = PciDevice->PciIo.Pci.Write (
> > +                                    &PciDevice->PciIo,
> > +                                    EfiPciIoWidthUint16,
> > +                                    Offset,
> > +                                    1,
> > +                                    &PcieDev.Uint16
> > +                                    );
> > +    //
> > +    // Restore TPL to its original level
> > +    //
> > +    gBS->RestoreTPL (OldTpl);
> > +
> > +    if (!EFI_ERROR(Status)) {
> > +      PciDevice->PciExpressCapabilityStructure.DeviceControl2.Uint16
> > + =
> > PcieDev.Uint16;
> > +    } else {
> > +      ReportPciWriteError (PciDevice->BusNumber,
> > + PciDevice->DeviceNumber,
> > PciDevice->FunctionNumber, Offset);
> > +    }
> > +  } else {
> > +    DEBUG (( DEBUG_INFO, "No AtomicOp,"));  }
> > +
> > +  return Status;
> > +}
> > +
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.h
> > index 2ee7d4d..1e287fc 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.h
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciExpressFeatures.h
> > @@ -168,4 +168,39 @@ ProgramCompletionTimeout (
> >    IN VOID                   *PciExFeatureConfiguration
> >    );
> >
> > +/**
> > +  Routine to setup the AtomicOp Requester in the PCI device, verifies
> > +the routing
> > +  support in the bridge devices, to be complaint as per the PCI Base
> > specification.
> > +
> > +  @param PciDevice                      A pointer to the PCI_IO_DEVICE.
> > +  @param PciExFeatureConfiguration      pointer to common configuration
> > table to
> > +                                        initialize the PCI Express
> > + feature
> > +
> > +  @retval EFI_SUCCESS                   bridge device routing capability is
> successful.
> > +          EFI_INVALID_PARAMETER         input parameter is NULL
> > +**/
> > +EFI_STATUS
> > +SetupAtomicOpRoutingSupport (
> > +  IN PCI_IO_DEVICE                              *PciDevice,
> > +  IN PCI_EXPRESS_FEATURES_CONFIGURATION_TABLE
> > *PciExFeatureConfiguration
> > +  );
> > +
> > +/**
> > +  Overrides the PCI Device Control 2 register AtomicOp Requester
> > +enable field; if
> > +  the hardware value is different than the intended value.
> > +
> > +  @param  PciDevice             A pointer to the PCI_IO_DEVICE instance.
> > +
> > +  @retval EFI_SUCCESS           The data was read from or written to the PCI
> > device.
> > +  @retval EFI_UNSUPPORTED       The address range specified by Offset,
> Width,
> > and Count is not
> > +                                valid for the PCI configuration header of the PCI controller.
> > +  @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invalid.
> > +
> > +**/
> > +EFI_STATUS
> > +ProgramAtomicOp (
> > +  IN PCI_IO_DEVICE          *PciDevice,
> > +  IN VOID                   *PciExFeatureConfiguration
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > index d4459f3..9d624a0 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > @@ -62,7 +62,7 @@ EFI_PCI_EXPRESS_PLATFORM_POLICY
> > mPciExpressPlatformPolicy = {
> >      //
> >      // support for PCI Express feature - Atomic Op
> >      //
> > -    FALSE,
> > +    TRUE,
> >      //
> >      // support for PCI Express feature - LTR
> >      //
> > @@ -125,6 +125,12 @@ PCI_EXPRESS_FEATURE_INITIALIZATION_POINT
> > mPciExpressFeatureInitializationList[]
> >    },
> >    {
> >      PciExpressFeatureProgramPhase,        PciExpressCto,
> > ProgramCompletionTimeout
> > +  },
> > +  {
> > +    PciExpressFeatureSetupPhase,          PciExpressAtomicOp,
> > SetupAtomicOpRoutingSupport
> > +  },
> > +  {
> > +    PciExpressFeatureProgramPhase,        PciExpressAtomicOp,
> > ProgramAtomicOp
> >    }
> >  };
> >
> > @@ -297,6 +303,16 @@ IsPciExpressFeatureExtendedSetupRequired (
> >          &&
> > PciExpressPolicy[mPciExpressFeatureInitializationList[idx].PciExpressF
> > eatureId]
> > == TRUE
> >      ) {
> >        return TRUE;
> > +    } else if (
> > +        //
> > +        // the PCI Express feature does not require extended setup phase but it
> > +        // does require global flag to track the AtomicOpRouting caoability to
> > +        // be tracked for all its bridge devices
> > +        //
> > +        idx == PciExpressAtomicOp
> > +        && PciExpressPolicy[idx] == TRUE
> > +        ) {
> > +      return TRUE;
> >      }
> >    }
> >
> > @@ -637,6 +653,11 @@ CreatePciRootBridgeDeviceNode (
> >      // the devices in the PCI tree
> >      //
> >      PciConfigTable->Lock_Max_Read_Request_Size  = FALSE;
> > +    //
> > +    // start by assuming the AtomicOp Routing capability is supported in the
> PCI
> > +    // tree
> > +    //
> > +    PciConfigTable->AtomicOpRoutingSupported    = TRUE;
> >    }
> >
> >    RootBridgeNode->PciExFeaturesConfigurationTable  = PciConfigTable;
> > diff -- git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > index a1fc39c..2bd565e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > @@ -80,6 +80,11 @@ struct
> > _PCI_EXPRESS_FEATURES_CONFIGURATION_TABLE {
> >    // lock the Max_Read_Request_Size for the entire PCI tree of a root port
> >    //
> >    BOOLEAN                                   Lock_Max_Read_Request_Size;
> > +  //
> > +  // to record the AtomicOp Routing capability of the PCI Heirarchy
> > + to enable  // the AtomicOp of the EP device  //
> > +  BOOLEAN                                   AtomicOpRoutingSupported;
> >  };
> >
> >  //
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > index 1afea19..2707976 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > @@ -334,6 +334,8 @@ SetupDefaultPciExpressDevicePolicy (
> >
> >    PciDevice->SetupCTO.Override = 0;
> >
> > +  PciDevice->SetupAtomicOp.Override = 0;
> > +
> >  }
> >
> >  /**
> > @@ -450,6 +452,14 @@ GetPciExpressDevicePolicy (
> >        PciDevice->SetupCTO.Override = 0;
> >      }
> >
> > +    //
> > +    // set the device-specific policy for AtomicOp
> > +    //
> > +    if (mPciExpressPlatformPolicy.AtomicOp) {
> > +      PciDevice->SetupAtomicOp =
> PciExpressDevicePolicy.DeviceCtl2AtomicOp;
> > +    } else {
> > +      PciDevice->SetupAtomicOp.Override = 0;
> > +    }
> >
> >      DEBUG ((
> >        DEBUG_INFO,
> > @@ -692,6 +702,19 @@ PciExpressPlatformNotifyDeviceState (
> >      PciExDeviceConfiguration.CTOsupport =
> EFI_PCI_EXPRESS_NOT_APPLICABLE;
> >    }
> >
> > +  //
> > +  // get the device-specific state for the PCIe AtomicOp feature  //
> > + if (mPciExpressPlatformPolicy.AtomicOp) {
> > +
> PciExDeviceConfiguration.DeviceCtl2AtomicOp.Enable_AtomicOpRequester
> > +    = (UINT8)PciDevice-
> > >PciExpressCapabilityStructure.DeviceControl2.Bits.AtomicOpRequester;
> > +
> > PciExDeviceConfiguration.DeviceCtl2AtomicOp.Enable_AtomicOpEgressBlock
> > in
> > g
> > +    =
> > + (UINT8)PciDevice->PciExpressCapabilityStructure.DeviceControl2.Bits.
> > + At
> > + omicOpEgressBlocking;
> > +  } else {
> > +    PciExDeviceConfiguration.DeviceCtl2AtomicOp.Override = 0;
> > +
> > + PciExDeviceConfiguration.DeviceCtl2AtomicOp.Enable_AtomicOpRequester
> > = 0;
> > +
> > + PciExDeviceConfiguration.DeviceCtl2AtomicOp.Enable_AtomicOpEgressBlo
> > + ck
> > + ing = 0;  }
> >
> >    if (mPciExPlatformProtocol != NULL) {
> >      return mPciExPlatformProtocol->NotifyDeviceState (
> > --
> > 2.21.0.windows.1
> >
> >
> >
> 
> 
>