Re: [PATCH v3 14/19] xen/arm: add Persistent Map (PMAP) infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2022 11:22, Julien Grall wrote:
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -28,6 +28,7 @@ obj-y += multicall.o
>  obj-y += notifier.o
>  obj-y += page_alloc.o
>  obj-$(CONFIG_HAS_PDX) += pdx.o
> +obj-bin-$(CONFIG_HAS_PMAP) += pmap.init.o
>  obj-$(CONFIG_PERF_COUNTERS) += perfc.o
>  obj-y += preempt.o
>  obj-y += random.o

Nit: Please move the insertion one line further down.

> --- /dev/null
> +++ b/xen/common/pmap.c
> @@ -0,0 +1,79 @@
> +#include <xen/bitops.h>
> +#include <xen/init.h>
> +#include <xen/pmap.h>
> +
> +#include <asm/pmap.h>
> +#include <asm/fixmap.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.

Is this comment stale from its original x86 purpose?

> + * 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;

I guess this wants to use DECLARE_BITMAP(), for ...

> +void *__init pmap_map(mfn_t mfn)
> +{
> +    unsigned long flags;
> +    unsigned int idx;
> +    unsigned int slot;
> +
> +    BUILD_BUG_ON(sizeof(inuse) * BITS_PER_BYTE < NUM_FIX_PMAP);
> +
> +    ASSERT(system_state < SYS_STATE_smp_boot);
> +
> +    local_irq_save(flags);
> +
> +    idx = find_first_zero_bit(&inuse, NUM_FIX_PMAP);

... this to be correct irrespective of how large NUM_FIX_PMAP is?
I think that's preferable over the BUILD_BUG_ON().

> +    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);
> +
> +    /*
> +     * 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().
> +     */
> +    arch_pmap_map(slot, mfn);

I'm less certain here, but like above I'm under the impression
that this comment may no longer be accurate.

> +    local_irq_restore(flags);

What is this IRQ save/restore intended to protect against, when
use of this function is limited to pre-SMP boot anyway?

Jan