Re: [Xen-devel] [PATCH] x86/altp2m: cleanup p2m_altp2m_lazy_copy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/04/2019 20:49, Tamas K Lengyel wrote:
> On Fri, Apr 12, 2019 at 1:44 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> 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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel