From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web11.8194.1576658301384270187 for ; Wed, 18 Dec 2019 00:38:24 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Dec 2019 00:38:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,328,1571727600"; d="scan'208";a="218077816" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 18 Dec 2019 00:38:20 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 00:38:20 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 00:38:19 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.71]) with mapi id 14.03.0439.000; Wed, 18 Dec 2019 16:38:18 +0800 From: "Ni, Ray" To: "Javeed, Ashraf" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size Thread-Index: AQHVmdK5Hh4LyD94NUChTjCCT7Tp3Ke+eYzQ Date: Wed, 18 Dec 2019 08:38:17 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A189E@SHSMSX104.ccr.corp.intel.com> References: <20191101150952.3340-1-ashraf.javeed@intel.com> <15D3127D273722D4.32624@groups.io> <95C5C2B113DE604FB208120C742E9824579172AC@BGSMSX101.gar.corp.intel.com> In-Reply-To: <95C5C2B113DE604FB208120C742E9824579172AC@BGSMSX101.gar.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjQ5N2Q2MjAtOTI1My00YWQzLWE1YWQtMGJlZTUzYTQ1OTdkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiekZPM2NVVnRCZjE0aXh1R2dNenNaK094cVRUbit6aENWWWxoNnFLVFRtRVYwdUFVMjNIckt2bURpQzV4Y25XSSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > > + UINT8 SetupMPS; 1. Can it be "MaxPayloadSize"? > > + > > + if (PciConfigPhase =3D=3D PciFeatureGetDevicePolicy) { > > + if (SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS)) { 2. Can you replace " SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS" w= ith "PciDevice->MaxPayloadSize =3D=3D EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO"? This makes the code more readable.=20 > > + MpsValue =3D (UINT8)PciDeviceCap.Bits.MaxPayloadSize; > > + // > > + // no change to PCI Root ports without any endpoint device > > + // > > + if (IS_PCI_BRIDGE (&PciDevice->Pci) && PciDeviceCap.Bits.MaxPay= loadSize) > > { > > + if (IsPciRootPortEmpty (PciDevice)) { > > + MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; > > + } > > + } 3. Above two if-s can be simplified as below? and please also copy the spe= c requirements here as comments. if (IsListEmpty (&PciDevice->ChildList)) { MpsValue =3D PCIE_MAX_PAYLOAD_SIZE_128B; } > > + } else { > > + MpsValue =3D TranslateMpsSetupValueToPci (PciDevice->SetupMPS); 4. The function name can be "UefiToPciMaxPayloadSize()". And I suggest the value stored in PciDevice->SetupMPS (MaxPayloadSize) is the macro value defined in PciExpress21.h. We could do the conversion just after the GetDe= vicePolicy() call. > > + } > > + // > > + // discard device policy override request if greater than PCI dev= ice capability > > + // > > + PciDevice->SetupMPS =3D MIN ((UINT8)PciDeviceCap.Bits.MaxPayloadS= ize, > > + MpsValue); } > > + > > + // > > + // align the MPS of the tree to the HCF with this device // if > > + (PciFeaturesConfigurationTable) { > > + MpsValue =3D PciFeaturesConfigurationTable->Max_Payload_Size; 5. Max_Payload_Size can be "MaxPayloadSize". MpsValue can be "MaxPayloadSize". > > + > > + MpsValue =3D MIN (PciDevice->SetupMPS, MpsValue); > > + PciDevice->SetupMPS =3D MIN (PciDevice->SetupMPS, MpsValue); > > + > > + if (MpsValue !=3D PciFeaturesConfigurationTable->Max_Payload_Size= ) { > > + PciFeaturesConfigurationTable->Max_Payload_Size =3D MpsValue; > > + } > > + } 6. Can you simplify the above logic? > > + > > + DEBUG (( DEBUG_INFO, > > + "MPS: %d [DevCap:%d],", > > + PciDevice->SetupMPS, PciDeviceCap.Bits.MaxPayloadSize > > + )); > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + Overrides the PCI Device Control register MaxPayloadSize register > > +field; if > > + the hardware value is different than the intended value. > > + > > + @param PciDevice A pointer to the PCI_IO_DEVICE instan= ce. > > + > > + @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 heade= r of the PCI controller. > > + @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invalid. > > + > > +**/ > > +EFI_STATUS > > +OverrideMaxPayloadSize ( > > + IN PCI_IO_DEVICE *PciDevice > > + ) 7. Can this name be "ProgramMaxPayloadSize" because the function does the register programming? > > +{ > > + PCI_REG_PCIE_DEVICE_CONTROL PcieDev; > > + UINT32 Offset; > > + EFI_STATUS Status; > > + EFI_TPL OldTpl; > > + > > + PcieDev.Uint16 =3D 0; > > + Offset =3D PciDevice->PciExpressCapabilityOffset + > > + OFFSET_OF (PCI_CAPABILITY_PCIEXP, DeviceControl); > > + Status =3D PciDevice->PciIo.Pci.Read ( > > + &PciDevice->PciIo, > > + EfiPciIoWidthUint16, > > + Offset, > > + 1, > > + &PcieDev.Uint16 > > + ); 8. The PciExp is cached in PciExp field in the PciDevice structure. Why do= you need to read it from HW again? > > + if (EFI_ERROR(Status)){ > > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) r= ead > > error!", > > + Offset > > + )); > > + return Status; > > + } > > + if (PcieDev.Bits.MaxPayloadSize !=3D PciDevice->SetupMPS) { > > + PcieDev.Bits.MaxPayloadSize =3D PciDevice->SetupMPS; > > + DEBUG (( DEBUG_INFO, "MPS=3D%d,", PciDevice->SetupMPS)); > > + > > + // > > + // Raise TPL to high level to disable timer interrupt while the w= rite operation > > completes > > + // > > + OldTpl =3D gBS->RaiseTPL (TPL_HIGH_LEVEL); > > + > > + Status =3D 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 =3D PcieDev.Uint16= ; > > + } else { > > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x)= write > > error!", > > + Offset > > + )); 9. We can use ASSERT_EFI_ERROR() here. Failure of register writing is a fa= tal error. > > + } > > + } else { > > + DEBUG (( DEBUG_INFO, "No write of MPS=3D%d,", PciDevice->SetupMPS= )); 10. Can we skip this debug message? > > + } > > + > > + return Status; > > +} > > > > /** > > helper routine to dump the PCIe Device Port Type @@ -669,6 +809,18 = @@ > > SetupDevicePciFeatures ( > > } > > } > > > > + DEBUG ((DEBUG_INFO, "[")); > > + // > > + // process the PCI device Max_Payload_Size feature // if > > + (SetupMaxPayloadSize ()) { > > + Status =3D ProcessMaxPayloadSize ( > > + PciDevice, > > + PciConfigPhase, > > + OtherPciFeaturesConfigTable > > + ); 11. Can this function be "CalculatemaxPayloadSize"? Process is too general= . > > + } > > + DEBUG ((DEBUG_INFO, "]\n")); > > return Status; > > } > > > > @@ -765,6 +917,10 @@ ProgramDevicePciFeatures ( { > > EFI_STATUS Status =3D EFI_SUCCESS; > > > > + if (SetupMaxPayloadSize ()) { > > + Status =3D OverrideMaxPayloadSize (PciDevice); } DEBUG (( > > + DEBUG_INFO, "\n")); > > return Status; > > } > > > > @@ -878,6 +1034,7 @@ AddPrimaryRootPortNode ( > > ); > > if (PciConfigTable) { > > PciConfigTable->ID =3D PortNumber; > > + PciConfigTable->Max_Payload_Size =3D > > PCIE_MAX_PAYLOAD_SIZE_4096B; > > } > > RootPortNode->OtherPciFeaturesConfigurationTable =3D PciConfigTabl= e; > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > index f92d008..e5ac2a3 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h > > @@ -79,6 +79,11 @@ struct _OTHER_PCI_FEATURES_CONFIGURATION_TABLE > > { > > // Configuration Table ID > > // > > UINTN ID; > > + // > > + // to configure the PCI feature Maximum payload size to maintain th= e > > + data packet // size among all the PCI devices in the PCI hierarchy > > + // > > + UINT8 Max_Payload_Size; > > }; > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > index 238959e..99badd6 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c > > @@ -356,6 +356,63 @@ GetPlatformPciOptionRom ( > > return Status; > > } > > > > +/** > > + Helper routine to indicate whether the given PCI device specific > > +policy value > > + dictates to override the Max_Payload_Size to a particular value, or > > +set as per > > + device capability. > > + > > + @param MPS Input device-specific policy should be in terms of = type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > + > > + @retval TRUE Setup Max_Payload_Size as per device capability > > + FALSE override as per device-specific platform policy > > +**/ > > +BOOLEAN > > +SetupMpsAsPerDeviceCapability ( > > + IN UINT8 MPS > > +) > > +{ > > + if (MPS =3D=3D EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO) { > > + return TRUE; > > + } else { > > + return FALSE; > > + } > > +} > > + > > +/** > > + Routine to translate the given device-specific platform policy from > > +type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base > > +Specification > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > + > > + @param MPS Input device-specific policy should be in terms of = type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > + > > + @retval Range values for the Max_Payload_Size as defined in= the PCI > > + Base Specification 4.0 **/ > > +UINT8 > > +TranslateMpsSetupValueToPci ( > > + IN UINT8 MPS > > +) > > +{ > > + switch (MPS) { > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_128B: > > + return PCIE_MAX_PAYLOAD_SIZE_128B; > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_256B: > > + return PCIE_MAX_PAYLOAD_SIZE_256B; > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_512B: > > + return PCIE_MAX_PAYLOAD_SIZE_512B; > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_1024B: > > + return PCIE_MAX_PAYLOAD_SIZE_1024B; > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_2048B: > > + return PCIE_MAX_PAYLOAD_SIZE_2048B; > > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_4096B: > > + return PCIE_MAX_PAYLOAD_SIZE_4096B; > > + default: > > + return PCIE_MAX_PAYLOAD_SIZE_128B; > > + } > > +} > > + > > /** > > Generic routine to setup the PCI features as per its predetermined = defaults. > > **/ > > @@ -364,6 +421,7 @@ SetupDefaultsDevicePlatformPolicy ( > > IN PCI_IO_DEVICE *PciDevice > > ) > > { > > + PciDevice->SetupMPS =3D EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO; > > } > > > > /** > > @@ -399,6 +457,7 @@ GetPciDevicePlatformPolicyEx ( > > // > > // platform chipset policies are returned for this PCI device > > // > > + PciIoDevice->SetupMPS =3D PciPlatformExtendedPolicy.DeviceCtlMP= S; > > > > DEBUG (( > > DEBUG_INFO, "[device policy: platform]" > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > index a13131c..786c00d 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h > > @@ -124,4 +124,36 @@ EFI_STATUS > > GetPciDevicePlatformPolicy ( > > IN PCI_IO_DEVICE *PciDevice > > ); > > + > > +/** > > + Helper routine to indicate whether the given PCI device specific > > +policy value > > + dictates to override the Max_Payload_Size to a particular value, or > > +set as per > > + device capability. > > + > > + @param MPS Input device-specific policy should be in terms of = type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > + > > + @retval TRUE Setup Max_Payload_Size as per device capability > > + FALSE override as per device-specific platform policy > > +**/ > > +BOOLEAN > > +SetupMpsAsPerDeviceCapability ( > > + IN UINT8 MPS > > +); > > + > > +/** > > + Routine to translate the given device-specific platform policy from > > +type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base > > +Specification > > + Revision 4.0; for the PCI feature Max_Payload_Size. > > + > > + @param MPS Input device-specific policy should be in terms of = type > > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE > > + > > + @retval Range values for the Max_Payload_Size as defined in= the PCI > > + Base Specification 4.0 **/ > > +UINT8 > > +TranslateMpsSetupValueToPci ( > > + IN UINT8 MPS > > +); > > #endif > > -- > > 2.21.0.windows.1 > > > > > >=20