Re: [edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser

["IanX Kuo" <ianx.kuo@intel.com>]

Hi Laszlo

Thanks for the remind. I will take care the date and Patch v4 in our next patch.

Thanks,
Ian Kuo
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, March 18, 2021 5:01 AM
To: devel@edk2.groups.io; Kuo, IanX <ianx.kuo@intel.com>
Cc: Ke, VincentX <vincentx.ke@intel.com>
Subject: Re: [edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser

Vincent, Ian:

something is seriously broken in your email setup. I have seen three messages from you guys on the list, and each one of those is "from the future". Here are the Date headers from the messages:

- Date: Thu,  8 Apr 2021 05:50:21 +0800
  https://edk2.groups.io/g/devel/message/72748

- Date: Sat, 10 Apr 2021 01:34:45 +0800
  https://edk2.groups.io/g/devel/message/72862

- Date: Sat, 10 Apr 2021 22:15:09 +0800
  https://edk2.groups.io/g/devel/message/72953

Please fix your clock setup, and post a new version.

This is why I am asking: the git-am manual says,

       --ignore-date
           By default the command records the date from the e-mail
           message as the commit author date, and uses the time of
           commit creation as the committer date. [...]

When we merge a patch using a github.com Pull Request, the "mergify bot"
preserves the Author Date field. That's a good thing in itself, but it means that the edk2 commit history would have a patch from the future -- a patch committed in March, but "authored" in April. That's bogus.

Whoever actually applies the patch (probably the ShellPkg maintainer) can work around the issue, by specifying the "--ignore-date" flag for "git-am". But that's easy to forget, and not the right thing anyway.

So please just fix your broken clock, and post a new version.

Also: you should have used v1, v2, v3 in the subject prefixes (just pass -v1, -v2, -v3 to git-format-patch). The next version that you post should be marked "v4".

Thanks
Laszlo


On 04/10/21 16:15, IanX Kuo wrote:
> From: VincentX Ke <vincentx.ke@intel.com>
> 
> Bugzilla: 3262 (https://bugzilla.tianocore.org/show_bug.cgi?id=3262)
> 
> No need to print PCIe details while CapabilityId is 0xFFFF.
> Limit the NextCapabilityOffset to PCI configuration space.
> 
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c 
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index a2f04d8db5..1e5dc75e27 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -2038,12 +2038,14 @@ LocatePciCapability (
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    );
>  
> @@ -2921,6 +2923,7 @@ ShellCommandRunPci (
>          PciExplainPciExpress (
>            (PCI_CAPABILITY_PCIEXP *) ((UINT8 *) &ConfigSpace + PcieCapabilityPtr),
>            ExtendedConfigSpace,
> +          ExtendedConfigSize,
>            ExtendedCapability
>            );
>        }
> @@ -5698,12 +5701,14 @@ PrintPciExtendedCapabilityDetails(
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    )
>  {
> @@ -5786,7 +5791,7 @@ PciExplainPciExpress (
>    }
>  
>    ExtHdr = (PCI_EXP_EXT_HDR*)ExtendedConfigSpace;
> -  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) 
> {
> +  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0 
> + && ExtHdr->CapabilityId != 0xFFFF) {
>      //
>      // Process this item
>      //
> @@ -5800,7 +5805,8 @@ PciExplainPciExpress (
>      //
>      // Advance to the next item if it exists
>      //
> -    if (ExtHdr->NextCapabilityOffset != 0) {
> +    if (ExtHdr->NextCapabilityOffset != 0 &&
> +       (ExtHdr->NextCapabilityOffset <= (UINT32) (ExtendedConfigSize 
> + + EFI_PCIE_CAPABILITY_BASE_OFFSET - sizeof (PCI_EXP_EXT_HDR)))) {
>        ExtHdr = (PCI_EXP_EXT_HDR*)(ExtendedConfigSpace + ExtHdr->NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
>      } else {
>        break;
>