Re: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block

["He, Jiangang" <Jiangang.He@amd.com>]

[AMD Official Use Only - General]

UsbHcFreeMemBlock()->IoMmuFreeBuffer()->mIoMmu->FreeBuffer(), which may end up calling PeiFreePages() depending on gEdkiiIoMmuPpiGuid implementation. Surely both will work since UsbHcFreeMemPool() can't be interrupted by any service call to use the memory just freed. Just for good coding practice reason, I pick the one aligning with XhciDxe.

Thanks,
Jiangang
-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Monday, December 19, 2022 12:40 AM
To: He, Jiangang <Jiangang.He@amd.com>; Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io
Cc: Lin, Kuei-Hung (Timothy) <Kuei-Hung.Lin@amd.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Sun, Zhikai <zhikai.sun@intel.com>; Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block

[AMD Official Use Only - General]

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Hello,

My take is that unlike in DXE, the UsbHcFreeMemBlock() implementation in PEI phase does not perform freeing the memory.

So I think both the solution:
* Provided at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01%7CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638070288126779782%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uJMNyYXMU22UZNjrKtDetyD2BqrvkualuorPZHAV3wg%3D&reserved=0, which aligns with EhciPei
* Mentioned at the end of your previous reply, which aligns with XhciDxe should work fine.

I will leave it to you for the final decision.

Best Regards,
Hao Wu

> -----Original Message-----
> From: He, Jiangang <Jiangang.He@amd.com>
> Sent: Friday, December 16, 2022 12:48 AM
> To: Chang, Abner <Abner.Chang@amd.com>; Wu, Hao A
> <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) <Kuei-Hung.Lin@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Sun, Zhikai
> <zhikai.sun@intel.com>; Kirkendall, Garrett
> <Garrett.Kirkendall@amd.com>
> Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> block
>
> [AMD Official Use Only - General]
>
> Yes, it is the same issue discussed in
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2
> .groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01%7CJiang
> ang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3dd8961fe4884e60
> 8e11a82d994e183d%7C0%7C0%7C638070288126936018%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7C%7C%7C&sdata=58j41QJxKbrQenhyZKYO4dxYj3Sat2kJejQGioZhtu4%3D&rese
> rved=0
>
> MdeModulePkg\Bus\Pci\XhciPei\UsbHcMem.c
>
>   for (Block = Pool->Head->Next; Block != NULL; Block = Pool->Head->Next) {
>     // UsbHcUnlinkMemBlock (Pool->Head, Block);
>     UsbHcFreeMemBlock (Pool, Block);
>   }
> Block = Pool->Head->Next never change without calling
> UsbHcUnlinkMemBlock (Pool->Head, Block), therefore dead loop.
>
> Our proposed fix came from dxe version of the equivalent file
> MdeModulePkg\Bus\Pci\XhciDxe\UsbHcMem.c but swapped two routine call
> order (Now I think it is incorrect as clarified below).
>   for (Block = Pool->Head->Next; Block != NULL; Block = Pool->Head->Next) {
>     UsbHcFreeMemBlock (Pool, Block);
>     UsbHcUnlinkMemBlock (Pool->Head, Block);
>   }
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01%7CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638070288126936018%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=58j41QJxKbrQenhyZKYO4dxYj3Sat2kJejQGioZhtu4%3D&reserved=0 proposed fix:
>
>   for (Block = Pool->Head->Next; Block != NULL; Block = Block ->Next) {
>     // UsbHcUnlinkMemBlock (Pool->Head, Block);
>     UsbHcFreeMemBlock (Pool, Block);
>   }
>
> I think it again, both proposals have problem of reading memory
> content in the buffer that has just been freed.
>
>   for (Block = Pool->Head->Next; Block != NULL; Block = Pool->Head->Next) {
>      UsbHcUnlinkMemBlock (Pool->Head, Block);
>     UsbHcFreeMemBlock (Pool, Block);
>   }
> is right solution and matches dxe version of UsbHcMem.c.
>
> Thanks,
> Jiangang
>
> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com>
> Sent: Wednesday, December 14, 2022 8:12 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) <Kuei-Hung.Lin@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Sun, Zhikai
> <zhikai.sun@intel.com>; Kirkendall, Garrett
> <Garrett.Kirkendall@amd.com>; He, Jiangang <Jiangang.He@amd.com>
> Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> block
>
> [AMD Official Use Only - General]
>
> Hi Jiangang,
> Could you please provide the context of this patch?
>
> Thanks
> Abner
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Monday, December 12, 2022 11:27 AM
> > To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) <Kuei-Hung.Lin@amd.com>; Ni, Ray
> > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Sun, Zhikai
> > <zhikai.sun@intel.com>; Kirkendall, Garrett
> > <Garrett.Kirkendall@amd.com>
> > Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei
> > memory block
> >
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > Sorry for a question, may I know what issue was met that leads to
> > the proposed patch?
> > Could you help to check if it is related with the topic discussed in
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2
> > .gr
> >
> oups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&amp;data=05%7C0
> 1%7
> >
> Cabner.chang%40amd.com%7Ccac49c2820e741b8c48a08dadbf0cc24%7C3d
> d8
> >
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C638064124512265992%7C
> Unk
> >
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1h
> >
> aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=z1Q7NRxN4GMA
> %2
> > FBxYd2D7Gnkc3aTD23mRnwNF3H5wE0k%3D&amp;reserved=0? Thanks in
> advance.
> >
> > Best Regards,
> > Hao Wu
> >
> > > -----Original Message-----
> > > From: abner.chang@amd.com <abner.chang@amd.com>
> > > Sent: Saturday, December 10, 2022 11:13 PM
> > > To: devel@edk2.groups.io
> > > Cc: Kuei-Hung.Lin@amd.com; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Garrett Kirkendall
> > > <garrett.kirkendall@amd.com>; Abner Chang <abner.chang@amd.com>
> > > Subject: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> > > block
> > >
> > > From: Abner Chang <abner.chang@amd.com>
> > >
> > > In V2: Add AMD copyright.
> > >
> > > Unlink the XhciPei memory block when it has been freed.
> > >
> > > Signed-off-by: Kuei-Hung Lin <Kuei-Hung.Lin@amd.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Garrett Kirkendall <garrett.kirkendall@amd.com>
> > > Cc: Abner Chang <abner.chang@amd.com>
> > > ---
> > >  MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c | 29
> > > ++++++++++++++++++++++++-
> > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c
> > > b/MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c
> > > index c64b38fcfc8..39ba31b0913 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c
> > > @@ -3,6 +3,7 @@ PEIM to produce gPeiUsb2HostControllerPpiGuid
> based
> > > on gPeiUsbControllerPpiGuid  which is used to enable recovery
> > > function from USB Drivers.
> > >
> > >  Copyright (c) 2014 - 2016, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
> > > +reserved.<BR>
> > >
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > @@ -365,6 +366,32 @@ UsbHcInitMemPool (
> > >    return Pool;
> > >  }
> > >
> > > +/**
> > > +  Unlink the memory block from the pool's list.
> > > +
> > > +  @param  Head           The block list head of the memory's pool.
> > > +  @param  BlockToUnlink  The memory block to unlink.
> > > +
> > > +**/
> > > +VOID
> > > +UsbHcUnlinkMemBlock (
> > > +  IN USBHC_MEM_BLOCK  *Head,
> > > +  IN USBHC_MEM_BLOCK  *BlockToUnlink
> > > +  )
> > > +{
> > > +  USBHC_MEM_BLOCK  *Block;
> > > +
> > > +  ASSERT ((Head != NULL) && (BlockToUnlink != NULL));
> > > +
> > > +  for (Block = Head; Block != NULL; Block = Block->Next) {
> > > +    if (Block->Next == BlockToUnlink) {
> > > +      Block->Next         = BlockToUnlink->Next;
> > > +      BlockToUnlink->Next = NULL;
> > > +      break;
> > > +    }
> > > +  }
> > > +}
> > > +
> > >  /**
> > >    Release the memory management pool.
> > >
> > > @@ -386,8 +413,8 @@ UsbHcFreeMemPool (
> > >    // first block.
> > >    //
> > >    for (Block = Pool->Head->Next; Block != NULL; Block =
> > > Pool->Head->Next) {
> > > -    // UsbHcUnlinkMemBlock (Pool->Head, Block);
> > >      UsbHcFreeMemBlock (Pool, Block);
> > > +    UsbHcUnlinkMemBlock (Pool->Head, Block);
> > >    }
> > >
> > >    UsbHcFreeMemBlock (Pool, Pool->Head);
> > > --
> > > 2.37.1.windows.1