|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 18/18] xen/arm64: mm: Rework switch_ttbr()
On Tue, 13 Dec 2022, Julien Grall wrote:
> Hi Stefano,
>
> On 13/12/2022 02:00, Stefano Stabellini wrote:
> > On Mon, 12 Dec 2022, Julien Grall wrote:
> > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > >
> > > At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> > > still on.
> > >
> > > Switching TTBR is like replacing existing mappings with new ones. So
> > > we need to follow the break-before-make sequence.
> > >
> > > In this case, it means the MMU needs to be switched off while the
> > > TTBR is updated. In order to disable the MMU, we need to first
> > > jump to an identity mapping.
> > >
> > > Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> > > top to temporary map the identity mapping and call switch_ttbr()
> > > via the identity address.
> > >
> > > switch_ttbr_id() is now reworked to temporarily turn off the MMU
> > > before updating the TTBR.
> > >
> > > We also need to make sure the helper switch_ttbr() is part of the
> > > identity mapping. So move _end_boot past it.
> > >
> > > The arm32 code will use a different approach. So this issue is for now
> > > only resolved on arm64.
> > >
> > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > This patch looks overall good to me, aside from the few minor comments
> > below. I would love for someone else, maybe from ARM, reviewing steps
> > 1-6 making sure they are the right sequence.
> >
> >
> > > ---
> > >
> > > Changes in v2:
> > > - Remove the arm32 changes. This will be addressed differently
> > > - Re-instate the instruct cache flush. This is not strictly
> > > necessary but kept it for safety.
> > > - Use "dsb ish" rather than "dsb sy".
> > >
> > > TODO:
> > > * Handle the case where the runtime Xen is loaded at a different
> > > position for cache coloring. This will be dealt separately.
> > > ---
> > > xen/arch/arm/arm64/head.S | 50 +++++++++++++++++++++++------------
> > > xen/arch/arm/arm64/mm.c | 39 +++++++++++++++++++++++++++
> > > xen/arch/arm/include/asm/mm.h | 2 ++
> > > xen/arch/arm/mm.c | 14 +++++-----
> > > 4 files changed, 82 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 663f5813b12e..1f69864492b6 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -816,30 +816,46 @@ ENDPROC(fail)
> > > * Switch TTBR
> > > *
> > > * x0 ttbr
> > > - *
> > > - * TODO: This code does not comply with break-before-make.
> > > */
> > > -ENTRY(switch_ttbr)
> > > - dsb sy /* Ensure the flushes happen before
> > > - * continuing */
> > > - isb /* Ensure synchronization with
> > > previous
> > > - * changes to text */
> > > - tlbi alle2 /* Flush hypervisor TLB */
> > > - ic iallu /* Flush I-cache */
> > > - dsb sy /* Ensure completion of TLB flush */
> > > +ENTRY(switch_ttbr_id)
> > > + /* 1) Ensure any previous read/write have completed */
> > > + dsb ish
> > > + isb
> > > +
> > > + /* 2) Turn off MMU */
> > > + mrs x1, SCTLR_EL2
> > > + bic x1, x1, #SCTLR_Axx_ELx_M
> >
> > do we need a "dsb sy" here? we have in enable_mmu
>
> Hmmm... The explanation of the dsb + isb in enable_mmu makes no sense. The isb
> doesn't flush the I-cache, it just flushes the pipeline.
>
> For the dsb, I am not convinced it is necessary because we already have the
> 'dsb nsh' above and in any case the barrier seems to be too strong.
>
> I guess that will be another patch... (probably at a lower priority).
>
> Now back to your question of the 'dsb' here. There is already a 'dsb ish'
> above. So memory access before turning off the MMU should be completed.
> Also...
>
> >
> >
> > > + msr SCTLR_EL2, x1
> > > + isb
>
> ... this isb will ensure the completion of SCTLR before the TLBs are flushed
> before. And there should be no memory access (or than instructions here). So I
> don't think the a dsb is needed.
>
> Would you mind to explain why you think there is one needed?
I am not at all sure whether it is needed or not, I was just noticing
that we have the "dsb sy" in enable_mmu and here we don't.
Thinking about it, the only reason for the additional dsb would be to
make sure that the two operations:
mrs x1, SCTLR_EL2
bic x1, x1, #SCTLR_Axx_ELx_M
are completed before disabling the MMU:
msr SCTLR_EL2, x1
Is that actually a requirement? I don't know.
> > > +
> > > + /*
> > > + * 3) Flush the TLBs.
> > > + * See asm/arm64/flushtlb.h for the explanation of the sequence.
> > > + */
> > > + dsb nshst
> > > + tlbi alle2
> > > + dsb nsh
> > > + isb
> > > +
> > > + /* 4) Update the TTBR */
> > > + msr TTBR0_EL2, x0
> > > isb
> > > - msr TTBR0_EL2, x0
> > > + /*
> > > + * 5) Flush I-cache
> > > + * This should not be necessary but it is kept for safety.
> > > + */
> > > + ic iallu
> > > + isb
> > > - isb /* Ensure synchronization with
> > > previous
> > > - * changes to text */
> > > - tlbi alle2 /* Flush hypervisor TLB */
> > > - ic iallu /* Flush I-cache */
> > > - dsb sy /* Ensure completion of TLB flush */
> > > + /* 5) Turn on the MMU */
> >
> > This should be 6)
>
> I will update it.
>
> >
> >
> > > + mrs x1, SCTLR_EL2
> > > + orr x1, x1, #SCTLR_Axx_ELx_M /* Enable MMU */
> > > + msr SCTLR_EL2, x1
> > > isb
> > > ret
> > > -ENDPROC(switch_ttbr)
> > > +ENDPROC(switch_ttbr_id)
> > > #ifdef CONFIG_EARLY_PRINTK
> > > /*
> > > diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
> > > index 9eaf545ea9dd..2ede4e75ae33 100644
> > > --- a/xen/arch/arm/arm64/mm.c
> > > +++ b/xen/arch/arm/arm64/mm.c
> > > @@ -31,6 +31,15 @@ static void __init prepare_boot_identity_mapping(void)
> > > lpae_t pte;
> > > DECLARE_OFFSETS(id_offsets, id_addr);
> > > + /*
> > > + * We will be re-using the boot ID tables. They may not have been
> > > + * zeroed but they should be unlinked. So it is fine to use
> > > + * clear_page().
> > > + */
> > > + clear_page(boot_first_id);
> > > + clear_page(boot_second_id);
> > > + clear_page(boot_third_id);
> >
> > Could this code be in patch #15?
>
> Yes, I can't remember why I decided to clear them in patch #18.
>
> > > if ( id_offsets[0] != 0 )
> > > panic("Cannot handled ID mapping above 512GB\n");
> > > @@ -111,6 +120,36 @@ void update_identity_mapping(bool enable)
> > > BUG_ON(rc);
> > > }
> > > +extern void switch_ttbr_id(uint64_t ttbr);
> > > +
> > > +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> > > +
> > > +void __init switch_ttbr(uint64_t ttbr)
> > > +{
> > > + vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> > > + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> > > + lpae_t pte;
> > > +
> > > + /* Enable the identity mapping in the boot page tables */
> >
> > See below...
> >
> > > + update_identity_mapping(true);
> > > + /* Enable the identity mapping in the runtime page tables */
> > > + pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
> > > + pte.pt.table = 1;
> > > + pte.pt.xn = 0;
> > > + pte.pt.ro = 1;
> > > + write_pte(&xen_third_id[third_table_offset(id_addr)], pte);
> > > +
> > > + /* Switch TTBR */
> > > + fn(ttbr);
> > > +
> > > + /*
> > > + * Disable the identity mapping in the runtime page tables.
> > > + * Note it is not necessary to disable it in the boot page tables
> > > + * because they are not going to be used by this CPU anymore.
> > > + */
> >
> > ...is update_identity_mapping acting on the boot pagetables or the
> > runtime pagetables? The two comments make me think that
> > update_identity_mapping is enabling mapping in the boot pagetables and
> > removing them from the runtime pagetable, which would be strangely
> > inconsistent.
>
> update_identity_mapping() is acting on the live page-tables (i.e. the one
> pointed by TTBR_EL2).
>
> Before switch_ttbr(), this will be the boot page-tables. But after, this will
> be the runtime page-tables.
>
> Would the following comment on top of the declaration of
> update_identity_mapping() clarifies it:
>
> "Enable/disable the identity mapping in the live page-tables (i.e. the one
> pointed by TTBR_EL2)."
Thank you!
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |