Re: [Xen-devel] [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/06/2016 03:52 PM, Julien Grall wrote:
>
>
> On 06/08/2016 12:26, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>> On 08/04/2016 04:50 PM, Julien Grall wrote:
>>> On 01/08/16 18:10, Sergej Proskurin wrote:
>>>> +
>>>> +        /* Check for a dropped page that may impact this altp2m. */
>>>> +        if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&
>>>
>>> Why the check to p2mt against p2m_invalid?
>>>
>>
>> I have encountered p2m entries or rather calls to p2m_apply_change with
>> valid MFN's, however, invalid types. That is, a page drop would not be
>> recognized if checked only against an invalid MFN.
>
> Because currently REMOVE operation has the MFN valid to be able to
> check whether the wanted mapping has been removed.
>
> However, I don't think this is safe to assume that p2m_invalid will
> always meant "REMOVE". It is actually used in different case.
>

Alright. To cope with this issue, I could simply check for INSERT/REMOVE
operations before calling altp2m_propagate_change. Then, it should
indeed be sufficient to check only for an invalid mfn. Thank you.

>>
>>>> +             gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
>>>> +             gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
>>>> +        {
>>>> +            if ( !reset_count++ )
>>>> +            {
>>>> +                altp2m_reset(p2m);
>>>> +                last_reset_idx = i;
>>>> +            }
>>>> +            else
>>>> +            {
>>>> +                /* At least 2 altp2m's impacted, so reset
>>>> everything. */
>>>
>>> So if you remove a 4KB page in more than 2 altp2m, you will flush all
>>> the p2m. This sounds really more time consuming (you have to free all
>>> the intermediate page table) than removing a single 4KB page.
>>
>> I agree: The solution has been directly adopted from the x86
>> implementation. It needs further design reconsideration. However, at
>> this point we would like to finish the overall architecture for ARM
>> before we go into further reconstructions.
>>
>>>
>>>> +                for ( i = 0; i < MAX_ALTP2M; i++ )
>>>> +                {
>>>> +                    if ( i == last_reset_idx ||
>>>> +                         d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
>>>> +                        continue;
>>>> +
>>>> +                    p2m = d->arch.altp2m_p2m[i];
>>>> +                    altp2m_reset(p2m);
>>>> +                }
>>>> +                goto out;
>>>> +            }
>>>> +        }
>>>> +        else if ( !mfn_eq(m, INVALID_MFN) )
>>>> +            modify_altp2m_range(d, p2m, sgfn, nr, smfn,
>>>> +                                mask, p2mt, p2ma);
>>>
>>> I am a bit concerned about this function. We decided to limit the size
>>> of the mapping to avoid long running memory operations (see XSA-158).
>>>
>>> With this function you multiply up to 10 times the duration of the
>>> operation.
>>>
>>
>> I see your point. However, on an active system, adaptions of the hostp2m
>> (while altp2m is active) should be very limited. Another solution would
>> be to simply flush the altp2m views on every hostp2m modification. IMO
>> this, however, would not really be a huge performance gain due to
>> de-allocation and freeing of the altp2m tables. Do you have another
>> idea?
>
> I would need to have a think. However, it is wrong to assume that
> change of hostp2m will be limited when altp2m is active. A guest is
> free to increase/decrease the memory reservation.
>
> You always need to consider the worth case and not the best case (i.e
> a guest behaving nicely).

Fair enough.

Best regards,
~Sergej


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