[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used



On Thu, 22 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 8/22/19 6:58 PM, Stefano Stabellini wrote:
> > On Mon, 12 Aug 2019, Julien Grall wrote:
> > > The 1:1 mapping may clash with other parts of the Xen virtual memory
> > > layout. At the moment, Xen is handling the clash by only creating a
> > > mapping to the runtime virtual address before enabling the MMU.
> > > 
> > > The rest of the mappings (such as the fixmap) will be mapped after the
> > > MMU is enabled. However, the code doing the mapping is not safe as it
> > > replace mapping without using the Break-Before-Make sequence.
> > > 
> > > As the 1:1 mapping can be anywhere in the memory, it is easier to remove
> > > all the entries added as soon as the 1:1 mapping is not used rather than
> > > adding the Break-Before-Make sequence everywhere.
> > > 
> > > It is difficult to track where exactly the 1:1 mapping was created
> > > without a full rework of create_page_tables(). Instead, introduce a new
> > > function remove_identity_mapping() will look where is the top-level entry
> > > for the 1:1 mapping and remove it.
> > > 
> > > The new function is only called for the boot CPU. Secondary CPUs will
> > > switch directly to the runtime page-tables so there are no need to
> > > remove the 1:1 mapping. Note that this still doesn't make the Secondary
> > > CPUs path safe but it is not making it worst.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > >      It is very likely we will need to re-introduce the 1:1 mapping to
> > > cater
> > >      secondary CPUs boot and suspend/resume. For now, the attempt is to
> > > make
> > >      boot CPU path fully Arm Arm compliant.
> > > 
> > >      Changes in v3:
> > >          - Avoid hardcoding slots
> > > 
> > >      Changes in v2:
> > >          - s/ID map/1:1 mapping/
> > >          - Rename remove_id_map() to remove_identity_mapping()
> > >          - Add missing signed-off-by
> > > ---
> > >   xen/arch/arm/arm64/head.S | 94
> > > +++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 79 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 50cff08756..ec138aae3e 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -33,6 +33,11 @@
> > >   #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1
> > > */
> > >   #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1
> > > */
> > >   +/* Convenience defines to get slot used by Xen mapping. */
> > > +#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
> > > +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> > > +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> > > +
> > >   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
> > >     #define __HEAD_FLAG_PHYS_BASE   1
> > > @@ -301,6 +306,13 @@ real_start_efi:
> > >           ldr   x0, =primary_switched
> > >           br    x0
> > >   primary_switched:
> > > +        /*
> > > +         * The 1:1 map may clash with other parts of the Xen virtual
> > > memory
> > > +         * layout. As it is not used anymore, remove it completely to
> > > +         * avoid having to worry about replacing existing mapping
> > > +         * afterwards.
> > > +         */
> > > +        bl    remove_identity_mapping
> > >           bl    setup_fixmap
> > >   #ifdef CONFIG_EARLY_PRINTK
> > >           /* Use a virtual address to access the UART. */
> > > @@ -626,10 +638,71 @@ enable_mmu:
> > >           ret
> > >   ENDPROC(enable_mmu)
> > >   +/*
> > > + * Remove the 1:1 map for the page-tables. It is not easy to keep track
> >                           ^ from
> 
> I will fix it in the next version.
> 
> > 
> > > + * where the 1:1 map was mapped, so we will look for the top-level entry
> > > + * exclusive to the 1:1 map and remove it.
> > > + *
> > > + * Inputs:
> > > + *   x19: paddr(start)
> > > + *
> > > + * Clobbers x0 - x1
> > > + */
> > > +remove_identity_mapping:
> > > +        /*
> > > +         * Find the zeroeth slot used. Remove the entry from zeroeth
> > > +         * table if the slot is not XEN_ZEROETH_SLOT.
> > 
> > This part of the comment is good
> > 
> > 
> > > +         * For slot XEN_ZEROETH_SLOT, the 1:1 mapping was either done in
> > > first
> > > +         * or second table.
> > 
> > I don't think this sentence is very useful now that the slot is not
> > hard-coded anymore. I would remove it. Instead, I would write something
> > like "The slot XEN_ZEROETH_SLOT is used for the XEN_VIRT_START mapping."
> > The same goes for all the other levels.
> 
> I think this is a bit confusing because one may think the XEN_VIRT_START
> mapping is using the full slot. Whereas it is only partially using it.
> 
> So I would prefer to drop the sentence completely.

That is also OK


> > > +         */
> > > +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
> > > +        cmp   x1, #XEN_ZEROETH_SLOT
> > > +        beq   1f
> > > +        /* It is not in slot XEN_ZEROETH_SLOT, remove the entry. */
> > > +        ldr   x0, =boot_pgtable         /* x0 := root table */
> > > +        str   xzr, [x0, x1, lsl #3]
> > > +        b     identity_mapping_removed
> > > +
> > > +1:
> > > +        /*
> > > +         * Find the first slot used. Remove the entry for the first
> > > +         * table if the slot is not XEN_FIRST_SLOT. For slot
> > > XEN_FIRST_SLOT,
> > > +         * the 1:1 mapping was done in the second table.
> > > +         */
> > > +        lsr   x1, x19, #FIRST_SHIFT
> > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > +        cmp   x1, #XEN_FIRST_SLOT
> > > +        beq   1f
> > > +        /* It is not in slot XEN_FIRST_SLOT, remove the entry. */
> > > +        ldr   x0, =boot_first           /* x0 := first table */
> > > +        str   xzr, [x0, x1, lsl #3]
> > > +        b     identity_mapping_removed
> > > +
> > > +1:
> > > +        /*
> > > +         * Find the second slot used. Remove the entry for the first
> > > +         * table if the slot is not XEN_SECOND_SLOT. For slot
> > > XEN_SECOND_SLOT,
> > > +         * it means the 1:1 mapping was not created.
> > > +         */
> > > +        lsr   x1, x19, #SECOND_SHIFT
> > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > +        cmp   x1, #XEN_SECOND_SLOT
> > > +        beq   identity_mapping_removed
> > > +        /* It is not in slot 1, remove the entry */
> > > +        ldr   x0, =boot_second          /* x0 := second table */
> > > +        str   xzr, [x0, x1, lsl #3]
> > > +
> > > +identity_mapping_removed:
> > > +        /* See asm-arm/arm64/flushtlb.h for the explanation of the
> > > sequence. */
> > > +        dsb   nshst
> > > +        tlbi  alle2
> > > +        dsb   nsh
> > > +        isb
> > 
> > I just want to point out that asm-arm/arm64/flushtlb.h says to use:
> > 
> >   * DSB ISHST        // Ensure prior page-tables updates have completed
> >   * TLBI...          // Invalidate the TLB
> >   * DSB ISH          // Ensure the TLB invalidation has completed
> >   * ISB              // See explanation below
> > 
> > Also implemented as:
> > 
> >      asm volatile(               \
> >          "dsb  ishst;"           \
> >          "tlbi "  # tlbop  ";"   \
> >          "dsb  ish;"             \
> >          "isb;"                  \
> >          : : : "memory");        \
> > 
> > Why is non-shareable enough? Shouldn't it be inner shareable?
> 
> I thought I answered this before. I should have probably clarified in the
> commit message.

I had the feeling you already answered but couldn't find the reference.
Yes please add something to the commit message or an in-code comment,
especially given that tlbflush.h is not updated yet.


> nsh is used (rather than ish) because the TLB flush is local (see page D5-230
> ARM DDI 0487D.a). For convenience here is the text:
> 
> "In all cases in this section where a DMB or DSB is referred to, it
> refers to a DMB or DSB whose required access type is
> both loads and stores. A DSB NSH is sufficient to ensure completion of
> TLB maintenance instructions that apply to a
> single PE. A DSB ISH is sufficient to ensure completion of TLB
> maintenance instructions that apply to PEs in the
> same Inner Shareable domain."
> 
> This is something Linux already does but I wasn't able to find the proper
> justification in the Arm Arm. So I chose a more conservative approach that is
> explained in section K11.5.3 (ARM DDI 0487D.a).
> 
> I have an action to update tlbflush.h before this is in a huge pile of
> cleanup/optimization.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.