Re: [RFC 3/4] Add xen superpage splitting support to arm

[Julien Grall <julien@xxxxxxx>]

Hi Demi,

On 13/12/2022 22:17, Demi Marie Obenour wrote:
> 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? 
Everything is feasible. Is this worth it? I don't think so. There are 
way we can avoid the shattering at first by simply not mapping all the RAM.
> 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?
If this is only done during boot, then I would argue you will want to 
crash Xen.
Cheers,

--
Julien Grall