Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
>