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,
At the end of PciFeatureGetDevicePolicy, value from the PCI features configuration table is the minimum one.
I prefer to defer the finalize of PciDevice->SetupMPS in the OverrideMaxPayloadSize().
Through this, the 4 phases can be reduced to 3 phases.

Thanks,
Ray

> -----Original Message-----
> From: Javeed, Ashraf <ashraf.javeed@intel.com>
> Sent: Wednesday, December 18, 2019 10:36 PM
> To: Ni, Ray <ray.ni@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
> 
> 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
> > > > >
> > > > >
> > > > >