[edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device exemption
Michael Kubacki
mikuback at linux.microsoft.com
Fri Aug 20 01:25:11 UTC 2021
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Monday, August 9, 2021 12:09 PM
>> To: devel at edk2.groups.io
>> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Liming Gao
>> <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>; Chris Ruffin <v-cruffin at microsoft.com>; Michael Kubacki
>> <michael.kubacki at microsoft.com>
>> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg/TestPointCheckLib: Add support for BME device
>> exemption
>>
>> From: Chris Ruffin <v-cruffin at 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 at intel.com>
>> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Chris Ruffin <v-cruffin at microsoft.com>
>> Co-authored-by: Michael Kubacki <michael.kubacki at microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki at 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 at edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney at intel.com]
>> -=-=-=-=-=-=
>>
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79607): https://edk2.groups.io/g/devel/message/79607
Mute This Topic: https://groups.io/mt/84776712/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list