Page MenuHomeFreeBSD

Fix assert in powerpc pmaps after introduction of object busy.
ClosedPublic

Authored by kib on Oct 15 2019, 4:25 PM.

Details

Summary

The VM_PAGE_OBJECT_BUSY_ASSERT() in pmap_enter() implementation should be only asserted when the code is executed as result of pmap_enter(), not when the same code is entered from e.g. pmap_enter_quick(). This is relevant for all PowerPC pmap variants, because mmu_*_enter() is used as the backend, and assert is located there.

Add a ppc private pmap_enter() flag to indicate that the call is not from pmap_enter().

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Oct 15 2019, 4:25 PM
markj accepted this revision.Oct 15 2019, 4:42 PM

I don't see a problem with the diff as-is, but I believe we can also assert that m->object is locked in the PMAP_ENTER_QUICK_LOCKED paths.

This revision is now accepted and ready to land.Oct 15 2019, 4:42 PM
bdragon added inline comments.Oct 15 2019, 5:29 PM
sys/powerpc/booke/pmap.c
2282 ↗(On Diff #63302)

flags is uninitialized in this case.

bdragon added inline comments.Oct 15 2019, 5:31 PM
sys/powerpc/booke/pmap.c
2282 ↗(On Diff #63302)

should be pmap_flags maybe?

bdragon requested changes to this revision.Oct 15 2019, 5:50 PM

booke compile errors.

This revision now requires changes to proceed.Oct 15 2019, 5:50 PM
kib updated this revision to Diff 63312.Oct 15 2019, 5:51 PM
kib marked 2 inline comments as done.

Update flags arg name for booke.
Assert that the object is locked if !quick.

I don't see an actual change for flags -> pmap_flags in mmu_booke_enter_locked(), did you upload the right diff?

kib updated this revision to Diff 63314.Oct 15 2019, 6:23 PM

Really update used var.

alc added a comment.Oct 15 2019, 7:05 PM

Why not use PMAP_ENTER_NOSLEEP instead of introducing a new flag?

alc added a comment.Oct 15 2019, 7:27 PM

If my recollection of the sparc64 pmap is correct, it will have the same issue.

bdragon accepted this revision.Oct 15 2019, 7:28 PM

X5000: Gets further, hangs with witness lock held too long on usbd_ctrl_lock during "sh /etc/rc autoboot" (Probably a separate issue)
POWER9: OK
G5: OK
RB800: Not tested yet
G4: Not tested yet

This revision is now accepted and ready to land.Oct 15 2019, 7:28 PM
kib added a comment.Oct 15 2019, 7:54 PM
In D22041#481448, @alc wrote:

Why not use PMAP_ENTER_NOSLEEP instead of introducing a new flag?

Because PMAP_ENTER_NOSLEEP is used in vm_fault_soft_fast(), where we very much want to assert that the object is busy.

kib added a comment.Oct 15 2019, 7:54 PM
In D22041#481451, @alc wrote:

If my recollection of the sparc64 pmap is correct, it will have the same issue.

Thank you for the hint, I will handle this after ppc is done.

G4: panic: vm_page_free_prep: mapping flags set in page 0xd032c9f8

markj added a comment.Oct 15 2019, 8:45 PM

G4: panic: vm_page_free_prep: mapping flags set in page 0xd032c9f8

Can you "show pginfo 0xd032c9f8" from DDB? In particular, which of PGA_EXECUTABLE and PGA_WRITEABLE is set?

screenshot of the backtrace at http://drop.rtk0.net/IMG_1255.JPG

I don't have keyboard access to actually run commands unless I lock out the framebuffer. Trying to remember the magic boot args to do so...

ok, rebuilt with nooptions vt so the ofw console stays up.

panic: vm_page_free_prep: mapping flags set in page 0xd1885a98

page 0xd1885a98 obj 0 pidx 0x1e phys 0x4e19d000 q 1 ref 0
  af 0x9 of 0x0 f 0x1 act 5 busy 1 valid 0xff dirty 0xff

ok, rebuilt with nooptions vt so the ofw console stays up.

panic: vm_page_free_prep: mapping flags set in page 0xd1885a98
page 0xd1885a98 obj 0 pidx 0x1e phys 0x4e19d000 q 1 ref 0
  af 0x9 of 0x0 f 0x1 act 5 busy 1 valid 0xff dirty 0xff

I believe this is fixed by D22044.

G4 appears to be working now when using markj's patch on top of this.

X5000: OK with one caveat -- I currently have to disable ehci.

alc accepted this revision.Oct 16 2019, 4:49 AM
This revision was automatically updated to reflect the committed changes.