amd64: make uiomove_fromphys functional for pages not mapped by DMAP

[emaste]

Summary

Place the code introduced in r268660 into a separate function that can be
called from uiomove_fromphys. Make the kva pages per-cpu in order to reduce
contention and allow parallel operations on pages not covered by the DMAP.
Also introduce a safety catch in PHYS_TO_DMAP and DMAP_TO_PHYS.

Sponsored by: Citrix Systems R&D

amd64/amd64/pmap.c:

• Introduce a boot-time hook to initialize the per-cpu variables.
• Factor out the code to deal with non DMAP addresses from pmap_copy_pages and place it in pmap_get_vaddr.

amd64/amd64/uio_machdep.c:

• Use pmap_get_vaddr in order to correctly deal with physical addresses not covered by the DMAP.

amd64/include/pmap.h:

• Add the prototypes for the new functions.

amd64/include/vmparam.h:

• Add safety catches to make sure PHYS_TO_DMAP and DMAP_TO_PHYS are only used with addresses covered by the DMAP.

[kib]

sys/amd64/amd64/pmap.c

396

You use sx because uiomove_fromphys() may fault while keeping the address allocated, right ? I am not sure could it recurse the lock in page fault.

Also, this lock is held for unbound time. E.g., nfs server could stop responding while pager requests the page content, and the situation results in the lock cascade for unlucky unreated threads which happen to execute on the same CPU as the thread initiated the pagein.

397

Using DPCPU for something that cannot be loaded as module is excessive. PMAP traditionally adds the required members to static pcpu structure.

Looking at big picture, does the non-DMAP mapped access is so contended that it makes the per-cpu scratch page frames reasonable ?

6874

Do you need PG_G (global) bit set for the mapping ?

sys/amd64/include/vmparam.h

190

Using inlines instead of gcc extension seems more clean.

[royger]

sys/amd64/amd64/pmap.c

396

Yes, a sx lock is needed since uiomove_fromphys may fault. So far I haven't seen it recurse, and looking at the paths in the vm fault handler it doesn't look like (although it's quite convoluted, so I wouldn't be surprised that I've missed something).

Maybe it would be better to get rid of the pcpu pages/lock and dynamically alloc them using kva_alloc/kva_free?

397

OK, I will add them to the static pcpu structure then. I've just used DPCPU because it seems cleaner to have this variables declared in the file where they are used.

When running FreeBSD as Dom0 physical addresses outside of the memory map are used to map memory from other domains (remember the privcmd driver). This is used to map memory used by the guest to perform IO operations, and also to map all the guest memory during save/restore/migration. If there are several HVM guests performing heavy IO this is going to be quite contented.

6874

No, I've switched to using vtopte and pte_store instead of pmap_kenter_attr.

sys/amd64/include/vmparam.h

190

If I convert them to inline functions I have to at least add the sys/types.h, vm/vm.h and vm/pmap.h headers, which is not very nice. IMHO I think it's best to use macros here.

[royger]

• Remove the usage of pcpu pages/locks and instead allocate the KVA pages on demand using vmem_alloc. This prevents holding the lock for unbound time.
• Do not set PG_G for the temporary mappings.

[kib]

sys/amd64/amd64/pmap.c

6870

I suggest to not call vmem_alloc() while pinned. The loop could be split into two,
first phase would do the allocations, then we pin the curthread, and then second loop would initialize ptes. Acquiring locks while pinned could cause starvation.

6891

I think we can safely unpin before freeing vmem allocations.

sys/amd64/amd64/uio_machdep.c

88–89

Hmm, again, you exchanged the sx lock to pin, while serving page fault. I prefer not to do this.

A possible solution is to add a flag to pmap_get_vaddr() to request global invalidation of the mapped address (and set PG_G on pte, which means that you can use pmap_qenter()), and set the flag when called from uiomove_fromphys().

[royger]

Thanks for the review.

Fixed the comments from the last version:

• Use two different loops in pmap_get_vaddr to avoid calling vmem_alloc with the thread pinned.
• If there's the possiblity of taking a page fault while mappings are active use global mappings and don't pin the thread.
• In pmap_remove_vaddr unpin (if needed) and then perform the cleanup actions.

[kib]

Overall, I think the patch is completed (small things I listed above are trivial).

My only big request is to add some big block comment before the pmap_get_vaddr() describing the function, and explaining the interface.

BTW, doesn't vaddr sound somewhat Linux'ish ?

sys/amd64/amd64/pmap.c

6872

This could be replaced with return (FALSE);. It simplifies the logic and avoids unneeded goto (no, I do not hate goto, but this one only complicates flow control, IMO).

sys/amd64/amd64/uio_machdep.c

68

Style requires avoiding initialization in declarations.

88–89

For UIO_NOCOPY, we can avoid mapping at all.

sys/amd64/include/vmparam.h

189

What about making PHYS_TO_DMAP() and DMAP_TO_PHYS() inlines ? I might have already asked about this, sorry then.

[royger]

Thanks for the review.

Regarding the name, maybe pmap_map_phys/pmap_unmap_phys sounds better? I'm not really good at naming TBH.

I will fix the nits and add a comment describing the usage, and unless you have other comments commit it.

sys/amd64/amd64/pmap.c

6872

I originally had a simple return here, but then I thought I could use a single exit point. Since I don't have a strong opinion I will change it to a return.

sys/amd64/amd64/uio_machdep.c

68

Done.

88–89

Yes, will gate the call to get_vaddr.

sys/amd64/include/vmparam.h

189

Yes, you already asked :). Here is/was my reply (not sure how to link to a specific comment, so I'm just copying it):

If I convert them to inline functions I have to at least add the sys/types.h, vm/vm.h and vm/pmap.h headers, which is not very nice. IMHO I think it's best to use macros here.

[kib]

May be, name it pmap_map_io_transient ? Or some contraction of the 'transient'.

[royger]

I didn't know/find any contraction of 'transient', so I've used it as-is. Committed as r273582, thanks for the review.

[emaste]

UEFI boot fails after this change

[emaste]

sys/amd64/include/vmparam.h

189

I found that reverting r273582 but applying just the KASSERT changes from here makes the UEFI boot fail the same way.

I've been discussing with kib on IRC and he identified the phys framebuffer access from vt(4) efifb - i.e. vt_efifb_init

[emaste]

Problem appears to be vt(4) efifb's use of PHYS_TO_DMAP too early in the boot (vt_efifb_init), although the resulting DMAP address is not accessed at that point.

[emaste]

Re-close as the UEFI issue is not a problem with this change, per se.