[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 14/19] xen/arm: add Persistent Map (PMAP) infrastructure
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.oNit: 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |