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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/02/2022 15:22, Jan Beulich wrote:
> 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.
Doh. I have moved the insertion.

> > --- /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?
Yes. I should reword to:

"This is used to set the page table before the map domain page 
infrastructure is initialized".
> > + * 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().
I agree. I will have a look to use DECLARE_BITMAP().

> > +    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.
This comment is still accurate for Arm. I also expect it to be accurate 
for all architectures because set_fixmap() is likely going to be 
implemented with generic PT helpers.
So I think it makes sense to keep it in common code. This explains why 
we are calling arch_pmap_map() rather than set_fixmap() directly.
> > +    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?
Hmmm... This patch has been through various revision before me. I went 
through the archives and couldn't tell why local_irq_restore() was added.
Looking at the code, none of the Xen page-table helpers expect to be 
called from interrupt context. So I am thinking to replace with an 
ASSERT/BUG_ON !in_irq().
Cheers,

--
Julien Grall