Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
[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
Sent: Monday, December 19, 2022 12:40 AM
To: He, Jiangang ; Chang, Abner ;
devel@edk2.groups.io
Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
; Zeng, Star ; Sun, Zhikai
; Kirkendall, Garrett
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
> Sent: Friday, December 16, 2022 12:48 AM
> To: Chang, Abner ; Wu, Hao A
> ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
>
> 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
> Sent: Wednesday, December 14, 2022 8:12 PM
> To: Wu, Hao A ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> ; He, Jiangang
> 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
> > Sent: Monday, December 12, 2022 11:27 AM
> > To: Chang, Abner ; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > ; Zeng, Star ; Sun, Zhikai
> > ; Kirkendall, Garrett
> >
> > Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei
>
Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
[AMD Official Use Only - General]
Yes, it is the same issue discussed in
https://edk2.groups.io/g/devel/topic/92833071#92165
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://edk2.groups.io/g/devel/topic/92833071#92165 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
Sent: Wednesday, December 14, 2022 8:12 PM
To: Wu, Hao A ; devel@edk2.groups.io
Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
; Zeng, Star ; Sun, Zhikai
; Kirkendall, Garrett ; He,
Jiangang
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
> Sent: Monday, December 12, 2022 11:27 AM
> To: Chang, Abner ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
>
> 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&data=05%7C01%7
> Cabner.chang%40amd.com%7Ccac49c2820e741b8c48a08dadbf0cc24%7C3dd8
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C638064124512265992%7CUnk
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=z1Q7NRxN4GMA%2
> FBxYd2D7Gnkc3aTD23mRnwNF3H5wE0k%3D&reserved=0? Thanks in advance.
>
> Best Regards,
> Hao Wu
>
> > -Original Message-
> > From: abner.ch...@amd.com
> > Sent: Saturday, December 10, 2022 11:13 PM
> > To: devel@edk2.groups.io
> > Cc: kuei-hung@amd.com; Wu, Hao A ; Ni, Ray
> > ; Garrett Kirkendall ;
> > Abner Chang
> > Subject: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> > block
> >
> > From: Abner Chang
> >
> > In V2: Add AMD copyright.
> >
> > Unlink the XhciPei memory block when it has been freed.
> >
> > Signed-off-by: Kuei-Hung Lin
> > Cc: Hao A Wu
> > Cc: Ray Ni
> > Cc: Garrett Kirkendall
> > Cc: Abner Chang
> > ---
> > 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.
> > +Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
> > +reserved.
> >
> > 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;
> > + bre
Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
[AMD Official Use Only - General]
I will send the update according to the discussion.
Thanks
Abner
> -Original Message-
> From: Wu, Hao A
> Sent: Tuesday, December 20, 2022 8:27 AM
> To: He, Jiangang ; Chang, Abner
> ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> 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.
>
>
> Thanks.
> You are right. I agree with you that aligning with XhciDxe is a better
> resolution.
>
> Best Regards,
> Hao Wu
>
> > -Original Message-
> > From: He, Jiangang
> > Sent: Tuesday, December 20, 2022 6:54 AM
> > To: Wu, Hao A ; Chang, Abner
> > ; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > ; Zeng, Star ; Sun, Zhikai
> > ; Kirkendall, Garrett
> >
> > Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei
> memory
> > block
> >
> > [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
> > Sent: Monday, December 19, 2022 12:40 AM
> > To: He, Jiangang ; Chang, Abner
> > ; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > ; Zeng, Star ; Sun, Zhikai
> > ; Kirkendall, Garrett
> >
> > 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%2Fedk
> 2
> > .groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01%
> 7
> > CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3d
> > d8961fe4884e608e11a82d994e183d%7C0%7C0%7C638070288126779782%7C
> >
> Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> T
> >
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uJMNyYXMU22
> > UZNjrKtDetyD2BqrvkualuorPZHAV3wg%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
> > > Sent: Friday, December 16, 2022 12:48 AM
> > > To: Chang, Abner ; Wu, Hao A
> > > ; devel@edk2.groups.io
> > > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > > ; Zeng, Star ; Sun, Zhikai
> > > ; Kirkendall, Garrett
> > >
> > > 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%2Fedk
> 2
> > > .groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01
> > %7CJiang
> > >
> > ang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3dd8961f
> > e4884e60
> > >
> > 8e11a82d994e183d%7C0%7C0%7C638070288126936018%7CUnknown%7CT
> > WFpbGZsb3d8
> > >
> >
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> > D%7C3
> > >
> >
> 000%7C%7C%7C&sdata=58j41QJxKbrQenhyZKYO4dxYj3Sat2kJejQGioZhtu4%
> 3
> > D&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%2Fedk
> 2
> > .groups.io%2Fg%2Fdevel%2Ftopic%2F92833071%2392165&data=05%7C01%
> 7
> > CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3d
> > d
Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
Thanks.
You are right. I agree with you that aligning with XhciDxe is a better
resolution.
Best Regards,
Hao Wu
> -Original Message-
> From: He, Jiangang
> Sent: Tuesday, December 20, 2022 6:54 AM
> To: Wu, Hao A ; Chang, Abner
> ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> block
>
> [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
> Sent: Monday, December 19, 2022 12:40 AM
> To: He, Jiangang ; Chang, Abner
> ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> 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%7
> CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3d
> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C638070288126779782%7C
> Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uJMNyYXMU22
> UZNjrKtDetyD2BqrvkualuorPZHAV3wg%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
> > Sent: Friday, December 16, 2022 12:48 AM
> > To: Chang, Abner ; Wu, Hao A
> > ; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > ; Zeng, Star ; Sun, Zhikai
> > ; Kirkendall, Garrett
> >
> > 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%7C3dd8961f
> e4884e60
> >
> 8e11a82d994e183d%7C0%7C0%7C638070288126936018%7CUnknown%7CT
> WFpbGZsb3d8
> >
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3
> >
> 000%7C%7C%7C&sdata=58j41QJxKbrQenhyZKYO4dxYj3Sat2kJejQGioZhtu4%3
> D&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%7
> CJiangang.He%40amd.com%7C528424aa303f4a2c9b1808dae18be036%7C3d
> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C638070288126936018%7C
> Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=58j41QJxKbrQen
> hyZKYO4dxYj3Sat2kJejQGioZhtu4%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
> > Sent: Wednesd
Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
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://edk2.groups.io/g/devel/topic/92833071#92165, 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
> Sent: Friday, December 16, 2022 12:48 AM
> To: Chang, Abner ; Wu, Hao A
> ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> Subject: RE: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> block
>
> [AMD Official Use Only - General]
>
> Yes, it is the same issue discussed in
> https://edk2.groups.io/g/devel/topic/92833071#92165
>
> 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://edk2.groups.io/g/devel/topic/92833071#92165 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
> Sent: Wednesday, December 14, 2022 8:12 PM
> To: Wu, Hao A ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett ;
> He, Jiangang
> 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
> > Sent: Monday, December 12, 2022 11:27 AM
> > To: Chang, Abner ; devel@edk2.groups.io
> > Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> > ; Zeng, Star ; Sun, Zhikai
> > ; Kirkendall, Garrett
> >
> > 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&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&sdata=z1Q7NRxN4GMA
> %2
> > FBxYd2D7Gnkc3aTD23mRnwNF3H5wE0k%3D&reserved=0? Thanks in
> advance.
> >
> > Best Regards,
> > Hao Wu
> >
> > > -Original Message-
> > > From: abner.ch...@amd.com
> > > Sent: Saturday, December 10, 2022 11:13 PM
> > > To: devel@edk2.groups.io
> > > Cc: kuei-hung@amd.com; Wu, Hao A ; Ni, Ray
> > > ; Garrett Kirkendall ;
> > > Abner Chang
> > > Subject: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> > > block
> > >
> > > From: Abner Chang
> > >
> > > In V2: Add AMD copyright.
> > >
> > > Unlink the XhciPei memory block when it has been freed.
> > >
> > > Signed-off-by: Kuei-Hung Lin
> > > Cc: Hao A Wu
> > > Cc: Ray Ni
> > > Cc: Garrett Kirkendall
> > > Cc: Abner Chang
> > > ---
> > > 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 whic
Re: [edk2-devel] [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
> Sent: Monday, December 12, 2022 11:27 AM
> To: Chang, Abner ; devel@edk2.groups.io
> Cc: Lin, Kuei-Hung (Timothy) ; Ni, Ray
> ; Zeng, Star ; Sun, Zhikai
> ; Kirkendall, Garrett
> 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&data=05%7C01%7
> Cabner.chang%40amd.com%7Ccac49c2820e741b8c48a08dadbf0cc24%7C3dd8
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C638064124512265992%7CUnk
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=z1Q7NRxN4GMA%2
> FBxYd2D7Gnkc3aTD23mRnwNF3H5wE0k%3D&reserved=0? Thanks in
> advance.
>
> Best Regards,
> Hao Wu
>
> > -Original Message-
> > From: abner.ch...@amd.com
> > Sent: Saturday, December 10, 2022 11:13 PM
> > To: devel@edk2.groups.io
> > Cc: kuei-hung@amd.com; Wu, Hao A ; Ni, Ray
> > ; Garrett Kirkendall ;
> > Abner Chang
> > Subject: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory
> > block
> >
> > From: Abner Chang
> >
> > In V2: Add AMD copyright.
> >
> > Unlink the XhciPei memory block when it has been freed.
> >
> > Signed-off-by: Kuei-Hung Lin
> > Cc: Hao A Wu
> > Cc: Ray Ni
> > Cc: Garrett Kirkendall
> > Cc: Abner Chang
> > ---
> > 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.
> > +Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
> > +reserved.
> >
> > 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97409): https://edk2.groups.io/g/devel/message/97409
Mute This Topic: https://groups.io/mt/95582755/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
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://edk2.groups.io/g/devel/topic/92833071#92165? Thanks in advance.
Best Regards,
Hao Wu
> -Original Message-
> From: abner.ch...@amd.com
> Sent: Saturday, December 10, 2022 11:13 PM
> To: devel@edk2.groups.io
> Cc: kuei-hung@amd.com; Wu, Hao A ; Ni, Ray
> ; Garrett Kirkendall ;
> Abner Chang
> Subject: [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
>
> From: Abner Chang
>
> In V2: Add AMD copyright.
>
> Unlink the XhciPei memory block when it has been freed.
>
> Signed-off-by: Kuei-Hung Lin
> Cc: Hao A Wu
> Cc: Ray Ni
> Cc: Garrett Kirkendall
> Cc: Abner Chang
> ---
> 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.
> +Copyright (C) 2022 Advanced Micro Devices, Inc. All rights reserved.
>
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97241): https://edk2.groups.io/g/devel/message/97241
Mute This Topic: https://groups.io/mt/95582755/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V2] MdeModulePkg/XhciPei: Unlinked XhciPei memory block
From: Abner Chang
In V2: Add AMD copyright.
Unlink the XhciPei memory block when it has been freed.
Signed-off-by: Kuei-Hung Lin
Cc: Hao A Wu
Cc: Ray Ni
Cc: Garrett Kirkendall
Cc: Abner Chang
---
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.
+Copyright (C) 2022 Advanced Micro Devices, Inc. All rights reserved.
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97228): https://edk2.groups.io/g/devel/message/97228
Mute This Topic: https://groups.io/mt/95582755/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
