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

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

Ray,
As discussed, the ProcessMaxPayloadSize() gets the minimum payload size for all devices under a certain root port, at the end of PciFeatureSetupPhase.
The value from the PCI features configuration table is aligned to each of PciDevice->SetupMPS, under a root port. 
The PciDevice->SetupMPS is used for its corresponding device's MaxReadRequestSize feature conditionally, since there is no PCIe capability for this feature.
Thus, as per the implementation design formulated here, all the PciDevice->SetupXXX has to be finalized, considering the device policy, during the phases - PciFeatureGetDevicePolicy & PciFeatureSetupPhase. Finally, it shall be programmed during the phase PciFeatureConfigurationPhase.
The OverrideMaxPayloadSize() does not have to use the PCI features configuration table, as that table is temporarily meant for aligning a feature value among all devices of a root port.

Responding to your other comments below inline.
Thanks
Ashraf


> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, December 18, 2019 2:40 PM
> To: Javeed, Ashraf <ashraf.javeed@intel.com>; 'devel@edk2.groups.io'
> <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
> 
> 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"?
I thought prefixing the name with "Setup" can imply that this data member of PCI_IO_DEVICE would help to indicate it takes the device policy, transforms into the final raw value after calculation, which shall be programmed in to hardware control register. I have used same style for all other features, and I have to rename all those....I don't see any conflict in the name SetupMPS as we know that MPS is shorthand for the 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.
> >
I thought the routine name itself informally indicate what type of action it has to take based on *_AUTO EFI encoding....I shall reconsider this.

> > > > +      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; }
> >
OK

> >
> >
> > > > +    } 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.
> >
Ok, I can rename to the suggested name for the translation routine. But, I cannot use that immediately after the GetDevicePolicy() because some of the PCIe features are defined in the capability register and its *_AUTO can be used in co-relation to that; like in case of MaxPayloadSize feature; but in case of PCIe feature like RelaxOrdering & NoSnoop, the PCI Base Specification has not defined any corresponding capability or hardware reference value for look-up, thus *_AUTO for these kind of features would be to just ignore it.

> > > > +    }
> > > > +    //
> > > > +    // 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".
> >
OK

> > > > +
> > > > +    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?
> >
Will check on this

> > > > +
> > > > +  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?
> >
OK

> > > > +{
> > > > +  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?
> >
Just for the sake of defensive programming; in case platform has done some overrides in between; since numerous callbacks are going to platform code of PCI Host Bridge Resource Allocation Protocol driver,  and PCI Platform Protocol driver. 

> > > > +  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.
> >
OK

> > > > +    }
> > > > +  } else {
> > > > +    DEBUG (( DEBUG_INFO, "No write of MPS=%d,",
> > > > + PciDevice->SetupMPS));
> >
> > 10. Can we skip this debug message?
> >
OK

> > > > + }
> > > > +
> > > > +  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.
> >
OK

> > > > +  }
> > > > +  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
> > > >
> > > >
> > > >