Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure
Hi Hongyan,
On 26/04/2021 10:41, Xia, Hongyan wrote:
On Sun, 2021-04-25 at 21:13 +0100, Julien Grall wrote:
From: Wei Liu
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.hongy...@amazon.com
Signed-off-by: Wei Liu
Signed-off-by: Hongyan Xia
[julien: Adapted for Arm]
Signed-off-by: Julien Grall
[...]
diff --git a/xen/arch/arm/pmap.c b/xen/arch/arm/pmap.c
new file mode 100644
index ..702b1bde982d
--- /dev/null
+++ b/xen/arch/arm/pmap.c
@@ -0,0 +1,101 @@
+#include
+#include
+
+#include
+#include
+#include
+
+/*
+ * To be able to use FIXMAP_PMAP_BEGIN.
+ * XXX: move fixmap definition in a separate header
+ */
+#include
+
+/*
+ * 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.
Good spot! I have updated my tree.
I noticed this line was already present before the Arm version so
probably my fault :(, which also needs to be fixed.
This should be taken care as the next version will create the pmap in
common code :).
+
+ASSERT(system_state < SYS_STATE_smp_boot);
+
+local_irq_save(flags);
+
+idx = find_first_zero_bit(, NUM_FIX_PMAP);
+if ( idx == NUM_FIX_PMAP )
+panic("Out of PMAP slots\n");
+
+__set_bit(idx, );
+
+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 = _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.
This is similar to what I had in mind. Let me give a try for the next
version.
+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, );
+entry = _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.
We could potential define flush_tlb_one_local() on Arm. But, I am not
sure this is worth it because the page table manipulation is mainly
happening in arch code so far. Although, this might change in the future.
+
+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 ..8e1dce93f8e4
--- /dev/null
+++ b/xen/include/asm-arm/pmap.h
@@ -0,0 +1,10 @@
+#ifndef __ASM_PMAP_H__
+#define
Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure
Hi Stefano,
On 15/05/2021 00:25, Stefano Stabellini wrote:
On Sun, 25 Apr 2021, Julien Grall wrote:
From: Wei Liu
+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);
+
+ASSERT(system_state < SYS_STATE_smp_boot);
One small concern here is that we have been using SYS_STATE_early_boot
to switch implementation of things like xen_map_table. Between
SYS_STATE_early_boot and SYS_STATE_smp_boot there is SYS_STATE_boot.
I guess I am wondering if instead of three potentially different mapping
functions (<= SYS_STATE_early_boot, < SYS_STATE_smp_boot, >=
SYS_STATE_smp_boot) we can get away with only two?
This is more flexible than the existing method to map memory when state
== SYS_STATE_early_boot. If you look at the next patch (#13), you will
see that there will be only two method to map memory.
[...]
diff --git a/xen/include/asm-arm/pmap.h b/xen/include/asm-arm/pmap.h
new file mode 100644
index ..8e1dce93f8e4
--- /dev/null
+++ b/xen/include/asm-arm/pmap.h
@@ -0,0 +1,10 @@
+#ifndef __ASM_PMAP_H__
+#define __ARM_PMAP_H__
ASM/ARM
I will fix it.
+/* Large enough for mapping 5 levels of page tables with some headroom */
+#define NUM_FIX_PMAP 8
+
+void *pmap_map(mfn_t mfn);
+void pmap_unmap(const void *p);
+#endif /* __ASM_PMAP_H__ */
Cheers,
--
Julien Grall
Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure
On Sun, 25 Apr 2021, Julien Grall wrote:
> From: Wei Liu
>
> 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]
>
>
> Signed-off-by: Wei Liu
> Signed-off-by: Hongyan Xia
> [julien: Adapted for Arm]
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - New patch
>
> Cc: Jan Beulich
> Cc: Wei Liu
> Cc: Andrew Cooper
> Cc: Roger Pau Monné
> Cc: Hongian Xia
>
> This is mostly a copy of the PMAP infrastructure currently discussed
> on x86. The only difference is how the page-tables are updated.
>
> I think we want to consider to provide a common infrastructure. But
> I haven't done it yet to gather feedback on the overall series
> first.
> ---
> xen/arch/arm/Makefile| 1 +
> xen/arch/arm/mm.c| 7 +--
> xen/arch/arm/pmap.c | 101 +++
> xen/include/asm-arm/config.h | 2 +
> xen/include/asm-arm/lpae.h | 8 +++
> xen/include/asm-arm/pmap.h | 10
> 6 files changed, 123 insertions(+), 6 deletions(-)
> create mode 100644 xen/arch/arm/pmap.c
> create mode 100644 xen/include/asm-arm/pmap.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index ca75f1040dcc..2196ada24941 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -40,6 +40,7 @@ obj-y += percpu.o
> obj-y += platform.o
> obj-y += platform_hypercall.o
> obj-y += physdev.o
> +obj-y += pmap.init.o
> obj-y += processor.o
> obj-y += psci.o
> obj-y += setup.o
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index d090fdfd5994..5e713b599611 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -288,12 +288,7 @@ void dump_hyp_walk(vaddr_t addr)
> dump_pt_walk(ttbr, addr, HYP_PT_ROOT_LEVEL, 1);
> }
>
> -/*
> - * Standard entry type that we'll use to build Xen's own pagetables.
> - * We put the same permissions at every level, because they're ignored
> - * by the walker in non-leaf entries.
> - */
> -static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
> +lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr)
> {
> lpae_t e = (lpae_t) {
> .pt = {
> diff --git a/xen/arch/arm/pmap.c b/xen/arch/arm/pmap.c
> new file mode 100644
> index ..702b1bde982d
> --- /dev/null
> +++ b/xen/arch/arm/pmap.c
> @@ -0,0 +1,101 @@
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * To be able to use FIXMAP_PMAP_BEGIN.
> + * XXX: move fixmap definition in a separate header
> + */
yes please :-)
> +#include
> +
> +/*
> + * 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 */
yep
> +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);
> +
> +ASSERT(system_state < SYS_STATE_smp_boot);
One small concern here is that we have been using SYS_STATE_early_boot
to switch implementation of things like xen_map_table. Between
SYS_STATE_early_boot and SYS_STATE_smp_boot there is SYS_STATE_boot.
I guess I am wondering if instead of three potentially different mapping
functions (<= SYS_STATE_early_boot, < SYS_STATE_smp_boot, >=
SYS_STATE_smp_boot) we can get away with only two?
> +local_irq_save(flags);
> +
> +idx = find_first_zero_bit(, NUM_FIX_PMAP);
> +if ( idx == NUM_FIX_PMAP )
> +panic("Out of PMAP slots\n");
> +
> +__set_bit(idx, );
> +
> +slot = idx + FIXMAP_PMAP_BEGIN;
> +ASSERT(slot >= FIXMAP_PMAP_BEGIN && slot <= FIXMAP_PMAP_END);
> +
> +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().
> + */
> +
Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure
On Sun, Apr 25, 2021 at 09:13:15PM +0100, Julien Grall wrote: > From: Wei Liu > > 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] > > > Signed-off-by: Wei Liu > Signed-off-by: Hongyan Xia > [julien: Adapted for Arm] > Signed-off-by: Julien Grall > > --- > Changes in v2: > - New patch > > Cc: Jan Beulich > Cc: Wei Liu > Cc: Andrew Cooper > Cc: Roger Pau Monné > Cc: Hongian Xia > > This is mostly a copy of the PMAP infrastructure currently discussed > on x86. The only difference is how the page-tables are updated. > > I think we want to consider to provide a common infrastructure. But > I haven't done it yet to gather feedback on the overall series > first. +1. This infrastructure should be common code with arch hooks where appropriate. Wei.
Re: [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure
On Sun, 2021-04-25 at 21:13 +0100, Julien Grall wrote:
> From: Wei Liu
>
> 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.hongy...@amazon.com
> >
>
> Signed-off-by: Wei Liu
> Signed-off-by: Hongyan Xia
> [julien: Adapted for Arm]
> Signed-off-by: Julien Grall
[...]
> diff --git a/xen/arch/arm/pmap.c b/xen/arch/arm/pmap.c
> new file mode 100644
> index ..702b1bde982d
> --- /dev/null
> +++ b/xen/arch/arm/pmap.c
> @@ -0,0 +1,101 @@
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * To be able to use FIXMAP_PMAP_BEGIN.
> + * XXX: move fixmap definition in a separate header
> + */
> +#include
> +
> +/*
> + * 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(, NUM_FIX_PMAP);
> +if ( idx == NUM_FIX_PMAP )
> +panic("Out of PMAP slots\n");
> +
> +__set_bit(idx, );
> +
> +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 = _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, );
> +entry = _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 ..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
