Hello,

Sorry for the delaye= d response.

For the PEI changes,= I found that when using the function IoMmuFreeBuffer() to

free the first/last = unaligned page(s), the below operations related to 'Mapping':

>| Status =3D mIoMmu->SetAttribute (mIoMmu,= Mapping, 0);

>| Status =3D mIoMmu->Unmap (mIoMmu, Mappin= g);

will be done multipl= e times. I think they should only be done once and should

happen when freeing = the aligned buffer.

Hence, I would sugge= st to add a new helper function called

IoMmuAllocateAlignedBuffer(= ), which will be similar to the existing

IoMmuAllocateBuffer() but w= ith a couple of additional things to handle:

1. If the IOMMU PPI = exists:

a. Allocate the buff= er according to the real number of pages needed;

b. (New) Free the fi= rst & last unaligned pages;

c. Map the allocated= aligned buffer;

d. Set the IOMMU att= ribute for the buffer.

2. If the IOMMU PPI = does not exist:

a. Allocate the buff= er according to the real number of pages needed;

b. (New) Free the fi= rst & last unaligned pages.

Also, could you help= to separate the change for XhciPei into a separate patch?

It will be helpful t= o locate the exact commit made to XhciPei and XhciD= xe in

the future.

Best Regards,=

Hao Wu

From: Ash= ish Singhal [mailto:ashishsingha@nvidia.com]
Sent: Tuesday, October 15, 2019 2:17 AM
To: Wu, Hao A; devel@edk2.groups.io; Ni, Ray
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation

Hello Hao,

Were you able to validate the PEI change= I sent last week?

Thanks

Ashish

From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Friday, October 11, 2019 12:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io <= devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation

Patch set 2 has been submitted for revie= w.

Thanks

Ashish

From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Thursday, October 10, 2019 11:53 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singh= al <ashishsingha@nvidia.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation

Hello Ashish,

I think the IOMMU = feature is added for the protection of FW integrity against

HW devices, which = is the motivation to use IOMMU PPI for memory allocation.

The fix is easy fo= r the DXE part since the IOMMU part has been integrated to the

implementation of = the PciIo pr= otocol.

As for the XhciPei = case, my thought is that a new helper function (maybe called

'IoMmuAllocateAlignedBuffer= ') can be added in DmaMem.c= file. The main differece

between IoMmuAllocateAlignedBu= ffer() and existing IoMmuAllocateBuffer() will be:

1. If IOMMU PPI ex= ists, mIoMmu-&= gt;AllocateBuffer() should be called with bigger

number of pages fo= r alignment and mIoMmu-&= gt;FreeBuffer() can be called right after

to free the unused= pages.

2. If IOMMU PPI do= es not exist, this case will fall back to a similar fix for

the DXE case.

Ideally, IoMmuFreeBuffer= () can be reused to free the aligned buffer.

Please help to rai= se if you observe any open for the above proposal, thanks.

Best Regards,

Hao Wu

From: d= evel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ashish Singhal
Sent: Friday, October 11, 2019 11:30 AM
To: Wu, Hao A; devel@edk2.groups.io; Ni, Ray
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page All= ocation

Hello Hao,

What is your motivation towards u= sing IoMMUPei= for memory allocation now instead of Pei Services? The fix would be simple= if we just change the number of pages needed to accommodate for the alignm= ent. The call to free the pages would not do anything.

If we use IoMMUPei= , we may need to change function definition for allocate to return how many= pages were allocated for each call so that freeing can be done correctly a= s well. This may need more refactoring of the code that initially thought of.

Thanks

Ashish

From:= Wu, Hao A <hao.a.wu@intel.com>
Sent: Thursday, October 10, 2019 9:00 PM
To: devel@edk2.groups.io; Ashish Singhal <ashishsingha@nvidia.co= m>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page All= ocation

Thanks Ashish,

Please help to sen= d out the fix for XhciPei = in a separate patch as well.

I can help to see = if verification can be done on my side.

Best Regards,

Hao Wu

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ashish Singhal
Sent: Friday, October 11, 2019 1:51 AM
To: Wu, Hao A; devel@edk2.g= roups.io; Ni, Ray
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page All= ocation

Hello Hao,

I agree that for completeness we shoul= d fix the issue in both DXE as well as PEI but on my target, we do not have= any PEI phase for me to be able to verify any change that I will be making. If you still want, I can still make the change so that = someone else can verify or we can have someone else fix PEI and push my cha= nge meanwhile to fix the issue in DXE.

Thanks

Ashish

From:<= span style=3D"font-size:11.0pt; font-family:"Calibri","sans-= serif"; color:black"> Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, October 9, 2019 8:09 PM
To: Ashish Singhal <a= shishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray &l= t;ray.ni@intel.com>
Subject: RE: [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation

> -----Orig= inal Message-----
> From: Ashish Singhal [mail= to:ashishsingha@nvidia.com]
> Sent: Thursday, October 10, 2019 9:58 AM
> To: Wu, Hao A; devel@edk2.gro= ups.io; Ni, Ray
> Subject: RE: [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation
>
> Hello Hao,
>
> I can see that the PEI also has the same issue and take a look at tha= t as well
> but I have no way to verify that as we are not using it. For the chan= ge I have
> made in DXE, I have verified it with an alignment of 4K and 64K.

Is it possible for you to verify the PEI case with a test PEI module that<= br> performs read operation to a USB storage device? I think this will trigger=
the affecting codes.

In my opinion, it would be better for the fix to be complete.

Best Regards,
Hao Wu

>
> Thanks
> Ashish
>
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@in= tel.com>
> Sent: Wednesday, October 9, 2019 7:33 PM
> To: Ashish Singhal <ash= ishsingha@nvidia.com>; devel@edk2.groups.io; Ni,
> Ray <ray.ni@intel.com><= br> > Subject: RE: [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation
>
> > -----Original Message-----
> > From: Ashish Singhal [mailto:ashishsingha@nvidia.com]
> > Sent: Thursday, October 10, 2019 1:02 AM
> > To: devel@edk2.groups.io= ; Wu, Hao A; Ni, Ray
> > Cc: Ashish Singhal
> > Subject: [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation
> >
> > While allocating pages aligned at an alignment higher than 4K, > > allocate memory taking into consideration the padding required f= or
> > that alignment. The calls to free pages takes care of this alrea= dy.
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> > ---
> > MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> > index fd79988..aa69c47 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> > @@ -656,7 +656,7 @@ UsbHcAllocateAlignedPages (
> >           = ;             <= span class=3D"x_SpellE">PciIo,
> >           = ;             <= span class=3D"x_SpellE">AllocateAnyPages<= /span>,
> >           = ;             <= span class=3D"x_SpellE">EfiBootServicesData,
> > -          &nb= sp;           Pages,
> > +          = ;            RealPages,
> >           = ;             &= amp;Memory,
> >           = ;             0=
> >           = ;             )= ;
>
>
> Hello,
>
> The change looks good to me.
>
> Just a couple of things to confirm:
>
> 1. I think there is a similar case within the XhciPei driver Could you help to=
> resolve it as well?
>
> I think for the PEI counterpart you may need to update both
> UsbHcAllocateAlig= nedPages() and UsbHcFreeAlignedPages<= /span>(), since the
> IOMMU helper functions like IoMmuAllocateBuffer() and IoMmuFreeBuffer= ()
> might not be suitable now. Instead, I think services in the IoMmu PPI can be
> used.
>
> 2. Could you help to provide the information on what test has been do= ne for
> the proposed patch?
>
> Thanks in advance.
>
> Best Regards,
> Hao Wu
>
>
> > --
> > 2.7.4
>
> ---------------------------------------------------------------------= --------------
> This email message is for the sole use of the intended recipient(s) a= nd may
> contain
> confidential information. Any unauthorized review, use, disclos= ure or
> distribution
> is prohibited. If you are not the intended recipient, please co= ntact the
> sender by
> reply email and destroy all copies of the original message.
> ---------------------------------------------------------------------= --------------