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

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

Ashraf,
Can ProcessMaxPayloadSize() get the minimum payload size for all devices under a certain root port?

I can understand that the payload size stored in the PCI features configuration table is the minimum value.
But the value stored in each PciDevice->SetupMPS is not the minimum value.

So OverrideMaxPayloadSize() should use the value stored in the PCI features configuration table instead of the value stored in PciDevice->SetupMPS.

Thanks,
Ray

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, December 18, 2019 4:38 PM
> To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size
> 
> > > +  UINT8                                     SetupMPS;
> 1. Can it be "MaxPayloadSize"?
> > > +
> > > +  if (PciConfigPhase == PciFeatureGetDevicePolicy) {
> > > +    if (SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS)) {
> 
> 2. Can you replace " SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS" with
> "PciDevice->MaxPayloadSize == EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO"?
> This makes the code more readable.
> 
> > > +      MpsValue = (UINT8)PciDeviceCap.Bits.MaxPayloadSize;
> > > +      //
> > > +      // no change to PCI Root ports without any endpoint device
> > > +      //
> > > +      if (IS_PCI_BRIDGE (&PciDevice->Pci) && PciDeviceCap.Bits.MaxPayloadSize)
> > > {
> > > +        if (IsPciRootPortEmpty (PciDevice)) {
> > > +          MpsValue = PCIE_MAX_PAYLOAD_SIZE_128B;
> > > +        }
> > > +      }
> 
> 3. Above two if-s can be simplified as below? and please also copy the spec requirements here as comments.
> if (IsListEmpty (&PciDevice->ChildList)) {
>   MpsValue = PCIE_MAX_PAYLOAD_SIZE_128B;
> }
> 
> 
> 
> > > +    } else {
> > > +      MpsValue = TranslateMpsSetupValueToPci (PciDevice->SetupMPS);
> 
> 4. The function name can be "UefiToPciMaxPayloadSize()". And I suggest the
> value stored in PciDevice->SetupMPS (MaxPayloadSize) is the macro value
> defined in PciExpress21.h. We could do the conversion just after the GetDevicePolicy() call.
> 
> > > +    }
> > > +    //
> > > +    // discard device policy override request if greater than PCI device capability
> > > +    //
> > > +    PciDevice->SetupMPS = MIN ((UINT8)PciDeviceCap.Bits.MaxPayloadSize,
> > > + MpsValue);  }
> > > +
> > > +  //
> > > +  // align the MPS of the tree to the HCF with this device  //  if
> > > + (PciFeaturesConfigurationTable) {
> > > +    MpsValue = PciFeaturesConfigurationTable->Max_Payload_Size;
> 
> 5. Max_Payload_Size can be "MaxPayloadSize".
> MpsValue can be "MaxPayloadSize".
> 
> > > +
> > > +    MpsValue = MIN (PciDevice->SetupMPS, MpsValue);
> > > +    PciDevice->SetupMPS = MIN (PciDevice->SetupMPS, MpsValue);
> > > +
> > > +    if (MpsValue != PciFeaturesConfigurationTable->Max_Payload_Size) {
> > > +      PciFeaturesConfigurationTable->Max_Payload_Size = MpsValue;
> > > +    }
> > > +  }
> 
> 6. Can you simplify the above logic?
> 
> > > +
> > > +  DEBUG (( DEBUG_INFO,
> > > +      "MPS: %d [DevCap:%d],",
> > > +      PciDevice->SetupMPS, PciDeviceCap.Bits.MaxPayloadSize
> > > +  ));
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +/**
> > > +  Overrides the PCI Device Control register MaxPayloadSize 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
> > > 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
> > > +OverrideMaxPayloadSize (
> > > +  IN PCI_IO_DEVICE          *PciDevice
> > > +  )
> 
> 7. Can this name be "ProgramMaxPayloadSize" because the function does
> the register programming?
> 
> > > +{
> > > +  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
> > > +                                );
> 
> 8. The PciExp is cached in PciExp field in the PciDevice structure. Why do you need
> to read it from HW again?
> 
> > > +  if (EFI_ERROR(Status)){
> > > +    DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) read
> > > error!",
> > > +        Offset
> > > +    ));
> > > +    return Status;
> > > +  }
> > > +  if (PcieDev.Bits.MaxPayloadSize != PciDevice->SetupMPS) {
> > > +    PcieDev.Bits.MaxPayloadSize = PciDevice->SetupMPS;
> > > +    DEBUG (( DEBUG_INFO, "MPS=%d,", PciDevice->SetupMPS));
> > > +
> > > +    //
> > > +    // 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
> > > +      ));
> 
> 9. We can use ASSERT_EFI_ERROR() here. Failure of register writing is a fatal error.
> 
> > > +    }
> > > +  } else {
> > > +    DEBUG (( DEBUG_INFO, "No write of MPS=%d,", PciDevice->SetupMPS));
> 
> 10. Can we skip this debug message?
> 
> > > + }
> > > +
> > > +  return Status;
> > > +}
> > >
> > >  /**
> > >    helper routine to dump the PCIe Device Port Type @@ -669,6 +809,18 @@
> > > SetupDevicePciFeatures (
> > >      }
> > >    }
> > >
> > > +  DEBUG ((DEBUG_INFO, "["));
> > > +  //
> > > +  // process the PCI device Max_Payload_Size feature  //  if
> > > + (SetupMaxPayloadSize ()) {
> > > +    Status = ProcessMaxPayloadSize (
> > > +              PciDevice,
> > > +              PciConfigPhase,
> > > +              OtherPciFeaturesConfigTable
> > > +              );
> 
> 11. Can this function be "CalculatemaxPayloadSize"? Process is too general.
> 
> > > +  }
> > > +  DEBUG ((DEBUG_INFO, "]\n"));
> > >    return Status;
> > >  }
> > >
> > > @@ -765,6 +917,10 @@ ProgramDevicePciFeatures (  {
> > >    EFI_STATUS           Status = EFI_SUCCESS;
> > >
> > > +  if (SetupMaxPayloadSize ()) {
> > > +    Status = OverrideMaxPayloadSize (PciDevice);  }  DEBUG ((
> > > + DEBUG_INFO, "\n"));
> > >    return Status;
> > >  }
> > >
> > > @@ -878,6 +1034,7 @@ AddPrimaryRootPortNode (
> > >                      );
> > >    if (PciConfigTable) {
> > >      PciConfigTable->ID                          = PortNumber;
> > > +    PciConfigTable->Max_Payload_Size            =
> > > PCIE_MAX_PAYLOAD_SIZE_4096B;
> > >    }
> > >    RootPortNode->OtherPciFeaturesConfigurationTable  = PciConfigTable;
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > > index f92d008..e5ac2a3 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > > @@ -79,6 +79,11 @@ struct _OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> > > {
> > >    // Configuration Table ID
> > >    //
> > >    UINTN                                     ID;
> > > +  //
> > > +  // to configure the PCI feature Maximum payload size to maintain the
> > > + data packet  // size among all the PCI devices in the PCI hierarchy
> > > + //
> > > +  UINT8                                     Max_Payload_Size;
> > >  };
> > >
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > > index 238959e..99badd6 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > > @@ -356,6 +356,63 @@ GetPlatformPciOptionRom (
> > >    return Status;
> > >  }
> > >
> > > +/**
> > > +  Helper routine to indicate whether the given PCI device specific
> > > +policy value
> > > +  dictates to override the Max_Payload_Size to a particular value, or
> > > +set as per
> > > +  device capability.
> > > +
> > > +  @param  MPS     Input device-specific policy should be in terms of type
> > > +                  EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > > +
> > > +  @retval TRUE    Setup Max_Payload_Size as per device capability
> > > +          FALSE   override as per device-specific platform policy
> > > +**/
> > > +BOOLEAN
> > > +SetupMpsAsPerDeviceCapability (
> > > +  IN  UINT8                   MPS
> > > +)
> > > +{
> > > +  if (MPS == EFI_PCI_CONF_MAX_PAYLOAD_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
> > > +  Revision 4.0; for the PCI feature Max_Payload_Size.
> > > +
> > > +  @param  MPS     Input device-specific policy should be in terms of type
> > > +                  EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > > +
> > > +  @retval         Range values for the Max_Payload_Size as defined in the PCI
> > > +                  Base Specification 4.0 **/
> > > +UINT8
> > > +TranslateMpsSetupValueToPci (
> > > +  IN  UINT8                   MPS
> > > +)
> > > +{
> > > +  switch (MPS) {
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_128B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_128B;
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_256B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_256B;
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_512B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_512B;
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_1024B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_1024B;
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_2048B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_2048B;
> > > +    case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_4096B:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_4096B;
> > > +    default:
> > > +      return PCIE_MAX_PAYLOAD_SIZE_128B;
> > > +  }
> > > +}
> > > +
> > >  /**
> > >    Generic routine to setup the PCI features as per its predetermined defaults.
> > >  **/
> > > @@ -364,6 +421,7 @@ SetupDefaultsDevicePlatformPolicy (
> > >    IN  PCI_IO_DEVICE               *PciDevice
> > >    )
> > >  {
> > > +  PciDevice->SetupMPS = EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO;
> > >  }
> > >
> > >  /**
> > > @@ -399,6 +457,7 @@ GetPciDevicePlatformPolicyEx (
> > >        //
> > >        // platform chipset policies are returned for this PCI device
> > >        //
> > > +      PciIoDevice->SetupMPS = PciPlatformExtendedPolicy.DeviceCtlMPS;
> > >
> > >        DEBUG ((
> > >            DEBUG_INFO, "[device policy: platform]"
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > > index a13131c..786c00d 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > > @@ -124,4 +124,36 @@ EFI_STATUS
> > >  GetPciDevicePlatformPolicy (
> > >    IN PCI_IO_DEVICE          *PciDevice
> > >    );
> > > +
> > > +/**
> > > +  Helper routine to indicate whether the given PCI device specific
> > > +policy value
> > > +  dictates to override the Max_Payload_Size to a particular value, or
> > > +set as per
> > > +  device capability.
> > > +
> > > +  @param  MPS     Input device-specific policy should be in terms of type
> > > +                  EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > > +
> > > +  @retval TRUE    Setup Max_Payload_Size as per device capability
> > > +          FALSE   override as per device-specific platform policy
> > > +**/
> > > +BOOLEAN
> > > +SetupMpsAsPerDeviceCapability (
> > > +  IN  UINT8                   MPS
> > > +);
> > > +
> > > +/**
> > > +  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
> > > +  Revision 4.0; for the PCI feature Max_Payload_Size.
> > > +
> > > +  @param  MPS     Input device-specific policy should be in terms of type
> > > +                  EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > > +
> > > +  @retval         Range values for the Max_Payload_Size as defined in the PCI
> > > +                  Base Specification 4.0 **/
> > > +UINT8
> > > +TranslateMpsSetupValueToPci (
> > > +  IN  UINT8                   MPS
> > > +);
> > >  #endif
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > >