| Message ID | 20230619170115.81398-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add some missing ISBs after updating the PTEs | expand |
Hi Julien, > -----Original Message----- > Subject: [PATCH 3/7] xen/arm64: head: Add missing isb in setup_fixmap() > > From: Julien Grall <jgrall@amazon.com> > > On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were > the following paragraph: > > "DMB and DSB instructions affect reads and writes to the memory system > generated by Load/Store instructions and data or unified cache > maintenance instructions being executed by the PE. Instruction fetches > or accesses caused by a hardware translation table access are not > explicit accesses." > > Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence > (it might be somewhere else in the Arm Arm). But the interpretation is > not much different. > > In setup_fixmap(), we write the fixmap area and may be used soon after, > for instance, to write to the UART. IOW, there could be hardware > translation table access. So we need to ensure the 'dsb' has completed > before continuing. Therefore add an 'isb'. > > Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is > not used") > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> I've also tested this patch on top of today's staging by our internal CI, which includes some arm64 emulated platforms and also real hardware, and this patch looks good, so: Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
> On 19 Jun 2023, at 18:01, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were > the following paragraph: > > "DMB and DSB instructions affect reads and writes to the memory system > generated by Load/Store instructions and data or unified cache > maintenance instructions being executed by the PE. Instruction fetches > or accesses caused by a hardware translation table access are not > explicit accesses." > > Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence > (it might be somewhere else in the Arm Arm). But the interpretation is > not much different. > > In setup_fixmap(), we write the fixmap area and may be used soon after, > for instance, to write to the UART. IOW, there could be hardware > translation table access. So we need to ensure the 'dsb' has completed > before continuing. Therefore add an 'isb'. > > Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
On 19/06/2023 19:01, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were > the following paragraph: > > "DMB and DSB instructions affect reads and writes to the memory system > generated by Load/Store instructions and data or unified cache > maintenance instructions being executed by the PE. Instruction fetches > or accesses caused by a hardware translation table access are not > explicit accesses." > > Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence > (it might be somewhere else in the Arm Arm). But the interpretation is > not much different. > > In setup_fixmap(), we write the fixmap area and may be used soon after, > for instance, to write to the UART. IOW, there could be hardware > translation table access. So we need to ensure the 'dsb' has completed > before continuing. Therefore add an 'isb'. > > Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> I'm happy with the whole series but I do not see a point in flooding each patch with my tag since you already got two (from Henry and Luca). When it comes to essential isb() after dsb() in arm64 head.S, I can see that we are missing one in enable_mmu() after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be complete after the execution of DSB by that PE, followed by a Context synchronization event", so I view isb as necessary there. We could also introduce (just like for arm32) flush_xen_tlb_local macro and use it there + remove opencoding it. ~Michal
Hi, On 21/06/2023 10:33, Michal Orzel wrote: > > > On 19/06/2023 19:01, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@amazon.com> >> >> On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were >> the following paragraph: >> >> "DMB and DSB instructions affect reads and writes to the memory system >> generated by Load/Store instructions and data or unified cache >> maintenance instructions being executed by the PE. Instruction fetches >> or accesses caused by a hardware translation table access are not >> explicit accesses." >> >> Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence >> (it might be somewhere else in the Arm Arm). But the interpretation is >> not much different. >> >> In setup_fixmap(), we write the fixmap area and may be used soon after, >> for instance, to write to the UART. IOW, there could be hardware >> translation table access. So we need to ensure the 'dsb' has completed >> before continuing. Therefore add an 'isb'. >> >> Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") >> Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > I'm happy with the whole series but I do not see a point in flooding each patch with my tag > since you already got two (from Henry and Luca). Thanks. To clarify, shall I add it in each patch or only this one? > > When it comes to essential isb() after dsb() in arm64 head.S, I can see that we are missing one in enable_mmu() > after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be complete after the execution of > DSB by that PE, followed by a Context synchronization event", so I view isb as necessary there. While there is no ISB directly after DSB NSH, there are one right after MSR. I don't think we need one before because nothing will use the TLBs between before the ISB. /* * The state of the TLBs is unknown before turning on the MMU. * Flush them to avoid stale one. */ tlbi alle2 /* Flush hypervisor TLBs */ dsb nsh /* Write Xen's PT's paddr into TTBR0_EL2 */ load_paddr x0, boot_pgtable msr TTBR0_EL2, x0 isb > We could also > introduce (just like for arm32) flush_xen_tlb_local macro and use it there + remove opencoding it. That would be good. But I don't think this is necessary here (see above). Cheers,
On 21/06/2023 12:02, Julien Grall wrote: > > > Hi, > > On 21/06/2023 10:33, Michal Orzel wrote: >> >> >> On 19/06/2023 19:01, Julien Grall wrote: >>> >>> >>> From: Julien Grall <jgrall@amazon.com> >>> >>> On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were >>> the following paragraph: >>> >>> "DMB and DSB instructions affect reads and writes to the memory system >>> generated by Load/Store instructions and data or unified cache >>> maintenance instructions being executed by the PE. Instruction fetches >>> or accesses caused by a hardware translation table access are not >>> explicit accesses." >>> >>> Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence >>> (it might be somewhere else in the Arm Arm). But the interpretation is >>> not much different. >>> >>> In setup_fixmap(), we write the fixmap area and may be used soon after, >>> for instance, to write to the UART. IOW, there could be hardware >>> translation table access. So we need to ensure the 'dsb' has completed >>> before continuing. Therefore add an 'isb'. >>> >>> Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >> Reviewed-by: Michal Orzel <michal.orzel@amd.com> >> >> I'm happy with the whole series but I do not see a point in flooding each patch with my tag >> since you already got two (from Henry and Luca). > > Thanks. To clarify, shall I add it in each patch or only this one? Whatever you prefer. If you care about my tag and want to have more than two, feel free to add it to all the patches. > >> >> When it comes to essential isb() after dsb() in arm64 head.S, I can see that we are missing one in enable_mmu() >> after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be complete after the execution of >> DSB by that PE, followed by a Context synchronization event", so I view isb as necessary there. > > While there is no ISB directly after DSB NSH, there are one right after > MSR. I don't think we need one before because nothing will use the TLBs > between before the ISB. > > /* > * The state of the TLBs is unknown before turning on the MMU. > * Flush them to avoid stale one. > */ > tlbi alle2 /* Flush hypervisor TLBs */ > dsb nsh > > /* Write Xen's PT's paddr into TTBR0_EL2 */ > load_paddr x0, boot_pgtable > msr TTBR0_EL2, x0 > isb > Although having isb after dsb would be clearer to the user and consistent with other TBL invalidations, the one after msr can do the job for now, I agree. ~Michal
Hi, On 21/06/2023 11:13, Michal Orzel wrote: > > > On 21/06/2023 12:02, Julien Grall wrote: >> >> >> Hi, >> >> On 21/06/2023 10:33, Michal Orzel wrote: >>> >>> >>> On 19/06/2023 19:01, Julien Grall wrote: >>>> >>>> >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were >>>> the following paragraph: >>>> >>>> "DMB and DSB instructions affect reads and writes to the memory system >>>> generated by Load/Store instructions and data or unified cache >>>> maintenance instructions being executed by the PE. Instruction fetches >>>> or accesses caused by a hardware translation table access are not >>>> explicit accesses." >>>> >>>> Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence >>>> (it might be somewhere else in the Arm Arm). But the interpretation is >>>> not much different. >>>> >>>> In setup_fixmap(), we write the fixmap area and may be used soon after, >>>> for instance, to write to the UART. IOW, there could be hardware >>>> translation table access. So we need to ensure the 'dsb' has completed >>>> before continuing. Therefore add an 'isb'. >>>> >>>> Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") >>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> Reviewed-by: Michal Orzel <michal.orzel@amd.com> >>> >>> I'm happy with the whole series but I do not see a point in flooding each patch with my tag >>> since you already got two (from Henry and Luca). >> >> Thanks. To clarify, shall I add it in each patch or only this one? > Whatever you prefer. If you care about my tag and want to have more than two, feel free to add it to > all the patches. Ok. I will not then because I need to add the ack manually. Cheers,
Hi Julien, > On 19 Jun 2023, at 19:01, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were > the following paragraph: > > "DMB and DSB instructions affect reads and writes to the memory system > generated by Load/Store instructions and data or unified cache > maintenance instructions being executed by the PE. Instruction fetches > or accesses caused by a hardware translation table access are not > explicit accesses." > > Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence > (it might be somewhere else in the Arm Arm). But the interpretation is > not much different. > > In setup_fixmap(), we write the fixmap area and may be used soon after, > for instance, to write to the UART. IOW, there could be hardware > translation table access. So we need to ensure the 'dsb' has completed > before continuing. Therefore add an 'isb'. > > Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not used") > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > xen/arch/arm/arm64/head.S | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index f37133cf7ccd..4ea64e70997b 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -769,6 +769,11 @@ setup_fixmap: > create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 > /* Ensure any page table updates made above have occurred. */ > dsb nshst > + /* > + * The fixmap area will be used soon after. So ensure no hardware > + * translation happens before the dsb completes. > + */ > + isb > > ret > ENDPROC(setup_fixmap) > -- > 2.40.1 >
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index f37133cf7ccd..4ea64e70997b 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -769,6 +769,11 @@ setup_fixmap: create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 /* Ensure any page table updates made above have occurred. */ dsb nshst + /* + * The fixmap area will be used soon after. So ensure no hardware + * translation happens before the dsb completes. + */ + isb ret ENDPROC(setup_fixmap)