Re: [PATCH v6 06/38] mm: Add default definition of set_ptes()

From: David Woodhouse
Date: Thu Oct 12 2023 - 09:53:17 EST

On Wed, 2023-08-02 at 16:13 +0100, Matthew Wilcox (Oracle) wrote:
> Most architectures can just define set_pte() and PFN_PTE_SHIFT to
> use this definition.  It's also a handy spot to document the guarantees
> provided by the MM.
>
> Suggested-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Reviewed-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> ---
>  include/linux/pgtable.h | 81 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 60 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index f34e0f2cb4d8..3fde0d5d1c29 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -182,6 +182,66 @@ static inline int pmd_young(pmd_t pmd)
>  }
>  #endif
>  
> +/*
> + * A facility to provide lazy MMU batching.  This allows PTE updates and
> + * page invalidations to be delayed until a call to leave lazy MMU mode
> + * is issued.  Some architectures may benefit from doing this, and it is
> + * beneficial for both shadow and direct mode hypervisors, which may batch
> + * the PTE updates which happen during this window.  Note that using this
> + * interface requires that read hazards be removed from the code.  A read
> + * hazard could result in the direct mode hypervisor case, since the actual
> + * write to the page tables may not yet have taken place, so reads though
> + * a raw PTE pointer after it has been modified are not guaranteed to be
> + * up to date.  This mode can only be entered and left under the protection of
> + * the page table locks for all page tables which may be modified.  In the UP
> + * case, this is required so that preemption is disabled, and in the SMP case,
> + * it must synchronize the delayed page table writes properly on other CPUs.
> + */
> +#ifndef __HAVE_ARCH_ENTER_LAZY_MMU_MODE
> +#define arch_enter_lazy_mmu_mode()     do {} while (0)
> +#define arch_leave_lazy_mmu_mode()     do {} while (0)
> +#define arch_flush_lazy_mmu_mode()     do {} while (0)
> +#endif
> +
> +#ifndef set_ptes
> +#ifdef PFN_PTE_SHIFT
> +/**
> + * set_ptes - Map consecutive pages to a contiguous range of addresses.
> + * @mm: Address space to map the pages into.
> + * @addr: Address to map the first page at.
> + * @ptep: Page table pointer for the first entry.
> + * @pte: Page table entry for the first page.
> + * @nr: Number of pages to map.
> + *
> + * May be overridden by the architecture, or the architecture can define
> + * set_pte() and PFN_PTE_SHIFT.
> + *
> + * Context: The caller holds the page table lock.  The pages all belong
> + * to the same folio.  The PTEs are all in the same PMD.
> + */
> +static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> +               pte_t *ptep, pte_t pte, unsigned int nr)
> +{
> +       page_table_check_ptes_set(mm, ptep, pte, nr);
> +
> +       arch_enter_lazy_mmu_mode();
> +       for (;;) {
> +               set_pte(ptep, pte);
> +               if (--nr == 0)
> +                       break;
> +               ptep++;
> +               pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
> +       }
> +       arch_leave_lazy_mmu_mode();
> +}


This breaks the Xen PV guest.

In move_ptes() in mm/mremap.c we arch_enter_lazy_mmu_mode() and then
loop calling set_pte_at(). Which now (or at least in a few commits time
when you wire it up for x86 in commit a3e1c9372c9b959) ends up in your
implementation of set_ptes(), calls arch_enter_lazy_mmu_mode() again,
and:

[ 0.628700] ------------[ cut here ]------------
[ 0.628718] kernel BUG at arch/x86/kernel/paravirt.c:144!
[ 0.628743] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[ 0.628769] CPU: 0 PID: 1 Comm: init Not tainted 6.5.0-rc4+ #1295
[ 0.628818] RIP: e030:paravirt_enter_lazy_mmu+0x24/0x30
[ 0.628839] Code: 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 65 8b 05 90 28 f9 7e 85 c0 75 10 65 c7 05 81 28 f9 7e 01 00 00 00 c3 cc cc cc cc <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
[ 0.628875] RSP: e02b:ffffc9004000ba48 EFLAGS: 00010202
[ 0.628891] RAX: 0000000000000001 RBX: ffff8880051b7100 RCX: 000ffffffffff000
[ 0.628908] RDX: 80000000763ff967 RSI: 80000000763ff967 RDI: ffff8880051b7100
[ 0.628925] RBP: 80000000763ff967 R08: ffff8880051b6868 R09: 00007ffce1a20000
[ 0.628943] R10: deadbeefdeadf00d R11: 0000000000000000 R12: 00007ffffffff000
[ 0.628964] R13: ffff8880050b7000 R14: 0000000000000001 R15: 00007fffffffe000
[ 0.628988] FS: 0000000000000000(0000) GS:ffff88807b800000(0000) knlGS:0000000000000000
[ 0.629007] CS: e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 0.629024] CR2: ffffc900003f5000 CR3: 0000000003904000 CR4: 0000000000050660
[ 0.629046] Call Trace:
[ 0.629055] <TASK>
[ 0.629066] ? die+0x36/0x90
[ 0.629081] ? do_trap+0xda/0x100
[ 0.629093] ? paravirt_enter_lazy_mmu+0x24/0x30
[ 0.629112] ? do_error_trap+0x6a/0x90
[ 0.629123] ? paravirt_enter_lazy_mmu+0x24/0x30
[ 0.629138] ? exc_invalid_op+0x50/0x70
[ 0.629155] ? paravirt_enter_lazy_mmu+0x24/0x30
[ 0.629169] ? asm_exc_invalid_op+0x1a/0x20
[ 0.629185] ? paravirt_enter_lazy_mmu+0x24/0x30
[ 0.629212] ? pte_offset_map_nolock+0x48/0xc0
[ 0.629226] set_ptes.constprop.0+0xd/0x30
[ 0.629240] move_ptes.isra.0+0xdd/0x290
[ 0.629253] ? pmd_install+0xab/0xd0
[ 0.629267] move_page_tables+0x3a0/0x850
[ 0.629294] shift_arg_pages+0xf4/0x1d0
[ 0.629317] setup_arg_pages+0x205/0x380
[ 0.629330] load_elf_binary+0x398/0xe00


I'm working on making PV kernels testable in qemu. With...

• some qemu fixes and a nasty hackish Xen console implementation:
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv-console
• a CONFIG_PV_SHIM_EXCLUSIVE build of Xen itself to run in the guest,
• some suitable disk image lying around, in ${GUEST_IMAGE}, and
• CONFIG_KVM_XEN enabled in your host kernel,

...you should be able to do something like:

$ ./qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split -drive file=${GUEST_IMAGE},if=none,id=disk -device xen-disk,drive=disk,vdev=xvda -m 1G -kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage -append "loglvl=all -- console=hvc0 root=/dev/xvda1" -display none





Attachment: smime.p7s
Description: S/MIME cryptographic signature