Re: [PATCH v2 7/8] xen: arm: use superpages in p2m when pages are suitably aligned

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 13/06/14 15:08, Ian Campbell wrote:
> On Thu, 2014-06-12 at 10:00 +0100, Julien Grall wrote:
>>
>> On 12/06/14 08:30, Ian Campbell wrote:
>>> On Wed, 2014-06-11 at 23:19 +0100, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> While I was looking closer to this patch I found something strange. Why
>>>> all the callers of guest_physmap_add_page in the directory common don't
>>>> check that the function success to create the mapping?
>>>
>>> "directory common"? I don't get your meaning.
>>
>> Sorry, I meant xen/common/
>
> I don't know the answer then.

CC Jan and Keir. I hope one of them have a clue on this.

>
>>>> [..]
>>>>
>>>>> +    case INSERT:
>>>>> +        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
>>>>> +           /* We do not handle replacing an existing table with a superpage */
>>>>> +             (level == 3 || !p2m_table(orig_pte)) )
>>>>> +        {
>>>>> +            /* New mapping is superpage aligned, make it */
>>>>> +            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
>>>>> +            if ( level < 3 )
>>>>
>>>> It's funny, sometimes you use level < 3 and some level != 3 (see in
>>>> ALLOCATE).
>>>
>>> True.
>>>
>>>> I think this pte.p2m.table set can be handled directly in
>>>> mfn_to_p2m_entry. This will avoid duplicating code.
>>>
>>> It can't because mfn_to_p2m_entry is used to create both table and
>>> mapping style entries.
>>
>> There is only on call where we don't override the pte.p2m.table bit (the
>> one at the end of p2m_create table).
>
> All of those other places *conditionally* set table.
>
>> I would move this extra test in the mfn_to_p2m_entry and override only
>> for this specific case.
>
> mfn_to_p2m_entry doesn't know either the level or whether the intention
> of the caller is to create a table or a mapping style entry.

AFAIU, you add/remove every callers of this function in this patch. 
Extending this function (or adding a new helper) would avoid to copy few 
time the same check.

>>>>> -        /* Got the next page */
>>>>> -        addr += PAGE_SIZE;
>>>>> +        rc = apply_one_level(d, &third[third_table_offset(addr)],
>>>>> +                             3, flush_pt, op,
>>>>> +                             start_gpaddr, end_gpaddr,
>>>>> +                             &addr, &maddr, &flush,
>>>>> +                             mattr, t);
>>>>> +        if ( rc < 0 ) goto out;
>>>>
>>>> Shall we redo the whole range if the mapping has failed here?
>>>
>>> s/redo/undo/?
>>
>> Undo sorry.
>
> I'm not sure, you could argue it either way I think. Is Arianna's series
> dealing with this in some way?

I think she handled it only for MMIO. I will have to look again into her 
series.

> Anyway, I think changing that behaviour would be a separate patch.

I didn't intend to ask you to undo the mapping in this patch, nor in 
this series.

I was just wondering what could happen if we fail to map the whole 
mapping. Hence, some callers of guest_physmap_add_page is not checking 
the return of this function. It seems that we may end up to partially 
map the range in guest and crash it.

Anyway,  I don't think it's too bad as the guest will likely fail later 
if we fail to map the region. The only issue is that this won't be 
trivial to find the source as we don't have any error message.

Regards,

-- 
Julien Grall