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
