On Tue, Dec 13, 2022 at 09:15:49PM +0000, Julien Grall wrote:
> Hi,
> 
> On 13/12/2022 19:54, Smith, Jackson wrote:
> > Updates xen_pt_update_entry function from xen/arch/arm/mm.c to
> > automatically split superpages as needed.
> Your signed-off-by is missing.
> 
> > ---
> >   xen/arch/arm/mm.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 78 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 6301752..91b9c2b 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -753,8 +753,78 @@ static int create_xen_table(lpae_t *entry)
> >   }
> >   #define XEN_TABLE_MAP_FAILED 0
> > -#define XEN_TABLE_SUPER_PAGE 1
> > -#define XEN_TABLE_NORMAL_PAGE 2
> > +#define XEN_TABLE_NORMAL_PAGE 1
> > +
> > +/* More or less taken from p2m_split_superpage, without the p2m stuff */
> > +static bool xen_split_superpage(lpae_t *entry, unsigned int level,
> > +                                unsigned int target, const unsigned int *offsets)
> > +{
> > +    struct page_info *page;
> > +    lpae_t pte, *table;
> > +    unsigned int i;
> > +    bool rv = true;
> > +
> > +    mfn_t mfn = lpae_get_mfn(*entry);
> > +    unsigned int next_level = level + 1;
> > +    unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level);
> > +
> > +    ASSERT(level < target);
> > +    ASSERT(lpae_is_superpage(*entry, level));
> > +
> > +    page = alloc_domheap_page(NULL, 0);
> Page-table may be allocated from the boot allocator. So you want to use
> create_xen_table().
> 
> > +    if ( !page )
> > +        return false;
> > +
> > +    table = __map_domain_page(page);
> 
> You want to use xen_map_table().
> 
> > +
> > +    /*
> > +     * We are either splitting a first level 1G page into 512 second level
> > +     * 2M pages, or a second level 2M page into 512 third level 4K pages.
> > +     */
> > +    for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> > +    {
> > +        lpae_t *new_entry = table + i;
> > +
> > +        /*
> > +         * Use the content of the superpage entry and override
> > +         * the necessary fields. So the correct permission are kept.
> > +         */
> > +        pte = *entry;
> > +        lpae_set_mfn(pte, mfn_add(mfn, i << level_order));
> > +
> > +        /*
> > +         * First and second level pages set walk.table = 0, but third
> > +         * level entries set walk.table = 1.
> > +         */
> > +        pte.walk.table = (next_level == 3);
> > +
> > +        write_pte(new_entry, pte);
> > +    }
> > +
> > +    /*
> > +     * Shatter superpage in the page to the level we want to make the
> > +     * changes.
> > +     * This is done outside the loop to avoid checking the offset to
> > +     * know whether the entry should be shattered for every entry.
> > +     */
> > +    if ( next_level != target )
> > +        rv = xen_split_superpage(table + offsets[next_level],
> > +                                 level + 1, target, offsets);
> > +
> > +    clean_dcache_va_range(table, PAGE_SIZE);
> 
> Cleaning the cache is not necessary. This is done in the P2M case because it
> is shared with the IOMMU which may not support coherent access.
> 
> > +    unmap_domain_page(table);
> 
> This would be xen_map
> 
> > +
> > +    /*
> > +     * Generate the entry for this new table we created,
> > +     * and write it back in place of the superpage entry.
> > +     */
> 
> I am afraid this is not compliant with the Arm Arm. If you want to update
> valid entry (e.g. shattering a superpage), then you need to follow the
> break-before-make sequence. This means that:
>   1. Replace the valid entry with an entry with an invalid one
>   2. Flush the TLBs
>   3. Write the new entry
> 
> Those steps will make your code compliant but it also means that a virtual
> address will be temporarily invalid so you could take a fault in the middle
> of your split if your stack or the table was part of the region. The same
> could happen for the other running CPUs but this is less problematic as they
> could spin on the page-table lock.

Could this be worked around by writing the critical section in
assembler?  The assembler code would never access the stack and would
run with interrupts disabled.  There could also be BUG() checks for
attempting to shatter a PTE that was needed to access the PTE in
question, though I suspect one can work around this with a temporary
PTE.  That said, shattering large pages requires allocating memory,
which might fail.  What happens if the allocation does fail?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab