public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption
@ 2021-08-09 19:08 Michael Kubacki
  2021-08-20  0:55 ` [edk2-devel] " Michael D Kinney
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Kubacki @ 2021-08-09 19:08 UTC (permalink / raw)
  To: devel
  Cc: Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong, Chris Ruffin,
	Michael Kubacki

From: Chris Ruffin <v-cruffin@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3541

Some platforms have devices which do not expose any additional
risk of DMA attacks but the BME bit cannot be disabled.

To allow MinPlatformPkg consumers to selectively exempt certain
devices from the PCI bus master test point, this change adds a
PCD to MinPlatformPkg.dec that allows those packages to specify
a list of PCI devices by S/B/D/F that should be excluded from
testing.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Chris Ruffin <v-cruffin@microsoft.com>
Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c            | 37 ++++++++++++++++++--
 Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c            | 35 ++++++++++++++++++
 Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec                                      |  4 +++
 Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf |  1 +
 Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf |  1 +
 5 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
index 514003944758..95f4fb8b7c7e 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
@@ -44,6 +44,13 @@ typedef struct {
   UINT32                        Data[48];
 } PCI_CONFIG_SPACE;
 
+typedef struct {
+  UINT8 Segment;
+  UINT8 Bus;
+  UINT8 Device;
+  UINT8 Function;
+} EXEMPT_DEVICE;
+
 #pragma pack()
 
 VOID
@@ -256,7 +263,7 @@ TestPointCheckPciResource (
   UINT16                            MinBus;
   UINT16                            MaxBus;
   BOOLEAN                           IsEnd;
-  
+
   DEBUG ((DEBUG_INFO, "==== TestPointCheckPciResource - Enter\n"));
   HandleBuf = NULL;
   Status = gBS->LocateHandleBuffer (
@@ -338,7 +345,7 @@ TestPointCheckPciResource (
                 // Device
                 DumpPciDevice ((UINT8)Bus, (UINT8)Device, (UINT8)Func, &PciData);
               }
-              
+
               //
               // If this is not a multi-function device, we can leave the loop
               // to deal with the next device.
@@ -360,7 +367,7 @@ TestPointCheckPciResource (
       }
     }
   }
-  
+
 Done:
   if (HandleBuf != NULL) {
     FreePool (HandleBuf);
@@ -396,6 +403,9 @@ TestPointCheckPciBusMaster (
   UINT8             HeaderType;
   EFI_STATUS        Status;
   PCI_SEGMENT_INFO  *PciSegmentInfo;
+  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
+  BOOLEAN           ExemptDeviceFound;
+  UINTN             Index;
 
   PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
   if (PciSegmentInfo == NULL) {
@@ -407,6 +417,27 @@ TestPointCheckPciBusMaster (
     for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
       for (Device = 0; Device <= 0x1F; Device++) {
         for (Function = 0; Function <= 0x7; Function++) {
+          //
+          // Some platforms have devices which do not expose any additional
+          // risk of DMA attacks but are not able to be turned off.  Allow
+          // the platform to define these devices and do not record errors
+          // for these devices.
+          //
+          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
+          ExemptDeviceFound = FALSE;
+          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
+            if (Segment == ExemptDevicePcdPtr[Index].Segment
+                && Bus == ExemptDevicePcdPtr[Index].Bus
+                && Device == ExemptDevicePcdPtr[Index].Device
+                && Function == ExemptDevicePcdPtr[Index].Function) {
+              ExemptDeviceFound = TRUE;
+            }
+          }
+
+          if (ExemptDeviceFound) {
+            continue;
+          }
+
           VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device, Function, PCI_VENDOR_ID_OFFSET));
           //
           // If VendorId = 0xffff, there does not exist a device at this
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
index 1061f8ac1c62..25c3caba6eed 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
@@ -14,6 +14,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/PciSegmentInfoLib.h>
 #include <IndustryStandard/Pci.h>
 
+#pragma pack(1)
+
+ typedef struct EXEMPT_DEVICE_STRUCT {
+  UINT8 Segment;
+  UINT8 Bus;
+  UINT8 Device;
+  UINT8 Function;
+} EXEMPT_DEVICE;
+
+#pragma pack()
+
 EFI_STATUS
 TestPointCheckPciBusMaster (
   VOID
@@ -29,6 +40,9 @@ TestPointCheckPciBusMaster (
   UINT8             HeaderType;
   EFI_STATUS        Status;
   PCI_SEGMENT_INFO  *PciSegmentInfo;
+  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
+  BOOLEAN           ExemptDeviceFound;
+  UINTN             Index;
 
   PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
   if (PciSegmentInfo == NULL) {
@@ -40,6 +54,27 @@ TestPointCheckPciBusMaster (
     for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
       for (Device = 0; Device <= 0x1F; Device++) {
         for (Function = 0; Function <= 0x7; Function++) {
+          //
+          // Some platforms have devices which do not expose any additional
+          // risk of DMA attacks but are not able to be turned off.  Allow
+          // the platform to define these devices and do not record errors
+          // for these devices.
+          //
+          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
+          ExemptDeviceFound = FALSE;
+          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
+            if (Segment == ExemptDevicePcdPtr[Index].Segment
+                && Bus == ExemptDevicePcdPtr[Index].Bus
+                && Device == ExemptDevicePcdPtr[Index].Device
+                && Function == ExemptDevicePcdPtr[Index].Function) {
+              ExemptDeviceFound = TRUE;
+            }
+          }
+
+          if (ExemptDeviceFound) {
+            continue;
+          }
+
           VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device, Function, PCI_VENDOR_ID_OFFSET));
           //
           // If VendorId = 0xffff, there does not exist a device at this
diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
index bcb42f0ef9e6..259038dde4df 100644
--- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
+++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
@@ -160,6 +160,10 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   #   Stage Advanced:                                             {0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
   gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature|{0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00100302
 
+  # The platform may define a list of devices that are exempt from PCI BME testing.
+  # PCD Format is {SegmentNumber1, BusNumber1, DeviceNumber1, FunctionNumber1, SegmentNumber2, BusNumber2, DeviceNumber2, FunctionNumber2, ...}
+  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme|{0}|VOID*|0x00100303
+
   ##
   ## The Flash relevant PCD are ineffective and will be patched basing on FDF definitions during build.
   ## Set all of them to 0 here to prevent from confusion.
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
index 2ae1db4ee483..15779eb9b6de 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
@@ -106,3 +106,4 @@ [Protocols]
 
 [Pcd]
   gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
+  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
index 51369fcedc1e..ea6dc6b8ba34 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
@@ -47,6 +47,7 @@ [Sources]
 
 [Pcd]
   gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
+  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
 
 [Guids]
   gEfiHobMemoryAllocStackGuid
-- 
2.28.0.windows.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption
  2021-08-09 19:08 [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption Michael Kubacki
@ 2021-08-20  0:55 ` Michael D Kinney
  2021-08-20  1:25   ` Michael Kubacki
  0 siblings, 1 reply; 3+ messages in thread
From: Michael D Kinney @ 2021-08-20  0:55 UTC (permalink / raw)
  To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
	Kinney, Michael D
  Cc: Chiu, Chasel, Desimone, Nathaniel L, Liming Gao, Dong, Eric,
	Chris Ruffin, Michael Kubacki

Hi Michael,

Using S/B/D/F is not a good way to provide this information.  

It can change based when other PCI devices are enabled/disabled/added/removed.

It is better to use a list of D/F similar to PCI Device Paths or just
use a PCI device path.

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Monday, August 9, 2021 12:09 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Chris Ruffin <v-cruffin@microsoft.com>; Michael Kubacki
> <michael.kubacki@microsoft.com>
> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device
> exemption
> 
> From: Chris Ruffin <v-cruffin@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3541
> 
> Some platforms have devices which do not expose any additional
> risk of DMA attacks but the BME bit cannot be disabled.
> 
> To allow MinPlatformPkg consumers to selectively exempt certain
> devices from the PCI bus master test point, this change adds a
> PCD to MinPlatformPkg.dec that allows those packages to specify
> a list of PCI devices by S/B/D/F that should be excluded from
> testing.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Chris Ruffin <v-cruffin@microsoft.com>
> Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c            | 37 ++++++++++++++++++--
>  Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c            | 35 ++++++++++++++++++
>  Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec                                      |  4 +++
>  Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf |  1 +
>  Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf |  1 +
>  5 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
> index 514003944758..95f4fb8b7c7e 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
> @@ -44,6 +44,13 @@ typedef struct {
>    UINT32                        Data[48];
>  } PCI_CONFIG_SPACE;
> 
> +typedef struct {
> +  UINT8 Segment;
> +  UINT8 Bus;
> +  UINT8 Device;
> +  UINT8 Function;
> +} EXEMPT_DEVICE;
> +
>  #pragma pack()
> 
>  VOID
> @@ -256,7 +263,7 @@ TestPointCheckPciResource (
>    UINT16                            MinBus;
>    UINT16                            MaxBus;
>    BOOLEAN                           IsEnd;
> -
> +
>    DEBUG ((DEBUG_INFO, "==== TestPointCheckPciResource - Enter\n"));
>    HandleBuf = NULL;
>    Status = gBS->LocateHandleBuffer (
> @@ -338,7 +345,7 @@ TestPointCheckPciResource (
>                  // Device
>                  DumpPciDevice ((UINT8)Bus, (UINT8)Device, (UINT8)Func, &PciData);
>                }
> -
> +
>                //
>                // If this is not a multi-function device, we can leave the loop
>                // to deal with the next device.
> @@ -360,7 +367,7 @@ TestPointCheckPciResource (
>        }
>      }
>    }
> -
> +
>  Done:
>    if (HandleBuf != NULL) {
>      FreePool (HandleBuf);
> @@ -396,6 +403,9 @@ TestPointCheckPciBusMaster (
>    UINT8             HeaderType;
>    EFI_STATUS        Status;
>    PCI_SEGMENT_INFO  *PciSegmentInfo;
> +  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
> +  BOOLEAN           ExemptDeviceFound;
> +  UINTN             Index;
> 
>    PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
>    if (PciSegmentInfo == NULL) {
> @@ -407,6 +417,27 @@ TestPointCheckPciBusMaster (
>      for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
>        for (Device = 0; Device <= 0x1F; Device++) {
>          for (Function = 0; Function <= 0x7; Function++) {
> +          //
> +          // Some platforms have devices which do not expose any additional
> +          // risk of DMA attacks but are not able to be turned off.  Allow
> +          // the platform to define these devices and do not record errors
> +          // for these devices.
> +          //
> +          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
> +          ExemptDeviceFound = FALSE;
> +          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
> +            if (Segment == ExemptDevicePcdPtr[Index].Segment
> +                && Bus == ExemptDevicePcdPtr[Index].Bus
> +                && Device == ExemptDevicePcdPtr[Index].Device
> +                && Function == ExemptDevicePcdPtr[Index].Function) {
> +              ExemptDeviceFound = TRUE;
> +            }
> +          }
> +
> +          if (ExemptDeviceFound) {
> +            continue;
> +          }
> +
>            VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device,
> Function, PCI_VENDOR_ID_OFFSET));
>            //
>            // If VendorId = 0xffff, there does not exist a device at this
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
> index 1061f8ac1c62..25c3caba6eed 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
> @@ -14,6 +14,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/PciSegmentInfoLib.h>
>  #include <IndustryStandard/Pci.h>
> 
> +#pragma pack(1)
> +
> + typedef struct EXEMPT_DEVICE_STRUCT {
> +  UINT8 Segment;
> +  UINT8 Bus;
> +  UINT8 Device;
> +  UINT8 Function;
> +} EXEMPT_DEVICE;
> +
> +#pragma pack()
> +
>  EFI_STATUS
>  TestPointCheckPciBusMaster (
>    VOID
> @@ -29,6 +40,9 @@ TestPointCheckPciBusMaster (
>    UINT8             HeaderType;
>    EFI_STATUS        Status;
>    PCI_SEGMENT_INFO  *PciSegmentInfo;
> +  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
> +  BOOLEAN           ExemptDeviceFound;
> +  UINTN             Index;
> 
>    PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
>    if (PciSegmentInfo == NULL) {
> @@ -40,6 +54,27 @@ TestPointCheckPciBusMaster (
>      for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
>        for (Device = 0; Device <= 0x1F; Device++) {
>          for (Function = 0; Function <= 0x7; Function++) {
> +          //
> +          // Some platforms have devices which do not expose any additional
> +          // risk of DMA attacks but are not able to be turned off.  Allow
> +          // the platform to define these devices and do not record errors
> +          // for these devices.
> +          //
> +          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
> +          ExemptDeviceFound = FALSE;
> +          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
> +            if (Segment == ExemptDevicePcdPtr[Index].Segment
> +                && Bus == ExemptDevicePcdPtr[Index].Bus
> +                && Device == ExemptDevicePcdPtr[Index].Device
> +                && Function == ExemptDevicePcdPtr[Index].Function) {
> +              ExemptDeviceFound = TRUE;
> +            }
> +          }
> +
> +          if (ExemptDeviceFound) {
> +            continue;
> +          }
> +
>            VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device,
> Function, PCI_VENDOR_ID_OFFSET));
>            //
>            // If VendorId = 0xffff, there does not exist a device at this
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index bcb42f0ef9e6..259038dde4df 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -160,6 +160,10 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    #   Stage Advanced:                                             {0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
>    gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature|{0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00100302
> 
> +  # The platform may define a list of devices that are exempt from PCI BME testing.
> +  # PCD Format is {SegmentNumber1, BusNumber1, DeviceNumber1, FunctionNumber1, SegmentNumber2, BusNumber2, DeviceNumber2,
> FunctionNumber2, ...}
> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme|{0}|VOID*|0x00100303
> +
>    ##
>    ## The Flash relevant PCD are ineffective and will be patched basing on FDF definitions during build.
>    ## Set all of them to 0 here to prevent from confusion.
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
> index 2ae1db4ee483..15779eb9b6de 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
> @@ -106,3 +106,4 @@ [Protocols]
> 
>  [Pcd]
>    gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
> index 51369fcedc1e..ea6dc6b8ba34 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
> @@ -47,6 +47,7 @@ [Sources]
> 
>  [Pcd]
>    gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
> 
>  [Guids]
>    gEfiHobMemoryAllocStackGuid
> --
> 2.28.0.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#78985): https://edk2.groups.io/g/devel/message/78985
> Mute This Topic: https://groups.io/mt/84776712/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption
  2021-08-20  0:55 ` [edk2-devel] " Michael D Kinney
@ 2021-08-20  1:25   ` Michael Kubacki
  0 siblings, 0 replies; 3+ messages in thread
From: Michael Kubacki @ 2021-08-20  1:25 UTC (permalink / raw)
  To: devel, michael.d.kinney
  Cc: Chiu, Chasel, Desimone, Nathaniel L, Liming Gao, Dong, Eric,
	Chris Ruffin, Michael Kubacki

That's a good point. We were mostly considering this for on-chip PCI 
devices. I will rework this in v2.

Thanks,
Michael

On 8/19/2021 8:55 PM, Michael D Kinney wrote:
> Hi Michael,
> 
> Using S/B/D/F is not a good way to provide this information.
> 
> It can change based when other PCI devices are enabled/disabled/added/removed.
> 
> It is better to use a list of D/F similar to PCI Device Paths or just
> use a PCI device path.
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Monday, August 9, 2021 12:09 PM
>> To: devel@edk2.groups.io
>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Chris Ruffin <v-cruffin@microsoft.com>; Michael Kubacki
>> <michael.kubacki@microsoft.com>
>> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device
>> exemption
>>
>> From: Chris Ruffin <v-cruffin@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3541
>>
>> Some platforms have devices which do not expose any additional
>> risk of DMA attacks but the BME bit cannot be disabled.
>>
>> To allow MinPlatformPkg consumers to selectively exempt certain
>> devices from the PCI bus master test point, this change adds a
>> PCD to MinPlatformPkg.dec that allows those packages to specify
>> a list of PCI devices by S/B/D/F that should be excluded from
>> testing.
>>
>> Cc: Chasel Chiu <chasel.chiu@intel.com>
>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Chris Ruffin <v-cruffin@microsoft.com>
>> Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>   Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c            | 37 ++++++++++++++++++--
>>   Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c            | 35 ++++++++++++++++++
>>   Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec                                      |  4 +++
>>   Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf |  1 +
>>   Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf |  1 +
>>   5 files changed, 75 insertions(+), 3 deletions(-)
>>
>> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
>> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
>> index 514003944758..95f4fb8b7c7e 100644
>> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
>> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckPci.c
>> @@ -44,6 +44,13 @@ typedef struct {
>>     UINT32                        Data[48];
>>   } PCI_CONFIG_SPACE;
>>
>> +typedef struct {
>> +  UINT8 Segment;
>> +  UINT8 Bus;
>> +  UINT8 Device;
>> +  UINT8 Function;
>> +} EXEMPT_DEVICE;
>> +
>>   #pragma pack()
>>
>>   VOID
>> @@ -256,7 +263,7 @@ TestPointCheckPciResource (
>>     UINT16                            MinBus;
>>     UINT16                            MaxBus;
>>     BOOLEAN                           IsEnd;
>> -
>> +
>>     DEBUG ((DEBUG_INFO, "==== TestPointCheckPciResource - Enter\n"));
>>     HandleBuf = NULL;
>>     Status = gBS->LocateHandleBuffer (
>> @@ -338,7 +345,7 @@ TestPointCheckPciResource (
>>                   // Device
>>                   DumpPciDevice ((UINT8)Bus, (UINT8)Device, (UINT8)Func, &PciData);
>>                 }
>> -
>> +
>>                 //
>>                 // If this is not a multi-function device, we can leave the loop
>>                 // to deal with the next device.
>> @@ -360,7 +367,7 @@ TestPointCheckPciResource (
>>         }
>>       }
>>     }
>> -
>> +
>>   Done:
>>     if (HandleBuf != NULL) {
>>       FreePool (HandleBuf);
>> @@ -396,6 +403,9 @@ TestPointCheckPciBusMaster (
>>     UINT8             HeaderType;
>>     EFI_STATUS        Status;
>>     PCI_SEGMENT_INFO  *PciSegmentInfo;
>> +  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
>> +  BOOLEAN           ExemptDeviceFound;
>> +  UINTN             Index;
>>
>>     PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
>>     if (PciSegmentInfo == NULL) {
>> @@ -407,6 +417,27 @@ TestPointCheckPciBusMaster (
>>       for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
>>         for (Device = 0; Device <= 0x1F; Device++) {
>>           for (Function = 0; Function <= 0x7; Function++) {
>> +          //
>> +          // Some platforms have devices which do not expose any additional
>> +          // risk of DMA attacks but are not able to be turned off.  Allow
>> +          // the platform to define these devices and do not record errors
>> +          // for these devices.
>> +          //
>> +          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
>> +          ExemptDeviceFound = FALSE;
>> +          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
>> +            if (Segment == ExemptDevicePcdPtr[Index].Segment
>> +                && Bus == ExemptDevicePcdPtr[Index].Bus
>> +                && Device == ExemptDevicePcdPtr[Index].Device
>> +                && Function == ExemptDevicePcdPtr[Index].Function) {
>> +              ExemptDeviceFound = TRUE;
>> +            }
>> +          }
>> +
>> +          if (ExemptDeviceFound) {
>> +            continue;
>> +          }
>> +
>>             VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device,
>> Function, PCI_VENDOR_ID_OFFSET));
>>             //
>>             // If VendorId = 0xffff, there does not exist a device at this
>> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
>> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
>> index 1061f8ac1c62..25c3caba6eed 100644
>> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
>> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckPci.c
>> @@ -14,6 +14,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>   #include <Library/PciSegmentInfoLib.h>
>>   #include <IndustryStandard/Pci.h>
>>
>> +#pragma pack(1)
>> +
>> + typedef struct EXEMPT_DEVICE_STRUCT {
>> +  UINT8 Segment;
>> +  UINT8 Bus;
>> +  UINT8 Device;
>> +  UINT8 Function;
>> +} EXEMPT_DEVICE;
>> +
>> +#pragma pack()
>> +
>>   EFI_STATUS
>>   TestPointCheckPciBusMaster (
>>     VOID
>> @@ -29,6 +40,9 @@ TestPointCheckPciBusMaster (
>>     UINT8             HeaderType;
>>     EFI_STATUS        Status;
>>     PCI_SEGMENT_INFO  *PciSegmentInfo;
>> +  EXEMPT_DEVICE     *ExemptDevicePcdPtr;
>> +  BOOLEAN           ExemptDeviceFound;
>> +  UINTN             Index;
>>
>>     PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);
>>     if (PciSegmentInfo == NULL) {
>> @@ -40,6 +54,27 @@ TestPointCheckPciBusMaster (
>>       for (Bus = PciSegmentInfo[Segment].StartBusNumber; Bus <= PciSegmentInfo[Segment].EndBusNumber; Bus++) {
>>         for (Device = 0; Device <= 0x1F; Device++) {
>>           for (Function = 0; Function <= 0x7; Function++) {
>> +          //
>> +          // Some platforms have devices which do not expose any additional
>> +          // risk of DMA attacks but are not able to be turned off.  Allow
>> +          // the platform to define these devices and do not record errors
>> +          // for these devices.
>> +          //
>> +          ExemptDevicePcdPtr = (EXEMPT_DEVICE *) PcdGetPtr (PcdTestPointIbvPlatformExemptPciBme);
>> +          ExemptDeviceFound = FALSE;
>> +          for (Index = 0; Index < (PcdGetSize (PcdTestPointIbvPlatformExemptPciBme) / sizeof (EXEMPT_DEVICE)); Index++) {
>> +            if (Segment == ExemptDevicePcdPtr[Index].Segment
>> +                && Bus == ExemptDevicePcdPtr[Index].Bus
>> +                && Device == ExemptDevicePcdPtr[Index].Device
>> +                && Function == ExemptDevicePcdPtr[Index].Function) {
>> +              ExemptDeviceFound = TRUE;
>> +            }
>> +          }
>> +
>> +          if (ExemptDeviceFound) {
>> +            continue;
>> +          }
>> +
>>             VendorId = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(PciSegmentInfo[Segment].SegmentNumber, Bus, Device,
>> Function, PCI_VENDOR_ID_OFFSET));
>>             //
>>             // If VendorId = 0xffff, there does not exist a device at this
>> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
>> index bcb42f0ef9e6..259038dde4df 100644
>> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
>> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
>> @@ -160,6 +160,10 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>     #   Stage Advanced:                                             {0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03,
>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
>>     gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature|{0x03, 0x0F, 0x03, 0x1D, 0x3F, 0x0F, 0x0F, 0x07, 0x03,
>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00100302
>>
>> +  # The platform may define a list of devices that are exempt from PCI BME testing.
>> +  # PCD Format is {SegmentNumber1, BusNumber1, DeviceNumber1, FunctionNumber1, SegmentNumber2, BusNumber2, DeviceNumber2,
>> FunctionNumber2, ...}
>> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme|{0}|VOID*|0x00100303
>> +
>>     ##
>>     ## The Flash relevant PCD are ineffective and will be patched basing on FDF definitions during build.
>>     ## Set all of them to 0 here to prevent from confusion.
>> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
>> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
>> index 2ae1db4ee483..15779eb9b6de 100644
>> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
>> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
>> @@ -106,3 +106,4 @@ [Protocols]
>>
>>   [Pcd]
>>     gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
>> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
>> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
>> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
>> index 51369fcedc1e..ea6dc6b8ba34 100644
>> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
>> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
>> @@ -47,6 +47,7 @@ [Sources]
>>
>>   [Pcd]
>>     gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature
>> +  gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformExemptPciBme
>>
>>   [Guids]
>>     gEfiHobMemoryAllocStackGuid
>> --
>> 2.28.0.windows.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#78985): https://edk2.groups.io/g/devel/message/78985
>> Mute This Topic: https://groups.io/mt/84776712/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-20  1:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-09 19:08 [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption Michael Kubacki
2021-08-20  0:55 ` [edk2-devel] " Michael D Kinney
2021-08-20  1:25   ` Michael Kubacki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox