From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web12.6525.1570773218624556241 for ; Thu, 10 Oct 2019 22:53:38 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Oct 2019 22:53:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,283,1566889200"; d="scan'208,217";a="193432070" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga008.fm.intel.com with ESMTP; 10 Oct 2019 22:53:38 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 10 Oct 2019 22:53:37 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 10 Oct 2019 22:53:37 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.119]) with mapi id 14.03.0439.000; Fri, 11 Oct 2019 13:53:35 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "ashishsingha@nvidia.com" , "Ni, Ray" Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation Thread-Topic: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation Thread-Index: AQHVfsNZ4DcOEA/aqEOUTbXlESnb2adTE2xQ//+FUgCAAIfmQIAAgk8AgAEfKnD//4KdgIAAh0Sw Date: Fri, 11 Oct 2019 05:53:35 +0000 Message-ID: References: <9ce268553db91fbe7fb13e2205d0e1611e1d0212.1570640221.git.ashishsingha@nvidia.com> , In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C946724SHSMSX104ccrcor_" --_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C946724SHSMSX104ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello Ashish, I think the IOMMU feature is added for the protection of FW integrity agai= nst HW devices, which is the motivation to use IOMMU PPI for memory allocation= . The fix is easy for the DXE part since the IOMMU part has been integrated = to the implementation of the PciIo protocol. As for the XhciPei case, my thought is that a new helper function (maybe c= alled 'IoMmuAllocateAlignedBuffer') can be added in DmaMem.c file. The main diff= erece between IoMmuAllocateAlignedBuffer() and existing IoMmuAllocateBuffer() wi= ll be: 1. If IOMMU PPI exists, mIoMmu->AllocateBuffer() should be called with big= ger number of pages for alignment and mIoMmu->FreeBuffer() can be called right= after to free the unused pages. 2. If IOMMU PPI does 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 raise if you observe any open for the above proposal, thank= s. Best Regards, Hao Wu From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ashi= sh 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 A= llocation Hello Hao, What is your motivation towards using IoMMUPei for memory allocation now i= nstead of Pei Services? The fix would be simple if we just change the numbe= r of pages needed to accommodate for the alignment. The call to free the pa= ges 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 as well. This may need more refactoring of the code that = initially thought of. Thanks Ashish From: Wu, Hao A Sent: Thursday, October 10, 2019 9:00 PM To: devel@edk2.groups.io; Ashish Singhal ; Ni, Ra= y Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page A= llocation Thanks Ashish, Please help to send 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.groups.io; Ni, Ray Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page A= llocation Hello Hao, I agree that for completeness we should 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 mak= e the change so that someone else can verify or we can have someone else fi= x PEI and push my change meanwhile to fix the issue in DXE. Thanks Ashish ________________________________ From: Wu, Hao A > Sent: Wednesday, October 9, 2019 8:09 PM To: Ashish Singhal >; devel@edk2.groups.io >; Ni, Ray > Subject: RE: [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page Allocation > -----Original Message----- > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > Sent: Thursday, October 10, 2019 9:58 AM > To: Wu, Hao A; devel@edk2.groups.io; Ni, Ra= y > 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 that a= s well > but I have no way to verify that as we are not using it. For the change = 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 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 > > Sent: Wednesday, October 9, 2019 7:33 PM > To: Ashish Singhal >; devel@edk2.groups.io; Ni, > Ray > > 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 for > > that alignment. The calls to free pages takes care of this already. > > > > Signed-off-by: Ashish Singhal > > > --- > > 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 ( > > PciIo, > > AllocateAnyPages, > > EfiBootServicesData, > > - Pages, > > + RealPages, > > &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 h= elp to > resolve it as well? > > I think for the PEI counterpart you may need to update both > UsbHcAllocateAlignedPages() and UsbHcFreeAlignedPages(), since the > IOMMU helper functions like IoMmuAllocateBuffer() and IoMmuFreeBuffer() > might not be suitable now. Instead, I think services in the IoMmu PPI ca= n be > used. > > 2. Could you help to provide the information on what test has been done = 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) and = may > contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact th= e > sender by > reply email and destroy all copies of the original message. > ------------------------------------------------------------------------= ----------- --_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C946724SHSMSX104ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hello Ashish,

 

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

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

 

The fix is easy for the DXE part since the IOMMU p= art has been integrated to the

implementation of the PciIo protocol.

 

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

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

between IoMmuAllocateAlignedBuffer() and existing IoMmuAllocateBuffer() will be:

 

1. If IOMMU PPI exists, mIoMmu->AllocateBu= ffer() should be called with bigger

number of pages for alignment and mIoMmu->FreeBuffer= () can be called right after

to free the unused pages.

 

2. If IOMMU PPI does not exist, this case will fal= l back to a similar fix for

the DXE case.

 

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

Please help to raise if you observe any open for t= he above proposal, thanks.

 

Best Regards,

Hao Wu

 

From: devel@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 Allocation

 

Hello Hao,

 

What is your motivation towards using IoMMUPei for memory allocation now instead o= f Pei Services? The fix would be simple if we just change the number of pag= es needed to accommodate for the alignment. The call to free the pages woul= d not do anything.

 

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

 

Thanks

Ashish

 

From: Wu, H= ao 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 Allocation

 

Thanks Ashish,

 =

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

I can help to see if ver= ification 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 Allocation

 

Hello Hao,

 

I agree that for completeness we should 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 change mea= nwhile to fix the issue in DXE.

 

Thanks

Ashish

From: 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

 

> -----Original= 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 @@ UsbHcAllocateAlig= nedPages (
> >           = ;             <= span class=3D"SpellE">PciIo,
> >           = ;             <= span class=3D"SpellE">AllocateAnyPages,
> >           = ;             <= span class=3D"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
> UsbHcAllocateAlignedPages() and UsbHcFreeAlignedPages(), since the
> IOMMU helper functions like IoMmuAllocateBuffe= r() 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.
> ---------------------------------------------------------------------= --------------

--_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C946724SHSMSX104ccrcor_--