* [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update @ 2023-05-20 9:19 Ard Biesheuvel 2023-05-22 8:57 ` Ard Biesheuvel 2023-05-22 10:56 ` Leif Lindholm 0 siblings, 2 replies; 7+ messages in thread From: Ard Biesheuvel @ 2023-05-20 9:19 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Liming Gao, Leif Lindholm, Michael D Kinney, Oliver Steffen The helper that updates live page table entries writes a zero entry, invalidates the covered address range from the TLBs, and finally writes the actual entry. This ensures that no TLB conflicts can occur. Writing the final entry needs to complete before any translations can be performed, as otherwise, the zero entry, which describes an invalid translation, may be observed by the page table walker, resulting in a translation fault. For this reason, the final write is followed by a DSB barrier instruction. However, this barrier will not stall the pipeline, and instruction fetches may still hit this invalid translation, as has been observed and reported by Oliver. To ensure that the new translation is fully active before returning from this helper, we have to insert an ISB barrier as well. Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Reported-by: Oliver Steffen <osteffen@redhat.com> Tested-by: Oliver Steffen <osteffen@redhat.com> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 + 1 file changed, 1 insertion(+) diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S index 887439bc042f0f16..1f0d8057926933d7 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S @@ -65,6 +65,7 @@ // write updated entry str x1, [x0] dsb nshst + isb .L2_\@: .endm -- 2.39.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-20 9:19 [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update Ard Biesheuvel @ 2023-05-22 8:57 ` Ard Biesheuvel 2023-05-22 10:56 ` Leif Lindholm 1 sibling, 0 replies; 7+ messages in thread From: Ard Biesheuvel @ 2023-05-22 8:57 UTC (permalink / raw) To: devel, Liming Gao, Leif Lindholm; +Cc: Michael D Kinney, Oliver Steffen Any objections to this? If not, I will push it later today. Thanks. On Sat, 20 May 2023 at 11:20, Ard Biesheuvel <ardb@kernel.org> wrote: > > The helper that updates live page table entries writes a zero entry, > invalidates the covered address range from the TLBs, and finally writes > the actual entry. This ensures that no TLB conflicts can occur. > > Writing the final entry needs to complete before any translations can be > performed, as otherwise, the zero entry, which describes an invalid > translation, may be observed by the page table walker, resulting in a > translation fault. For this reason, the final write is followed by a DSB > barrier instruction. > > However, this barrier will not stall the pipeline, and instruction > fetches may still hit this invalid translation, as has been observed and > reported by Oliver. To ensure that the new translation is fully active > before returning from this helper, we have to insert an ISB barrier as > well. > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Reported-by: Oliver Steffen <osteffen@redhat.com> > Tested-by: Oliver Steffen <osteffen@redhat.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > index 887439bc042f0f16..1f0d8057926933d7 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > @@ -65,6 +65,7 @@ > // write updated entry > str x1, [x0] > dsb nshst > + isb > > .L2_\@: > .endm > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-20 9:19 [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update Ard Biesheuvel 2023-05-22 8:57 ` Ard Biesheuvel @ 2023-05-22 10:56 ` Leif Lindholm 2023-05-22 14:08 ` [edk2-devel] " Michael D Kinney 1 sibling, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2023-05-22 10:56 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel, Liming Gao, Michael D Kinney, Oliver Steffen On Sat, May 20, 2023 at 11:19:58 +0200, Ard Biesheuvel wrote: > The helper that updates live page table entries writes a zero entry, > invalidates the covered address range from the TLBs, and finally writes > the actual entry. This ensures that no TLB conflicts can occur. > > Writing the final entry needs to complete before any translations can be > performed, as otherwise, the zero entry, which describes an invalid > translation, may be observed by the page table walker, resulting in a > translation fault. For this reason, the final write is followed by a DSB > barrier instruction. > > However, this barrier will not stall the pipeline, and instruction > fetches may still hit this invalid translation, as has been observed and > reported by Oliver. To ensure that the new translation is fully active > before returning from this helper, we have to insert an ISB barrier as > well. > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Reported-by: Oliver Steffen <osteffen@redhat.com> > Tested-by: Oliver Steffen <osteffen@redhat.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com> We need this in the stable tag. Note: the isb instruction forces the synchronization of certain architectural events. It has no other effects. I.e., any issues exposed by this addition would already have been present before it. As such, I would suggest this addition need *not* affect the stable tag schedule. / Leif > --- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > index 887439bc042f0f16..1f0d8057926933d7 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > @@ -65,6 +65,7 @@ > // write updated entry > str x1, [x0] > dsb nshst > + isb > > .L2_\@: > .endm > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-22 10:56 ` Leif Lindholm @ 2023-05-22 14:08 ` Michael D Kinney 2023-05-22 16:19 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Michael D Kinney @ 2023-05-22 14:08 UTC (permalink / raw) To: devel@edk2.groups.io, quic_llindhol@quicinc.com, Ard Biesheuvel Cc: Gao, Liming, Oliver Steffen, Kinney, Michael D Acked-by: Michael D Kinney <michael.d.kinney@intel.com> No objection to merging for the stable tag. Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif > Lindholm > Sent: Monday, May 22, 2023 3:56 AM > To: Ard Biesheuvel <ardb@kernel.org> > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; > Kinney, Michael D <michael.d.kinney@intel.com>; Oliver Steffen > <osteffen@redhat.com> > Subject: Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: > Add missing ISB after page table update > > On Sat, May 20, 2023 at 11:19:58 +0200, Ard Biesheuvel wrote: > > The helper that updates live page table entries writes a zero entry, > > invalidates the covered address range from the TLBs, and finally writes > > the actual entry. This ensures that no TLB conflicts can occur. > > > > Writing the final entry needs to complete before any translations can be > > performed, as otherwise, the zero entry, which describes an invalid > > translation, may be observed by the page table walker, resulting in a > > translation fault. For this reason, the final write is followed by a DSB > > barrier instruction. > > > > However, this barrier will not stall the pipeline, and instruction > > fetches may still hit this invalid translation, as has been observed and > > reported by Oliver. To ensure that the new translation is fully active > > before returning from this helper, we have to insert an ISB barrier as > > well. > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Reported-by: Oliver Steffen <osteffen@redhat.com> > > Tested-by: Oliver Steffen <osteffen@redhat.com> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com> > > We need this in the stable tag. > > Note: the isb instruction forces the synchronization of certain > architectural events. It has no other effects. I.e., any issues > exposed by this addition would already have been present before it. > > As such, I would suggest this addition need *not* affect the stable > tag schedule. > > / > Leif > > > --- > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git > a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > index 887439bc042f0f16..1f0d8057926933d7 100644 > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > @@ -65,6 +65,7 @@ > > // write updated entry > > str x1, [x0] > > dsb nshst > > + isb > > > > .L2_\@: > > .endm > > -- > > 2.39.2 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-22 14:08 ` [edk2-devel] " Michael D Kinney @ 2023-05-22 16:19 ` Ard Biesheuvel 2023-05-23 0:43 ` Michael D Kinney 0 siblings, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2023-05-22 16:19 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com, Gao, Liming, Oliver Steffen PR here: https://github.com/tianocore/edk2/pull/4418 I cannot set the 'push' label myself, it seems. On Mon, 22 May 2023 at 16:10, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > Acked-by: Michael D Kinney <michael.d.kinney@intel.com> > > No objection to merging for the stable tag. > > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif > > Lindholm > > Sent: Monday, May 22, 2023 3:56 AM > > To: Ard Biesheuvel <ardb@kernel.org> > > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; > > Kinney, Michael D <michael.d.kinney@intel.com>; Oliver Steffen > > <osteffen@redhat.com> > > Subject: Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: > > Add missing ISB after page table update > > > > On Sat, May 20, 2023 at 11:19:58 +0200, Ard Biesheuvel wrote: > > > The helper that updates live page table entries writes a zero entry, > > > invalidates the covered address range from the TLBs, and finally writes > > > the actual entry. This ensures that no TLB conflicts can occur. > > > > > > Writing the final entry needs to complete before any translations can be > > > performed, as otherwise, the zero entry, which describes an invalid > > > translation, may be observed by the page table walker, resulting in a > > > translation fault. For this reason, the final write is followed by a DSB > > > barrier instruction. > > > > > > However, this barrier will not stall the pipeline, and instruction > > > fetches may still hit this invalid translation, as has been observed and > > > reported by Oliver. To ensure that the new translation is fully active > > > before returning from this helper, we have to insert an ISB barrier as > > > well. > > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > > > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > > Reported-by: Oliver Steffen <osteffen@redhat.com> > > > Tested-by: Oliver Steffen <osteffen@redhat.com> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com> > > > > We need this in the stable tag. > > > > Note: the isb instruction forces the synchronization of certain > > architectural events. It has no other effects. I.e., any issues > > exposed by this addition would already have been present before it. > > > > As such, I would suggest this addition need *not* affect the stable > > tag schedule. > > > > / > > Leif > > > > > --- > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git > > a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > index 887439bc042f0f16..1f0d8057926933d7 100644 > > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > @@ -65,6 +65,7 @@ > > > // write updated entry > > > str x1, [x0] > > > dsb nshst > > > + isb > > > > > > .L2_\@: > > > .endm > > > -- > > > 2.39.2 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-22 16:19 ` Ard Biesheuvel @ 2023-05-23 0:43 ` Michael D Kinney 2023-05-23 6:53 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Michael D Kinney @ 2023-05-23 0:43 UTC (permalink / raw) To: devel@edk2.groups.io, ardb@kernel.org Cc: quic_llindhol@quicinc.com, Gao, Liming, Oliver Steffen, Kinney, Michael D I have set push label. It is merged now. During hard freeze, the release owner and stewards are the only once that can set the label. Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard > Biesheuvel > Sent: Monday, May 22, 2023 9:19 AM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; quic_llindhol@quicinc.com; Gao, Liming > <gaoliming@byosoft.com.cn>; Oliver Steffen <osteffen@redhat.com> > Subject: Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: > Add missing ISB after page table update > > PR here: https://github.com/tianocore/edk2/pull/4418 > > I cannot set the 'push' label myself, it seems. > > > > On Mon, 22 May 2023 at 16:10, Kinney, Michael D > <michael.d.kinney@intel.com> wrote: > > > > Acked-by: Michael D Kinney <michael.d.kinney@intel.com> > > > > No objection to merging for the stable tag. > > > > Mike > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif > > > Lindholm > > > Sent: Monday, May 22, 2023 3:56 AM > > > To: Ard Biesheuvel <ardb@kernel.org> > > > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; > > > Kinney, Michael D <michael.d.kinney@intel.com>; Oliver Steffen > > > <osteffen@redhat.com> > > > Subject: Re: [edk2-devel] [PATCH edk2-stable202305] > ArmPkg/ArmMmuLib: > > > Add missing ISB after page table update > > > > > > On Sat, May 20, 2023 at 11:19:58 +0200, Ard Biesheuvel wrote: > > > > The helper that updates live page table entries writes a zero entry, > > > > invalidates the covered address range from the TLBs, and finally writes > > > > the actual entry. This ensures that no TLB conflicts can occur. > > > > > > > > Writing the final entry needs to complete before any translations can > be > > > > performed, as otherwise, the zero entry, which describes an invalid > > > > translation, may be observed by the page table walker, resulting in a > > > > translation fault. For this reason, the final write is followed by a DSB > > > > barrier instruction. > > > > > > > > However, this barrier will not stall the pipeline, and instruction > > > > fetches may still hit this invalid translation, as has been observed and > > > > reported by Oliver. To ensure that the new translation is fully active > > > > before returning from this helper, we have to insert an ISB barrier as > > > > well. > > > > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > > > > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > > > Reported-by: Oliver Steffen <osteffen@redhat.com> > > > > Tested-by: Oliver Steffen <osteffen@redhat.com> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com> > > > > > > We need this in the stable tag. > > > > > > Note: the isb instruction forces the synchronization of certain > > > architectural events. It has no other effects. I.e., any issues > > > exposed by this addition would already have been present before it. > > > > > > As such, I would suggest this addition need *not* affect the stable > > > tag schedule. > > > > > > / > > > Leif > > > > > > > --- > > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 1 > + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git > > > a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > > index 887439bc042f0f16..1f0d8057926933d7 100644 > > > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > > +++ > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > > > @@ -65,6 +65,7 @@ > > > > // write updated entry > > > > str x1, [x0] > > > > dsb nshst > > > > + isb > > > > > > > > .L2_\@: > > > > .endm > > > > -- > > > > 2.39.2 > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update 2023-05-23 0:43 ` Michael D Kinney @ 2023-05-23 6:53 ` Ard Biesheuvel 0 siblings, 0 replies; 7+ messages in thread From: Ard Biesheuvel @ 2023-05-23 6:53 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com, Gao, Liming, Oliver Steffen On Tue, 23 May 2023 at 02:44, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > I have set push label. It is merged now. > > During hard freeze, the release owner and stewards are the only once that can set the label. > Yeah that seems fair. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-23 6:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-20 9:19 [PATCH edk2-stable202305] ArmPkg/ArmMmuLib: Add missing ISB after page table update Ard Biesheuvel 2023-05-22 8:57 ` Ard Biesheuvel 2023-05-22 10:56 ` Leif Lindholm 2023-05-22 14:08 ` [edk2-devel] " Michael D Kinney 2023-05-22 16:19 ` Ard Biesheuvel 2023-05-23 0:43 ` Michael D Kinney 2023-05-23 6:53 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox