Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 11/12] PciBusDxe: New PCI feature No-Snoop

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

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

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 11/12]
> PciBusDxe: New PCI feature No-Snoop
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2313
> 
> The code changes are made; as per the PCI Base Specification 4 Revision 1; to
> enable the configuration of new PCI feature No-Snoop (NS), which enables the
> PCI function to initiate requests if it does not require har- dware enforced
> cache-coherency for its transactions.
> 
> The code changes are made to configure only those PCI devices which are
> requested to override by platform through the new PCI Platform protocol
> interface for device-specific policies.
> 
> 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  | 78
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c | 45
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 9f017b7..be1c341 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -293,6 +293,7 @@ struct _PCI_IO_DEVICE {
>    UINT8                                     SetupMPS;
>    UINT8                                     SetupMRRS;
>    PCI_FEATURE_POLICY                        SetupRO;
> +  PCI_FEATURE_POLICY                        SetupNS;
>  };
> 
>  #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 a60cb42..a7f0a2f 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> @@ -986,6 +986,81 @@ OverrideRelaxOrder (
>    return Status;
>  }
> 
> +/**
> +  Overrides the PCI Device Control register No-Snoop 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
> +OverrideNoSnoop (
> +  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 (PciDevice->SetupRO.Override
> +      &&  PcieDev.Bits.NoSnoop != PciDevice->SetupNS.Act
> +      ) {
> +    PcieDev.Bits.NoSnoop = PciDevice->SetupNS.Act;
> +    DEBUG (( DEBUG_INFO, "NS=%d", PciDevice->SetupNS.Act));
> +
> +    //
> +    // 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 NS,", PciDevice->SetupRO.Act));
> + }
> +
> +  return Status;
> +}
> +
>  /**
>    helper routine to dump the PCIe Device Port Type  **/ @@ -1200,6 +1275,9
> @@ ProgramDevicePciFeatures (
>    if (SetupRelaxOrder ()) {
>      Status = OverrideRelaxOrder (PciDevice);
>    }
> +  if (SetupNoSnoop ()) {
> +    Status = OverrideNoSnoop (PciDevice);  }
>    DEBUG (( DEBUG_INFO, "\n"));
>    return Status;
>  }
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> index f1e7039..47295cd 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> @@ -509,6 +509,46 @@ SetDevicePolicyRelaxOrder (
>    }
>  }
> 
> +/**
> +  Routine to set the device-specific policy for the PCI feature
> +No-Snoop enable
> +  or disable
> +
> +  @param  NoSnoop       value corresponding to data type
> EFI_PCI_CONF_NO_SNOOP
> +  @param  PciDevice     A pointer to PCI_IO_DEVICE
> +**/
> +VOID
> +SetDevicePolicyNoSnoop (
> +  IN  EFI_PCI_CONF_NO_SNOOP       NoSnoop,
> +  OUT PCI_IO_DEVICE               *PciDevice
> +  )
> +{
> +  //
> +  // implementation specific rules for the usage of PCI_FEATURE_POLICY
> +members
> +  // exclusively for the PCI Feature No-Snoop
> +  //
> +  // .Override = 0 to skip this PCI feature No-Snoop for the PCI device
> +  // .Override = 1 to program this No-Snoop PCI feature
> +  //      .Act = 1 to enable the No-Snoop in the PCI device
> +  //      .Act = 0 to disable the No-Snoop in the PCI device
> +  //
> +  switch (NoSnoop) {
> +    case  EFI_PCI_CONF_NS_AUTO:
> +      PciDevice->SetupNS.Override = 0;
> +      break;
> +    case  EFI_PCI_CONF_NS_DISABLE:
> +      PciDevice->SetupNS.Override = 1;
> +      PciDevice->SetupNS.Act = 0;
> +      break;
> +    case  EFI_PCI_CONF_NS_ENABLE:
> +      PciDevice->SetupNS.Override = 1;
> +      PciDevice->SetupNS.Act = 1;
> +      break;
> +    default:
> +      PciDevice->SetupNS.Override = 0;
> +      break;
> +  }
> +}
> +
>  /**
>    Generic routine to setup the PCI features as per its predetermined defaults.
>  **/
> @@ -520,6 +560,7 @@ SetupDefaultsDevicePlatformPolicy (
>    PciDevice->SetupMPS = EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO;
>    PciDevice->SetupMRRS = EFI_PCI_CONF_MAX_READ_REQ_SIZE_AUTO;
>    PciDevice->SetupRO.Override = 0;
> +  PciDevice->SetupNS.Override = 0;
>  }
> 
>  /**
> @@ -561,6 +602,10 @@ GetPciDevicePlatformPolicyEx (
>        // set device specific policy for Relax Ordering
>        //
>        SetDevicePolicyRelaxOrder
> (PciPlatformExtendedPolicy.DeviceCtlRelaxOrder, PciIoDevice);
> +      //
> +      // set the device specific policy for No-Snoop
> +      //
> +      SetDevicePolicyNoSnoop
> + (PciPlatformExtendedPolicy.DeviceCtlNoSnoop, PciIoDevice);
> 
>        DEBUG ((
>            DEBUG_INFO, "[device policy: platform]"
> --
> 2.21.0.windows.1
> 
> 
>