Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 09/12] PciBusDxe: New PCI feature Max_Read_Req_Size

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

This patch is also uploaded in the following Repo for review:-
https://github.com/ashrafj/edk2-staging/commit/d16d7ea7df972fb711bca22266743c3602cf450b

Thanks
Ashraf

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Javeed,
> Ashraf
> Sent: Friday, November 1, 2019 8:40 PM
> 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 09/12]
> PciBusDxe: New PCI feature Max_Read_Req_Size
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2194
> 
> The code changes are made to enable the configuration of new PCI feature
> Max_Read_Req_Size (MRRS), which defines the memory read request size for
> the PCI transactions, as per the PCI Base Specification 4 Revision 1.
> 
> The code changes are made to configure a common value that is applicable to
> all the child nodes originating from the primary parent root port of the root
> bridge instance, based on following 3 criteria:-
> (1) if platform defines MRRS device policy for any one PCI device in the
>      tree than align all the devices in the PCI tree to that same value
> (2) if platform does not provide device policy for any of the devices in
>     the PCI tree than setup the MRRS value equivalent to MPS value for
>     all PCI devices to meet the criteria for the isochronous traffic
> (3) if platform does not provide device policy for any of the devices in
>     the PCI tree and platform firmware policy has not selected the PCI
>     bus driver to configure the MPS; than configuration of the MRRS is
>     performed based on highest common value of the MPS advertized in the
>     PCI device capability registers of the PCI devices
> 
> This programming of MRRS gets the device-specific platform policy using the
> new PCI Platform Protocol interface, defined in the below record:-
> https://bugzilla.tianocore.org/show_bug.cgi?id=1954
> 
> 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/PciFeatureSupport.c  | 204
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h  |   9 +++++++++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c |  59
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h |  32
> ++++++++++++++++++++++++++++++++
>  5 files changed, 305 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 065ae54..38abd20 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -290,6 +290,7 @@ struct _PCI_IO_DEVICE {
>    // Other PCI features setup flags
>    //
>    UINT8                                     SetupMPS;
> +  UINT8                                     SetupMRRS;
>  };
> 
>  #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \ diff --git
> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> index 8fdaa05..614285f 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> @@ -650,6 +650,121 @@ ProcessMaxPayloadSize (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  The main routine which process the PCI feature Max_Read_Req_Size as
> +per the
> +  device-specific platform policy, as well as in complaince with the
> +PCI Base
> +  specification Revision 4, that aligns the value for the entire PCI
> +heirarchy
> +  starting from its physical PCI Root port / Bridge device.
> +
> +  @param PciDevice                      A pointer to the PCI_IO_DEVICE.
> +  @param PciConfigPhase                 for the PCI feature configuration phases:
> +                                        PciFeatureGetDevicePolicy &
> + PciFeatureSetupPhase  @param PciFeaturesConfigurationTable  pointer to
> + OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> +
> +  @retval EFI_SUCCESS                   processing of PCI feature
> Max_Read_Req_Size
> +                                        is successful.
> +**/
> +EFI_STATUS
> +ProcessMaxReadReqSize (
> +  IN  PCI_IO_DEVICE                           *PciDevice,
> +  IN  PCI_FEATURE_CONFIGURATION_PHASE         PciConfigPhase,
> +  IN  OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> +*PciFeaturesConfigurationTable
> +  )
> +{
> +  PCI_REG_PCIE_DEVICE_CAPABILITY  PciDeviceCap;
> +  UINT8                           MrrsValue;
> +
> +  PciDeviceCap.Uint32 =
> + PciDevice->PciExpStruct.DeviceCapability.Uint32;
> +
> +  if (PciConfigPhase == PciFeatureGetDevicePolicy) {
> +    if (SetupMrrsAsPerDeviceCapability (PciDevice->SetupMRRS)) {
> +      //
> +      // The maximum read request size is not the data packet size of the TLP,
> +      // but the memory read request size, and set to the function as a requestor
> +      // to not exceed this limit.
> +      // However, for the PCI device capable of isochronous traffic; this memory
> read
> +      // request size should not extend beyond the Max_Payload_Size. Thus, in
> case if
> +      // device policy return by platform indicates to set as per device capability
> +      // than set as per Max_Payload_Size configuration value
> +      //
> +      if (SetupMaxPayloadSize ()) {
> +        MrrsValue = PciDevice->SetupMPS;
> +      } else {
> +        //
> +        // in case this driver is not required to configure the Max_Payload_Size
> +        // than consider programming HCF of the device capability's
> Max_Payload_Size
> +        // in this PCI hierarchy; thus making this an implementation specific feature
> +        // which the platform should avoid. For better results, the platform should
> +        // make both the Max_Payload_Size & Max_Read_Request_Size to be
> configured
> +        // by this driver
> +        //
> +        MrrsValue = (UINT8)PciDeviceCap.Bits.MaxPayloadSize;
> +      }
> +    } else {
> +      //
> +      // override as per platform based device policy
> +      //
> +      MrrsValue = TranslateMrrsSetupValueToPci (PciDevice->SetupMRRS);
> +      //
> +      // align this device's Max_Read_Request_Size value to the entire PCI tree
> +      //
> +      if (PciFeaturesConfigurationTable) {
> +        if (!PciFeaturesConfigurationTable->Lock_Max_Read_Request_Size) {
> +          PciFeaturesConfigurationTable->Lock_Max_Read_Request_Size = TRUE;
> +          PciFeaturesConfigurationTable->Max_Read_Request_Size = MrrsValue;
> +        } else {
> +          //
> +          // in case of another user enforced value of MRRS within the same tree,
> +          // pick the smallest between the locked value and this value; to set
> +          // across entire PCI tree nodes
> +          //
> +          MrrsValue = MIN (
> +                        MrrsValue,
> +                        PciFeaturesConfigurationTable->Max_Read_Request_Size
> +                        );
> +          PciFeaturesConfigurationTable->Max_Read_Request_Size = MrrsValue;
> +        }
> +      }
> +    }
> +    //
> +    // align this device's Max_Read_Request_Size to derived configuration value
> +    //
> +    PciDevice->SetupMRRS = MrrsValue;
> +
> +  }
> +
> +  //
> +  // align the Max_Read_Request_Size of the PCI tree based on 3 conditions:
> +  // first, if user defines MRRS for any one PCI device in the tree
> + than align  // all the devices in the PCI tree.
> +  // second, if user override is not define for this PCI tree than
> + setup the MRRS  // based on MPS value of the tree to meet the criteria
> + for the isochronous  // traffic.
> +  // third, if no user override, or platform firmware policy has not
> + selected  // this PCI bus driver to configure the MPS; than configure
> + the MRRS to a  // highest common value of PCI device capability for
> + the MPS found among all  // the PCI devices in this tree  //  if
> + (PciFeaturesConfigurationTable) {
> +    if (PciFeaturesConfigurationTable->Lock_Max_Read_Request_Size) {
> +      PciDevice->SetupMRRS = PciFeaturesConfigurationTable-
> >Max_Read_Request_Size;
> +    } else {
> +      if (SetupMaxPayloadSize ()) {
> +        PciDevice->SetupMRRS = PciDevice->SetupMPS;
> +      } else {
> +        PciDevice->SetupMRRS = MIN (
> +                                PciDevice->SetupMRRS,
> +                                PciFeaturesConfigurationTable->Max_Read_Request_Size
> +                                );
> +      }
> +      PciFeaturesConfigurationTable->Max_Read_Request_Size = PciDevice-
> >SetupMRRS;
> +    }
> +  }
> +  DEBUG (( DEBUG_INFO, "MRRS: %d,", PciDevice->SetupMRRS));
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Overrides the PCI Device Control register MaxPayloadSize register field; if
>    the hardware value is different than the intended value.
> @@ -723,6 +838,79 @@ OverrideMaxPayloadSize (
>    return Status;
>  }
> 
> +/**
> +  Overrides the PCI Device Control register Max_Read_Req_Size register
> +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
> controller.
> +  @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
> +OverrideMaxReadReqSize (
> +  IN PCI_IO_DEVICE          *PciDevice
> +  )
> +{
> +  PCI_REG_PCIE_DEVICE_CONTROL PcieDev;
> +  UINT32                      Offset;
> +  EFI_STATUS                  Status;
> +  EFI_TPL                     OldTpl;
> +
> +  PcieDev.Uint16 = 0;
> +  Offset = PciDevice->PciExpressCapabilityOffset +
> +               OFFSET_OF (PCI_CAPABILITY_PCIEXP, DeviceControl);
> + Status = PciDevice->PciIo.Pci.Read (
> +                                  &PciDevice->PciIo,
> +                                  EfiPciIoWidthUint16,
> +                                  Offset,
> +                                  1,
> +                                  &PcieDev.Uint16
> +                                );
> +  if (EFI_ERROR(Status)){
> +    DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) read
> error!",
> +        Offset
> +    ));
> +    return Status;
> +  }
> +  if (PcieDev.Bits.MaxReadRequestSize != PciDevice->SetupMRRS) {
> +    PcieDev.Bits.MaxReadRequestSize = PciDevice->SetupMRRS;
> +    DEBUG (( DEBUG_INFO, "Max_Read_Request_Size: %d,",
> + PciDevice->SetupMRRS));
> +
> +    //
> +    // 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->PciExpStruct.DeviceControl.Uint16 = PcieDev.Uint16;
> +    } else {
> +      DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) write
> error!",
> +          Offset
> +      ));
> +    }
> +  } else {
> +    DEBUG (( DEBUG_INFO, "No write of MRRS=%d,",
> + PciDevice->SetupMRRS));  }
> +
> +  return Status;
> +}
> +
>  /**
>    helper routine to dump the PCIe Device Port Type  **/ @@ -820,6 +1008,17
> @@ SetupDevicePciFeatures (
>                OtherPciFeaturesConfigTable
>                );
>    }
> +  //
> +  // implementation specific rule:- the MRRS of any PCI device should
> + be processed  // only after the MPS is processed for that device  //
> + if (SetupMaxReadReqSize ()) {
> +    Status = ProcessMaxReadReqSize (
> +              PciDevice,
> +              PciConfigPhase,
> +              OtherPciFeaturesConfigTable
> +              );
> +  }
>    DEBUG ((DEBUG_INFO, "]\n"));
>    return Status;
>  }
> @@ -920,6 +1119,9 @@ ProgramDevicePciFeatures (
>    if (SetupMaxPayloadSize ()) {
>      Status = OverrideMaxPayloadSize (PciDevice);
>    }
> +  if (SetupMaxReadReqSize ()) {
> +    Status = OverrideMaxReadReqSize (PciDevice);  }
>    DEBUG (( DEBUG_INFO, "\n"));
>    return Status;
>  }
> @@ -1035,6 +1237,8 @@ AddPrimaryRootPortNode (
>    if (PciConfigTable) {
>      PciConfigTable->ID                          = PortNumber;
>      PciConfigTable->Max_Payload_Size            =
> PCIE_MAX_PAYLOAD_SIZE_4096B;
> +    PciConfigTable->Max_Read_Request_Size       =
> PCIE_MAX_READ_REQ_SIZE_4096B;
> +    PciConfigTable->Lock_Max_Read_Request_Size  = FALSE;
>    }
>    RootPortNode->OtherPciFeaturesConfigurationTable  = PciConfigTable;
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> index e5ac2a3..96ee6ff 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> @@ -84,6 +84,15 @@ struct _OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> {
>    // size among all the PCI devices in the PCI hierarchy
>    //
>    UINT8                                     Max_Payload_Size;
> +  //
> +  // to configure the PCI feature maximum read request size to maintain
> + the memory  // requester size among all the PCI devices in the PCI
> + hierarchy  //
> +  UINT8                                     Max_Read_Request_Size;
> +  //
> +  // lock the Max_Read_Request_Size for the entire PCI tree of a root
> + port  //
> +  BOOLEAN                                   Lock_Max_Read_Request_Size;
>  };
> 
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> index 99badd6..f032b5d 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> @@ -379,6 +379,29 @@ SetupMpsAsPerDeviceCapability (
>    }
>  }
> 
> +/**
> +  Helper routine to indicate whether the given PCI device specific
> +policy value
> +  dictates to override the Max_Read_Req_Size to a particular value, or
> +set as per
> +  device capability.
> +
> +  @param  MRRS    Input device-specific policy should be in terms of type
> +                  EFI_PCI_CONF_MAX_READ_REQ_SIZE
> +
> +  @retval TRUE    Setup Max_Read_Req_Size as per device capability
> +          FALSE   override as per device-specific platform policy
> +**/
> +BOOLEAN
> +SetupMrrsAsPerDeviceCapability (
> +  IN  UINT8                   MRRS
> +)
> +{
> +  if (MRRS == EFI_PCI_CONF_MAX_READ_REQ_SIZE_AUTO) {
> +    return TRUE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
>  /**
>    Routine to translate the given device-specific platform policy from type
>    EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base
> Specification @@ -413,6 +436,40 @@ TranslateMpsSetupValueToPci (
>    }
>  }
> 
> +/**
> +  Routine to translate the given device-specific platform policy from
> +type
> +  EFI_PCI_CONF_MAX_READ_REQ_SIZE to HW-specific value, as per PCI Base
> +Specification
> +  Revision 4.0; for the PCI feature Max_Read_Req_Size.
> +
> +  @param  MRRS    Input device-specific policy should be in terms of type
> +                  EFI_PCI_CONF_MAX_READ_REQ_SIZE
> +
> +  @retval         Range values for the Max_Read_Req_Size as defined in the PCI
> +                  Base Specification 4.0 **/
> +UINT8
> +TranslateMrrsSetupValueToPci (
> +  IN  UINT8                   MRRS
> +)
> +{
> +  switch (MRRS) {
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_128B:
> +      return PCIE_MAX_READ_REQ_SIZE_128B;
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_256B:
> +      return PCIE_MAX_READ_REQ_SIZE_256B;
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_512B:
> +      return PCIE_MAX_READ_REQ_SIZE_512B;
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_1024B:
> +      return PCIE_MAX_READ_REQ_SIZE_1024B;
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_2048B:
> +      return PCIE_MAX_READ_REQ_SIZE_2048B;
> +    case EFI_PCI_CONF_MAX_READ_REQ_SIZE_4096B:
> +      return PCIE_MAX_READ_REQ_SIZE_4096B;
> +    default:
> +      return PCIE_MAX_READ_REQ_SIZE_128B;
> +  }
> +}
> +
>  /**
>    Generic routine to setup the PCI features as per its predetermined defaults.
>  **/
> @@ -422,6 +479,7 @@ SetupDefaultsDevicePlatformPolicy (
>    )
>  {
>    PciDevice->SetupMPS = EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO;
> +  PciDevice->SetupMRRS = EFI_PCI_CONF_MAX_READ_REQ_SIZE_AUTO;
>  }
> 
>  /**
> @@ -458,6 +516,7 @@ GetPciDevicePlatformPolicyEx (
>        // platform chipset policies are returned for this PCI device
>        //
>        PciIoDevice->SetupMPS = PciPlatformExtendedPolicy.DeviceCtlMPS;
> +      PciIoDevice->SetupMRRS = PciPlatformExtendedPolicy.DeviceCtlMRRS;
> 
>        DEBUG ((
>            DEBUG_INFO, "[device policy: platform]"
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> index 786c00d..8ed3836 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> @@ -141,6 +141,22 @@ SetupMpsAsPerDeviceCapability (
>    IN  UINT8                   MPS
>  );
> 
> +/**
> +  Helper routine to indicate whether the given PCI device specific
> +policy value
> +  dictates to override the Max_Read_Req_Size to a particular value, or
> +set as per
> +  device capability.
> +
> +  @param  MRRS    Input device-specific policy should be in terms of type
> +                  EFI_PCI_CONF_MAX_READ_REQ_SIZE
> +
> +  @retval TRUE    Setup Max_Read_Req_Size as per device capability
> +          FALSE   override as per device-specific platform policy
> +**/
> +BOOLEAN
> +SetupMrrsAsPerDeviceCapability (
> +  IN  UINT8                   MRRS
> +);
> +
>  /**
>    Routine to translate the given device-specific platform policy from type
>    EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base
> Specification @@ -156,4 +172,20 @@ UINT8  TranslateMpsSetupValueToPci (
>    IN  UINT8                   MPS
>  );
> +
> +/**
> +  Routine to translate the given device-specific platform policy from
> +type
> +  EFI_PCI_CONF_MAX_READ_REQ_SIZE to HW-specific value, as per PCI Base
> +Specification
> +  Revision 4.0; for the PCI feature Max_Read_Req_Size.
> +
> +  @param  MRRS    Input device-specific policy should be in terms of type
> +                  EFI_PCI_CONF_MAX_READ_REQ_SIZE
> +
> +  @retval         Range values for the Max_Read_Req_Size as defined in the PCI
> +                  Base Specification 4.0 **/
> +UINT8
> +TranslateMrrsSetupValueToPci (
> +  IN  UINT8                   MRRS
> +);
>  #endif
> --
> 2.21.0.windows.1
> 
> 
>