Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure

["Xia, Hongyan" <hongyxia@xxxxxxxxxx>]

On Sun, 2021-04-25 at 21:13 +0100, Julien Grall wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> The basic idea is like Persistent Kernel Map (PKMAP) in Linux. We
> pre-populate all the relevant page tables before the system is fully
> set up.
> 
> We will need it on Arm in order to rework the arm64 version of
> xenheap_setup_mappings() as we may need to use pages allocated from
> the boot allocator before they are effectively mapped.
> 
> This infrastructure is not lock-protected therefore can only be used
> before smpboot. After smpboot, map_domain_page() has to be used.
> 
> This is based on the x86 version [1] that was originally implemented
> by Wei Liu.
> 
> Take the opportunity to switch the parameter attr from unsigned to
> unsigned int.
> 
> [1] <
> e92da4ad6015b6089737fcccba3ec1d6424649a5.1588278317.git.hongyxia@xxxxxxxxxx
> >
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> [julien: Adapted for Arm]
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

[...]

> diff --git a/xen/arch/arm/pmap.c b/xen/arch/arm/pmap.c
> new file mode 100644
> index 000000000000..702b1bde982d
> --- /dev/null
> +++ b/xen/arch/arm/pmap.c
> @@ -0,0 +1,101 @@
> +#include <xen/init.h>
> +#include <xen/mm.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/flushtlb.h>
> +#include <asm/pmap.h>
> +
> +/*
> + * To be able to use FIXMAP_PMAP_BEGIN.
> + * XXX: move fixmap definition in a separate header
> + */
> +#include <xen/acpi.h>
> +
> +/*
> + * Simple mapping infrastructure to map / unmap pages in fixed map.
> + * This is used to set up the page table for mapcache, which is used
> + * by map domain page infrastructure.
> + *
> + * This structure is not protected by any locks, so it must not be
> used after
> + * smp bring-up.
> + */
> +
> +/* Bitmap to track which slot is used */
> +static unsigned long __initdata inuse;
> +
> +/* XXX: Find an header to declare it */
> +extern lpae_t xen_fixmap[LPAE_ENTRIES];
> +
> +void *__init pmap_map(mfn_t mfn)
> +{
> +    unsigned long flags;
> +    unsigned int idx;
> +    vaddr_t linear;
> +    unsigned int slot;
> +    lpae_t *entry, pte;
> +
> +    BUILD_BUG_ON(sizeof(inuse) * BITS_PER_LONG < NUM_FIX_PMAP);

This seems wrong to me. It should multiply with something like
BITS_PER_BYTE.

I noticed this line was already present before the Arm version so
probably my fault :(, which also needs to be fixed.

> +
> +    ASSERT(system_state < SYS_STATE_smp_boot);
> +
> +    local_irq_save(flags);
> +
> +    idx = find_first_zero_bit(&inuse, NUM_FIX_PMAP);
> +    if ( idx == NUM_FIX_PMAP )
> +        panic("Out of PMAP slots\n");
> +
> +    __set_bit(idx, &inuse);
> +
> +    slot = idx + FIXMAP_PMAP_BEGIN;
> +    ASSERT(slot >= FIXMAP_PMAP_BEGIN && slot <= FIXMAP_PMAP_END);
> +

From here...

> +    linear = FIXMAP_ADDR(slot);
> +    /*
> +     * We cannot use set_fixmap() here. We use PMAP when there is no
> direct map,
> +     * so map_pages_to_xen() called by set_fixmap() needs to map
> pages on
> +     * demand, which then calls pmap() again, resulting in a loop.
> Modify the
> +     * PTEs directly instead. The same is true for pmap_unmap().
> +     */
> +    entry = &xen_fixmap[third_table_offset(linear)];
> +
> +    ASSERT(!lpae_is_valid(*entry));
> +
> +    pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
> +    pte.pt.table = 1;
> +    write_pte(entry, pte);
> +

...to here, I wonder if we can move this chunk into arch (like void
*arch_write_pmap_slot(slot)). Such an arch function hides how fixmap is
handled and how page table entry is written behind arch, and the rest
can just be common.

> +    local_irq_restore(flags);
> +
> +    return (void *)linear;
> +}
> +
> +void __init pmap_unmap(const void *p)
> +{
> +    unsigned long flags;
> +    unsigned int idx;
> +    lpae_t *entry;
> +    lpae_t pte = { 0 };
> +    unsigned int slot = third_table_offset((vaddr_t)p);
> +
> +    ASSERT(system_state < SYS_STATE_smp_boot);
> +    ASSERT(slot >= FIXMAP_PMAP_BEGIN && slot <= FIXMAP_PMAP_END);
> +
> +    idx = slot - FIXMAP_PMAP_BEGIN;
> +    local_irq_save(flags);
> +
> +    __clear_bit(idx, &inuse);
> +    entry = &xen_fixmap[third_table_offset((vaddr_t)p)];
> +    write_pte(entry, pte);
> +    flush_xen_tlb_range_va_local((vaddr_t)p, PAGE_SIZE);

and the same for the above, something like arch_clear_pmap(void *) and
the rest into common.

From a quick glance, I don't think x86 and Arm share any useful TLB
flush helpers? So the TLB flush probably should be behind arch as well.

> +
> +    local_irq_restore(flags);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

[...]

> diff --git a/xen/include/asm-arm/pmap.h b/xen/include/asm-arm/pmap.h
> new file mode 100644
> index 000000000000..8e1dce93f8e4
> --- /dev/null
> +++ b/xen/include/asm-arm/pmap.h
> @@ -0,0 +1,10 @@
> +#ifndef __ASM_PMAP_H__
> +#define __ARM_PMAP_H__

This line doesn't seem to match the #ifndef, but if the functions are
moved to common, this header can be moved to common as well.

Hongyan