Re: [Xen-devel] [PATCH v3 33/38] arm/p2m: Add altp2m paging mechanism

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien


[...]

>>>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>>>                                        const union hsr hsr)
>>>  {
>>> @@ -2445,6 +2465,14 @@ static void do_trap_instr_abort_guest(struct
>>> cpu_user_regs *regs,
>>>          break;
>>>      case FSC_FLT_TRANS:
>>>          /*
>>> +         * The guest shall retry accessing the page if the altp2m
>>> handler
>>> +         * succeeds. Otherwise, we continue injecting an instruction
>>> abort
>>> +         * exception.
>>> +         */
>>> +        if ( try_handle_altp2m(current, gpa, npfec) )
>>> +            return;
>> I would have expected that altp2m is the last thing we want to check
>> in the abort handler. I.e after p2m_lookup.
>>
> Alright, I will reorder both calls, thank you.

Actually, reordering of these calls (try_handle_altp2m/altp2m_lazy_copy
and p2m_lookup) would make the system stall if altp2m is active. This is
because the p2m_lookup routine considers only the host's p2m, which will
most likely return a mfn != INVALID_MFN and thus entirely skip the call
to altp2m_lazy_copy. Thus, both calls should rather be conditional as
shown in the following:

if ( likely(!altp2m_active(current->domain)) )
{
    /*
     * The PT walk may have failed because someone was playing
     * with the Stage-2 page table. Walk the Stage-2 PT to check
     * if the entry exists. If it's the case, return to the guest
     */
    mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL);
    if ( !mfn_eq(mfn, INVALID_MFN) )
        return;
}
else
    /*
     * The guest shall retry accessing the page if the altp2m handler
     * succeeds. Otherwise, we continue injecting an instruction abort
     * exception.
     */
    if ( altp2m_lazy_copy(current, _gfn(paddr_to_pfn(gpa))) )
        return;


Please excuse me for the confusion.

>>> +
>>> +        /*
>>>           * The PT walk may have failed because someone was playing
>>>           * with the Stage-2 page table. Walk the Stage-2 PT to check
>>>           * if the entry exists. If it's the case, return to the guest
>>> @@ -2547,6 +2575,13 @@ static void do_trap_data_abort_guest(struct
>>> cpu_user_regs *regs,
>>>          }
>>>
>>>          /*
>>> +         * The guest shall retry accessing the page if the altp2m
>>> handler
>>> +         * succeeds. Otherwise, we continue injecting a data abort
>>> exception.
>>> +         */
>>> +        if ( try_handle_altp2m(current, info.gpa, npfec) )
>>> +            return;
>> Ditto here.
>>

Same here.

Cheers,
~Sergej


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel