On 12/04/2019 20:49, Tamas K Lengyel wrote: > On Fri, Apr 12, 2019 at 1:44 PM Andrew Cooper <[email protected]> > wrote: >>> + p2m_lock(ap2m); >>> >>> - mfn = get_gfn_type_access(hp2m, gfn_x(gfn), &p2mt, &p2ma, >>> - P2M_ALLOC, &page_order); >>> - __put_gfn(hp2m, gfn_x(gfn)); >>> + amfn = __get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, >>> + 0, NULL, false); >>> >>> - if ( mfn_eq(mfn, INVALID_MFN) ) >>> + /* Bail if entry is already in altp2m or there is no entry is hostp2m >>> */ >> This comment then reduces to: >> >> /* Entry already present in ap2m? Caller should handle the fault. */ >> >>> + if ( !mfn_eq(amfn, INVALID_MFN) || mfn_eq(hmfn, INVALID_MFN) ) >>> + { >>> + p2m_unlock(ap2m); >>> return 0; >>> - >>> - p2m_lock(*ap2m); >>> + } >>> >>> /* >>> * If this is a superpage mapping, round down both frame numbers >>> * to the start of the superpage. >>> */ >>> mask = ~((1UL << page_order) - 1); >>> - mfn = _mfn(mfn_x(mfn) & mask); >>> - gfn = _gfn(gfn_x(gfn) & mask); >>> + hmfn = _mfn(mfn_x(hmfn) & mask); >>> + gfn = _gfn(gfn_l & mask); >>> >>> - rv = p2m_set_entry(*ap2m, gfn, mfn, page_order, p2mt, p2ma); >>> - p2m_unlock(*ap2m); >>> + rv = p2m_set_entry(ap2m, gfn, hmfn, page_order, hp2mt, hp2ma); >>> + p2m_unlock(ap2m); >>> >>> if ( rv ) >>> { >>> gdprintk(XENLOG_ERR, >>> - "failed to set entry for %#"PRIx64" -> %#"PRIx64" p2m >>> %#"PRIx64"\n", >>> - gfn_x(gfn), mfn_x(mfn), (unsigned long)*ap2m); >>> - domain_crash(hp2m->domain); >>> + "failed to set entry for %#"PRIx64" -> %#"PRIx64" p2m >>> %#"PRIx64"\n", >>> + gfn_l, mfn_x(hmfn), (unsigned long)ap2m); >> This should be a gprintk(), not a gdprintk(), because there is nothing >> more infuriating in release builds than a domain_crash() with no >> qualification. >> >> Printing the virtual address of the ap2m is also useless as far as >> diagnostics go. How about something like: >> >> "Failed to lazy copy gfn %"PRI_gfn" -> %"PRI_mfn" (type %u, access %u, >> order %u) for view %u\n" >> >> if there is an easy way to access the view id. This at least stands a >> chance of being useful when debugging. > Yeap, makes sense.
Oh, and with rv as well so we can get some hint as to why the set_entry() failed. As you're touching every site, how about renaming it to ret or rc for consistency? ~Andrew _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
