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

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

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/

>>> +                return P2M_ONE_PROGRESS;
>>> +            }
>>> +            else if ( level == 3 )
>>> +                return -ENOMEM;
>>> +        }
>>> +
>>> +        BUG_ON(level == 3); /* L3 is always superpage aligned */
>>
>> Did you mean page-aligned?
>
> What I meant was that an L3 entry is always "superpage aligned" because
> it is the smallest possible element. Since I wrote that I renamed my
> is_superpage_aligned function to is_mapping_aligned. I should perhaps
> update this comment to reflect that, which would make it clearer.

Oh ok. In my mind, L3 is using page alignment not superpage alignment. I 
think was confuse because in your cover letter you define superpage as 
2M or 1G mappings.

>> [..]
>>
>>> +    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).

I would move this extra test in the mfn_to_p2m_entry and override only 
for this specific case.

>> [..]
>>
>>> +        }
>>> +        else
>>> +        {
>>> +            /* New mapping is not superpage aligned, create a new table entry */
>>> +            BUG_ON(level == 3); /* L3 is always superpage aligned */
>>
>> Did you mean page-aligned?
>>
>> [..]
>>
>>> -        /* 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.

Regards,

-- 
Julien Grall