From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web11.2461.1571102474305681614 for ; Mon, 14 Oct 2019 18:21:14 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Oct 2019 18:21:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,297,1566889200"; d="scan'208,217";a="207390546" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga002.jf.intel.com with ESMTP; 14 Oct 2019 18:21:13 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 14 Oct 2019 18:21:12 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.225]) with mapi id 14.03.0439.000; Tue, 15 Oct 2019 09:21:10 +0800 From: "Wu, Hao A" To: Ashish Singhal , "devel@edk2.groups.io" , "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//4KdgIAAh0SwgABt3QCABLmvAIAA/CRA Date: Tue, 15 Oct 2019 01:21:09 +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_B80AF82E9BFB8E4FBD8C89DA810C6A093C94745DSHSMSX104ccrcor_" --_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C94745DSHSMSX104ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello, Sorry for the delayed 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 'Ma= pping': >| Status =3D mIoMmu->SetAttribute (mIoMmu, Mapping, 0); >| Status =3D mIoMmu->Unmap (mIoMmu, Mapping); will be done multiple times. I think they should only be done once and sho= uld happen when freeing the aligned buffer. Hence, I would suggest to add a new helper function called IoMmuAllocateAlignedBuffer(), which will be similar to the existing IoMmuAllocateBuffer() but with a couple of additional things to handle: 1. If the IOMMU PPI exists: a. Allocate the buffer according to the real number of pages needed; b. (New) Free the first & last unaligned pages; c. Map the allocated aligned buffer; d. Set the IOMMU attribute for the buffer. 2. If the IOMMU PPI does not exist: a. Allocate the buffer according to the real number of pages needed; b. (New) Free the first & last unaligned pages. Also, could you help to separate the change for XhciPei into a separate pa= tch? It will be helpful to locate the exact commit made to XhciPei and XhciDxe = in the future. Best Regards, Hao Wu From: Ashish 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 A= llocation Hello Hao, Were you able to validate the PEI change I sent last week? Thanks Ashish ________________________________ From: Ashish Singhal Sent: Friday, October 11, 2019 12:07 PM To: Wu, Hao A ; devel@edk2.groups.io ; Ni, Ray Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page A= llocation Patch set 2 has been submitted for review. Thanks Ashish ________________________________ From: Wu, Hao A Sent: Thursday, October 10, 2019 11:53 PM To: devel@edk2.groups.io ; Ashish Singhal ; Ni, Ray Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Fix Aligned Page A= llocation 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_B80AF82E9BFB8E4FBD8C89DA810C6A093C94745DSHSMSX104ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hello,

 

Sorry for the delayed response.<= /p>

 

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

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

 

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

>|  Status =3D mIoMmu->Unmap (mIoMmu, Mapping);<= /o:p>

 

will be done multiple times. I think they should o= nly be done once and should

happen when freeing the aligned buffer.=

 

Hence, I would suggest to add a new helper functio= n called

IoMmuAllocateAlignedBuffer<= /span>(), which will be similar to the existing

IoMmuAllocateBuffer<= /span>() but with a couple of additional things to handle:

 

1. If the IOMMU PPI exists:

a. Allocate the buffer according to the real numbe= r of pages needed;

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

c. Map the allocated aligned buffer;

d. Set the IOMMU attribute for the buffer.

 

2. If the IOMMU PPI does not exist:

a. Allocate the buffer according to the real numbe= r of pages needed;

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

 

 

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

It will be helpful to locate the exact commit made= to XhciPei and XhciDxe in

the future.

 

Best Regards,

Hao Wu

 

From: Ashish 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 review.

 

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 feat= ure is added for the protection of FW integrity against

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

 

The fix is easy for th= e DXE part since the IOMMU part has been integrated to the

implementation of the PciIo protoc= ol.

 

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

'IoMmuAllocateAlignedBuffer') can b= e added in DmaMem.c fil= e. The main differece

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

 

1. If IOMMU PPI exists= , mIoMmu-><= span class=3D"SpellE">AllocateBuffer= () should be called with bigger

number of pages for al= ignment and mIoMmu-><= span class=3D"SpellE">FreeBuffer() c= an be called right after

to free the unused pag= es.

 

2. If IOMMU PPI does n= ot 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 i= f you observe any open for the 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 Allocat= ion

 

Hello Hao,

 

What is your motivation towards usin= g 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 alignment.= 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 pag= es were allocated for each call so that freeing can be done correctly as we= ll. 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 Allocat= ion

 

Thanks Ashish,<= o:p>

 

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

I can help to see if v= erification 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 Allocat= ion

 

Hello Hao,

 

I agree that for completeness we should f= ix the issue in both DXE as well as PEI but on my target, we do not have an= y 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 so= meone else can verify or we can have someone else fix PEI and push my chang= e meanwhile to fix the issue in DXE.

 

Thanks

Ashish

From: Wu, Hao A <ha= o.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

 

> -----Origin= al 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/Xh= ciDxe/UsbHcMe= m.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe<= /span>/UsbHcMem.c
> > b/MdeModulePkg/Bus/Pci/XhciDx= e/UsbHcMem.c<= /span>
> > index fd79988..aa69c47 100644
> > --- a/MdeModulePkg/Bus/Pci/Xh= ciDxe/UsbHcMe= m.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> > @@ -656,7 +656,7 @@ UsbHcAllocateAlignedPages (
> >           = ;             <= 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
> UsbHcAllocateAlignedP= ages() and UsbHcFreeAlignedPages(), 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.
> ---------------------------------------------------------------------= --------------

--_000_B80AF82E9BFB8E4FBD8C89DA810C6A093C94745DSHSMSX104ccrcor_--