[edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Zhiguang Liu]

After ConvertMemoryPageToNotPresent, there is always a flush TLB
function. So, to improve performance, there is no need to write CR3
inside ConvertMemoryPageToNotPresent

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index 15c7015fb8..c6894458f7 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
   }
 
   ASSERT_EFI_ERROR (Status);
-  AsmWriteCr3 (PageTable);
   return Status;
 }
 
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113514): https://edk2.groups.io/g/devel/message/113514
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, January 10, 2024 1:43 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in
> ConvertMemoryPageToNotPresent
> 
> After ConvertMemoryPageToNotPresent, there is always a flush TLB
> function. So, to improve performance, there is no need to write CR3
> inside ConvertMemoryPageToNotPresent
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index 15c7015fb8..c6894458f7 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
>    }
> 
>    ASSERT_EFI_ERROR (Status);
> -  AsmWriteCr3 (PageTable);
>    return Status;
>  }
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113520): https://edk2.groups.io/g/devel/message/113520
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Laszlo Ersek]

On 1/10/24 06:43, Zhiguang Liu wrote:
> After ConvertMemoryPageToNotPresent, there is always a flush TLB
> function. So, to improve performance, there is no need to write CR3
> inside ConvertMemoryPageToNotPresent
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index 15c7015fb8..c6894458f7 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
>    }
>  
>    ASSERT_EFI_ERROR (Status);
> -  AsmWriteCr3 (PageTable);
>    return Status;
>  }
>  

(1) I mostly understand the point that the commit message makes, but the
message is not clear enough. The real point is that we have two
ConvertMemoryPageToNotPresent() calls altogether, and each of those
takes place in a *loop*, and each of those loops is followed by a
CpuFlushTlb() call.

The loops matter. If there were no loops, then we might be motivated to
choose a different solution (for example, to move centralize the
CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or
something similar).

So, please update the commit message; mention the loops.

(2) I can't easily see why this change is actually correct. IIRC,
writing CR3 has a "side effect" of flushing the TLB. But obviously,
that's not the *only* effect of writing CR3. You could say that
CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs.
Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(),
you need to demonstrate that the functionality that now is *not* going
to be done has always been superfluous. In more direct terms, you need
to show that the AsmWriteCr3() write call that's being removed here
never actuall changes the *value* of CR3.

And I cannot show that myself very easily.
ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(),
"PageTable" is first set from AsmReadCr3(), then passed twice to
PageTableMap() by reference (!), and then it is written back to CR3. If
at least one of those PageTableMap() calls change "PageTable", then the
AsmWriteCr3() call at the end that's now being removed actually changes
the value of CR3, and then the patch would be wrong.

It's possible that PageTableMap() never changes the value of
"PageTable", but it must be proved, and the evidence should be included
in the commit message.

(3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent()
will no longer flush the TLB itself -- that will always be left to the
caller. This new caller responsibility should be documented in the
leading comment of ConvertMemoryPageToNotPresent(). We already have a
paragraph there starting with "Caller should make sure..."

Sorry for making such a big deal out of this, but such simple-looking
changes can sometimes case obscure (and rarely occurring) bugs down the
road. If you already have evidence that CR3 does not change here, that's
great, but then please don't think it's "obvious"; just go ahead and
document it.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113527): https://edk2.groups.io/g/devel/message/113527
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Ni, Ray]

Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 10, 2024 7:57 PM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in
> ConvertMemoryPageToNotPresent
> 
> On 1/10/24 06:43, Zhiguang Liu wrote:
> > After ConvertMemoryPageToNotPresent, there is always a flush TLB
> > function. So, to improve performance, there is no need to write CR3
> > inside ConvertMemoryPageToNotPresent
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >  UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > index 15c7015fb8..c6894458f7 100644
> > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
> >    }
> >
> >    ASSERT_EFI_ERROR (Status);
> > -  AsmWriteCr3 (PageTable);
> >    return Status;
> >  }
> >
> 
> (1) I mostly understand the point that the commit message makes, but the
> message is not clear enough. The real point is that we have two
> ConvertMemoryPageToNotPresent() calls altogether, and each of those
> takes place in a *loop*, and each of those loops is followed by a
> CpuFlushTlb() call.
> 
> The loops matter. If there were no loops, then we might be motivated to
> choose a different solution (for example, to move centralize the
> CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or
> something similar).
> 
> So, please update the commit message; mention the loops.
> 
> (2) I can't easily see why this change is actually correct. IIRC,
> writing CR3 has a "side effect" of flushing the TLB. But obviously,
> that's not the *only* effect of writing CR3. You could say that
> CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs.
> Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(),
> you need to demonstrate that the functionality that now is *not* going
> to be done has always been superfluous. In more direct terms, you need
> to show that the AsmWriteCr3() write call that's being removed here
> never actuall changes the *value* of CR3.
> 
> And I cannot show that myself very easily.
> ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(),
> "PageTable" is first set from AsmReadCr3(), then passed twice to
> PageTableMap() by reference (!), and then it is written back to CR3. If
> at least one of those PageTableMap() calls change "PageTable", then the
> AsmWriteCr3() call at the end that's now being removed actually changes
> the value of CR3, and then the patch would be wrong.
> 
> It's possible that PageTableMap() never changes the value of
> "PageTable", but it must be proved, and the evidence should be included
> in the commit message.
> 
> (3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent()
> will no longer flush the TLB itself -- that will always be left to the
> caller. This new caller responsibility should be documented in the
> leading comment of ConvertMemoryPageToNotPresent(). We already have a
> paragraph there starting with "Caller should make sure..."
> 
> Sorry for making such a big deal out of this, but such simple-looking
> changes can sometimes case obscure (and rarely occurring) bugs down the
> road. If you already have evidence that CR3 does not change here, that's
> great, but then please don't think it's "obvious"; just go ahead and
> document it.

Laszlo,
Happy to see these comments! All make sense!

PageTableMap() only modifies the PageTable root pointer when creating from zero.
Since there is only one instance of the PageTableLib, I think we could update the
PageTableLib API comments to explicitly mention that. So point (2) will be resolved.

> 
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113569): https://edk2.groups.io/g/devel/message/113569
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Laszlo Ersek]

On 1/11/24 03:08, Ni, Ray wrote:
> 
> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, January 10, 2024 7:57 PM
>> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Gerd Hoffmann <kraxel@redhat.com>
>> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in
>> ConvertMemoryPageToNotPresent
>>
>> On 1/10/24 06:43, Zhiguang Liu wrote:
>>> After ConvertMemoryPageToNotPresent, there is always a flush TLB
>>> function. So, to improve performance, there is no need to write CR3
>>> inside ConvertMemoryPageToNotPresent
>>>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
>>> ---
>>>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> index 15c7015fb8..c6894458f7 100644
>>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
>>>    }
>>>
>>>    ASSERT_EFI_ERROR (Status);
>>> -  AsmWriteCr3 (PageTable);
>>>    return Status;
>>>  }
>>>
>>
>> (1) I mostly understand the point that the commit message makes, but the
>> message is not clear enough. The real point is that we have two
>> ConvertMemoryPageToNotPresent() calls altogether, and each of those
>> takes place in a *loop*, and each of those loops is followed by a
>> CpuFlushTlb() call.
>>
>> The loops matter. If there were no loops, then we might be motivated to
>> choose a different solution (for example, to move centralize the
>> CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or
>> something similar).
>>
>> So, please update the commit message; mention the loops.
>>
>> (2) I can't easily see why this change is actually correct. IIRC,
>> writing CR3 has a "side effect" of flushing the TLB. But obviously,
>> that's not the *only* effect of writing CR3. You could say that
>> CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs.
>> Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(),
>> you need to demonstrate that the functionality that now is *not* going
>> to be done has always been superfluous. In more direct terms, you need
>> to show that the AsmWriteCr3() write call that's being removed here
>> never actuall changes the *value* of CR3.
>>
>> And I cannot show that myself very easily.
>> ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(),
>> "PageTable" is first set from AsmReadCr3(), then passed twice to
>> PageTableMap() by reference (!), and then it is written back to CR3. If
>> at least one of those PageTableMap() calls change "PageTable", then the
>> AsmWriteCr3() call at the end that's now being removed actually changes
>> the value of CR3, and then the patch would be wrong.
>>
>> It's possible that PageTableMap() never changes the value of
>> "PageTable", but it must be proved, and the evidence should be included
>> in the commit message.
>>
>> (3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent()
>> will no longer flush the TLB itself -- that will always be left to the
>> caller. This new caller responsibility should be documented in the
>> leading comment of ConvertMemoryPageToNotPresent(). We already have a
>> paragraph there starting with "Caller should make sure..."
>>
>> Sorry for making such a big deal out of this, but such simple-looking
>> changes can sometimes case obscure (and rarely occurring) bugs down the
>> road. If you already have evidence that CR3 does not change here, that's
>> great, but then please don't think it's "obvious"; just go ahead and
>> document it.
> 
> Laszlo,
> Happy to see these comments! All make sense!
> 
> PageTableMap() only modifies the PageTable root pointer when creating from zero.
> Since there is only one instance of the PageTableLib, I think we could update the
> PageTableLib API comments to explicitly mention that. So point (2) will be resolved.

That should work, thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113597): https://edk2.groups.io/g/devel/message/113597
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-