Good feedback. Comment below: > -----Original Message----- > From: Ni, Ray <ray...@intel.com> > Sent: Thursday, November 7, 2019 2:56 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com> > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Lou, Yun > <yun....@intel.com> > Subject: RE: [edk2-devel] [PATCH V2 5/6] > IntelSiliconPkg/SamplePlatformDevicePolicyDxe: Add sample policy. > > 1. I am sorry but I just realized GetDevicePolicy() returns a pointer of > policy to > caller, > instead of copying the policy data to caller's buffer. It's a bit strange > to me. > Can you > please change the prototype so that caller needs to pass in a policy > structure > and > GetDevicePolicy() fills the structure buffer using CopyMem from > mDeviceSecurityPolicyNone? > "*DeviceSecurityPolicy = &mDeviceSecurityPolicyNone;" > "DeviceSecurityPolicy = &mDeviceSecurityPolicyMeasurement;" [Jiewen] Agree. Will update in V3.
> > 2. I thought SetDeviceState() in your sample will reset the > EDKII_DEVICE_SECURITY_STATE_ERROR_UEFI_UNSUPPORTED > to 0. Why didn't the sample do that? [Jiewen] I rename the function to NotifyDeviceState() in V3. I think that is clear. Also the state parameter is IN. The caller should not change it. > > 3. I just realized you didn't define the version macro for > EDKII_DEVICE_SECURITY_POLICY_PROTOCOL.Version. > I suggest you define a macro for the version instead of let > producer/consumer > use hard-code number (0x1). > And I am not sure if you will use the same version macro for the > protocol.version, securitypolicy.version and > securitystate.version. I assume you will. No matter yes or no, can you > please > state that through comments > in the policy protocol header file clearly? [Jiewen] Agree. Will update in V3. > > > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, > > Jiewen > > Sent: Thursday, October 31, 2019 8:31 PM > > To: devel@edk2.groups.io > > Cc: Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V > > <rangasai.v.chaga...@intel.com>; Lou, Yun <yun....@intel.com> > > Subject: [edk2-devel] [PATCH V2 5/6] > > IntelSiliconPkg/SamplePlatformDevicePolicyDxe: Add sample policy. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2303 > > > > This driver provides the platform sample policy to measure a NVMe card. > > > > Cc: Ray Ni <ray...@intel.com> > > Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> > > Cc: Yun Lou <yun....@intel.com> > > Signed-off-by: Jiewen Yao <jiewen....@intel.com> > > --- > > > > Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDevicePoli > > cyDxe/SamplePlatformDevicePolicyDxe.c | 189 ++++++++++++++++++++ > > > > Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDevicePoli > > cyDxe/SamplePlatformDevicePolicyDxe.inf | 40 +++++ > > 2 files changed, 229 insertions(+) > > > > diff --git > > a/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDeviceP > > olicyDxe/SamplePlatformDevicePolicyDxe.c > > b/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDeviceP > > olicyDxe/SamplePlatformDevicePolicyDxe.c > > new file mode 100644 > > index 0000000000..1f01b961a8 > > --- /dev/null > > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformD > > +++ evicePolicyDxe/SamplePlatformDevicePolicyDxe.c > > @@ -0,0 +1,189 @@ > > +/** @file > > + EDKII Device Security library for PCI device > > + > > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include <Uefi.h> > > +#include <IndustryStandard/Spdm.h> > > +#include <IndustryStandard/Pci.h> > > +#include <Protocol/PciIo.h> > > +#include <Protocol/DeviceSecurity.h> > > +#include <Protocol/PlatformDeviceSecurityPolicy.h> > > +#include <Library/DebugLib.h> > > +#include <Library/BaseMemoryLib.h> > > +#include <Library/UefiBootServicesTableLib.h> > > +#include <Library/MemoryAllocationLib.h> > > + > > +EDKII_DEVICE_SECURITY_POLICY mDeviceSecurityPolicyNone = { > > + 0x1, > > + 0, > > + 0, > > +}; > > +EDKII_DEVICE_SECURITY_POLICY mDeviceSecurityPolicyMeasurement > > = { > > + 0x1, > > + EDKII_DEVICE_MEASUREMENT_POLICY_REQUIRED, > > + 0, > > +}; > > + > > +/** > > + This function returns the device security policy associated with the > > device. > > + > > + @param[in] This The protocol instance pointer. > > + @param[in] DeviceId The Identifier for the device. > > + @param[out] DeviceSecurityPolicy The Device Security Policy associated > > with the device. > > + > > + @retval EFI_SUCCESS The device security policy is returned > > +**/ > > +EFI_STATUS > > +EFIAPI > > +GetDevicePolicy ( > > + IN EDKII_DEVICE_SECURITY_POLICY_PROTOCOL *This, > > + IN EDKII_DEVICE_IDENTIFIER *DeviceId, > > + OUT EDKII_DEVICE_SECURITY_POLICY **DeviceSecurityPolicy > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_PCI_IO_PROTOCOL *PciIo; > > + UINT16 PciVendorId; > > + UINT16 PciDeviceId; > > + > > + *DeviceSecurityPolicy = &mDeviceSecurityPolicyNone; > > + > > + DEBUG ((DEBUG_INFO, "GetDevicePolicy - 0x%g\n", > > + &DeviceId->DeviceType)); > > + > > + if (!CompareGuid (&DeviceId->DeviceType, > > &gEdkiiDeviceIdentifierTypePciGuid)) { > > + return EFI_SUCCESS; > > + } > > + > > + Status = gBS->HandleProtocol ( > > + DeviceId->DeviceHandle, > > + &gEdkiiDeviceIdentifierTypePciGuid, > > + (VOID **)&PciIo > > + ); > > + if (EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_ERROR, "Locate - DeviceIdentifierTypePci - %r\n", > > Status)); > > + return EFI_SUCCESS; > > + } > > + > > + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, > > + PCI_VENDOR_ID_OFFSET, 1, &PciVendorId); ASSERT_EFI_ERROR(Status); > > + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, > > + PCI_DEVICE_ID_OFFSET, 1, &PciDeviceId); ASSERT_EFI_ERROR(Status); > > + DEBUG ((DEBUG_INFO, "PCI Info - %04x:%04x\n", PciVendorId, > > + PciDeviceId)); > > + > > + if ((PciVendorId == 0x8086) && (PciDeviceId == 0x0B60)) { > > + *DeviceSecurityPolicy = &mDeviceSecurityPolicyMeasurement; > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + This function sets the device state based upon the authentication result. > > + > > + @param[in] This The protocol instance pointer. > > + @param[in] DeviceId The Identifier for the device. > > + @param[in] DeviceSecurityState The Device Security state associated > > with the device. > > + > > + @retval EFI_SUCCESS The device state is set > > +**/ > > +EFI_STATUS > > +EFIAPI > > +SetDeviceState ( > > + IN EDKII_DEVICE_SECURITY_POLICY_PROTOCOL *This, > > + IN EDKII_DEVICE_IDENTIFIER *DeviceId, > > + IN EDKII_DEVICE_SECURITY_STATE *DeviceSecurityState > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_PCI_IO_PROTOCOL *PciIo; > > + UINT16 PciVendorId; > > + UINT16 PciDeviceId; > > + UINTN Segment; > > + UINTN Bus; > > + UINTN Device; > > + UINTN Function; > > + > > + DEBUG ((DEBUG_INFO, "SetDeviceState - 0x%g\n", > > + &DeviceId->DeviceType)); > > + > > + if (!CompareGuid (&DeviceId->DeviceType, > > &gEdkiiDeviceIdentifierTypePciGuid)) { > > + return EFI_SUCCESS; > > + } > > + > > + Status = gBS->HandleProtocol ( > > + DeviceId->DeviceHandle, > > + &gEdkiiDeviceIdentifierTypePciGuid, > > + (VOID **)&PciIo > > + ); > > + if (EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_ERROR, "Locate - DeviceIdentifierTypePci - %r\n", > > Status)); > > + return EFI_SUCCESS; > > + } > > + > > + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, > > + PCI_VENDOR_ID_OFFSET, 1, &PciVendorId); ASSERT_EFI_ERROR(Status); > > + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, > > + PCI_DEVICE_ID_OFFSET, 1, &PciDeviceId); ASSERT_EFI_ERROR(Status); > > + DEBUG ((DEBUG_INFO, "PCI Info - %04x:%04x\n", PciVendorId, > > + PciDeviceId)); > > + > > + Status = PciIo->GetLocation ( > > + PciIo, > > + &Segment, > > + &Bus, > > + &Device, > > + &Function > > + ); > > + if (!EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_INFO, "PCI Loc - %04x:%02x:%02x:%02x\n", > > + Segment, Bus, Device, Function)); } > > + > > + DEBUG ((DEBUG_INFO, "State - Measurement - 0x%08x, Authentication - > > 0x%08x\n", > > + DeviceSecurityState->MeasurementState, > > + DeviceSecurityState->AuthenticationState > > + )); > > + > > + return EFI_SUCCESS; > > +} > > + > > +EDKII_DEVICE_SECURITY_POLICY_PROTOCOL mDeviceSecurityPolicy = { > > + 0x1, > > + GetDevicePolicy, > > + SetDeviceState, > > +}; > > + > > +/** > > + Entrypoint of the device security driver. > > + > > + @param[in] ImageHandle ImageHandle of the loaded driver @param[in] > > + SystemTable Pointer to the System Table > > + > > + @retval EFI_SUCCESS The Protocol is installed. > > + @retval EFI_OUT_OF_RESOURCES Not enough resources available to > > initialize driver. > > + @retval EFI_DEVICE_ERROR A device error occurred attempting to > > initialize the driver. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +MainEntryPoint ( > > + IN EFI_HANDLE ImageHandle, > > + IN EFI_SYSTEM_TABLE *SystemTable > > + ) > > +{ > > + EFI_HANDLE Handle; > > + EFI_STATUS Status; > > + > > + Handle = NULL; > > + Status = gBS->InstallProtocolInterface ( > > + &Handle, > > + &gEdkiiDeviceSecurityPolicyProtocolGuid, > > + EFI_NATIVE_INTERFACE, > > + &mDeviceSecurityPolicy > > + ); > > + ASSERT_EFI_ERROR(Status); > > + > > + return Status; > > +} > > diff --git > > a/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDeviceP > > olicyDxe/SamplePlatformDevicePolicyDxe.inf > > b/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformDeviceP > > olicyDxe/SamplePlatformDevicePolicyDxe.inf > > new file mode 100644 > > index 0000000000..a9b77d8371 > > --- /dev/null > > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/PcieSecurity/SamplePlatformD > > +++ evicePolicyDxe/SamplePlatformDevicePolicyDxe.inf > > @@ -0,0 +1,40 @@ > > +## @file > > +# EDKII Device Security library for PCI device # # Copyright (c) 2019, > > +Intel Corporation. All rights reserved.<BR> # SPDX-License-Identifier: > > +BSD-2-Clause-Patent # ## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = SamplePlatformDevicePolicyDxe > > + FILE_GUID = 7EA7AACF-7ED3-4166-8271-B21156523620 > > + MODULE_TYPE = DXE_DRIVER > > + VERSION_STRING = 1.0 > > + ENTRY_POINT = MainEntryPoint > > + > > +[Sources] > > + SamplePlatformDevicePolicyDxe.c > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + IntelSiliconPkg/IntelSiliconPkg.dec > > + > > +[LibraryClasses] > > + UefiRuntimeServicesTableLib > > + UefiBootServicesTableLib > > + UefiDriverEntryPoint > > + MemoryAllocationLib > > + DevicePathLib > > + BaseMemoryLib > > + PrintLib > > + DebugLib > > + > > +[Protocols] > > + gEdkiiDeviceSecurityPolicyProtocolGuid ## PRODUCES > > + gEdkiiDeviceIdentifierTypePciGuid ## COMSUMES > > + > > +[Depex] > > + TRUE > > -- > > 2.19.2.windows.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50188): https://edk2.groups.io/g/devel/message/50188 Mute This Topic: https://groups.io/mt/40117827/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
