Re: [Xen-devel] [PATCH v5 10/15] x86/altp2m: add remaining support routines.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Monday, July 20, 2015 11:38 PM
>
>>>>>>>> +void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
>>>>>>>> +                                 mfn_t mfn, unsigned int page_order,
>>>>>>>> +                                 p2m_type_t p2mt, p2m_access_t
>>>>>>>> +p2ma) {
>>>>>>>> +    struct p2m_domain *p2m;
>>>>>>>> +    p2m_access_t a;
>>>>>>>> +    p2m_type_t t;
>>>>>>>> +    mfn_t m;
>>>>>>>> +    uint16_t i;
>>>>>>>> +    bool_t reset_p2m;
>>>>>>>> +    unsigned int reset_count = 0;
>>>>>>>> +    uint16_t last_reset_idx = ~0;
>>>>>>>> +
>>>>>>>> +    if ( !altp2m_active(d) )
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    altp2m_list_lock(d);
>>>>>>>> +
>>>>>>>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>>>>> +    {
>>>>>>>> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
>>>>>>>> +            continue;
>>>>>>>> +
>>>>>>>> +        p2m = d->arch.altp2m_p2m[i];
>>>>>>>> +        m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0,
>>>>>>>> + NULL);
>>>>>>>> +
>>>>>>>> +        reset_p2m = 0;
>>>>>>>> +
>>>>>>>> +        /* Check for a dropped page that may impact this altp2m */
>>>>>>>> +        if ( mfn_x(mfn) == INVALID_MFN &&
>>>>>>>> +             gfn_x(gfn) >= p2m->min_remapped_gfn &&
>>>>>>>> +             gfn_x(gfn) <= p2m->max_remapped_gfn )
>>>>>>>> +            reset_p2m = 1;
>>>>>>>
>>>>>>>Considering that this looks like an optimization, what's the
>>>>>>>downside of possibly having min=0 and max=<end-of-address-
>space>?
>>>I.e.
>>>>>>>can there a long latency operation result that's this way a guest
>>>>>>>can
>>>effect?
>>>>>>>
>>>>>>
>>>>>> ... A p2m is a gfn->mfn map, amongst other things. There is a
>>>>>> reverse
>>>>>> mfn->gfn map, but that is only valid for the host p2m. Unless the
>>>>>> remap altp2m hypercall is used, the gfn->mfn map in every altp2m
>>>>>> mirrors the gfn->mfn map in the host p2m (or a subset thereof, due
>>>>>> to lazy-copy), so handling removal of an mfn from a guest is simple:
>>>>>> do a reverse look up for the host p2m and mark the relevant gfn as
>>>>>> invalid, then do the same for every altp2m where that gfn is
>>>>>> currently
>>>valid.
>>>>>>
>>>>>> Remap changes things: it says take gfn1 and replace ->mfn with the
>>>>>> ->mfn of gfn2. Here is where the optimization is used and the
>>>>>> ->invalidate
>>>>>logic is:
>>>>>> record the lowest and highest gfn2's that have been used in remap
>>>>>> ops; if an mfn is dropped from the hostp2m, for the purposes of
>>>>>> altp2m invalidation, see if the gfn derived from the host p2m
>>>>>> reverse lookup falls within the range of used gfn2's. If it does,
>>>>>> an invalidation is required. Which is why min and max are inited
>>>>>> the way they are - hope the explanation clarifies this optimization.
>>>>>
>>>>>Sadly it doesn't, it just re-states what I already understood and
>>>>>doesn't answer the question: What happens if min=0 and
>>>>>max=<end-of-address-
>>>>>space>? I.e. can the guest nullify the optimization by careful
>>>>>space>fiddling
>>>> issuing
>>>>>some of the new hypercalls, and if so will this have any negative
>>>>>impact on
>>>> the
>>>>>hypervisor? I'm asking this from a security standpoint ...
>>>>>
>>>>
>>>> To take that exact case, If min=0 and max=<end of address space>
>>>> then any hostp2m change where the first mfn is dropped, will cause
>>>> all altp2ms to be reset even if the mfn dropped doesn't affect
>>>> altp2ms at all, which wont serve as an optimization at all - Hope that
>clarifies.
>>>
>>>Again - no. I understand the optimization is gone then. But what's the
>effect?
>>>I.e. will the guest, by extending this range to be arbitrarily wide,
>>>be able
>> to
>>>cause a long latency hypervisor operation (i.e. a DoS)?
>>>
>>
>> The extent of the range affects the likelihood of an invalidation. It
>> has no impact on the cost of an invalidation (so no its not a DoS issue).
>> I'm not sure what change you are suggesting here or just clarification
>> (if you think this optimization is confusing perhaps some
>> documentation of this optimization will help?)
>
>Well, the optimization must be optimizing _something_. And hence
>_something_ must go sub-optimal when the optimization is being subverted.
>And the question is how much worse un-optimized is compared to optimized.
>
>It _looks like_ the overall effect really is just to avoid a one time (for a 
>given
>non-preemptible operation) reset, but whether that's really the case depends
>on the calling contexts (which, as said a couple of times before, is hard to 
>see
>for a patch that introduces functions without callers - hence the question).
>

As you now understand, invalidating an altp2m effectively resets it to be a 
(lazily-copied) exact duplicate of the host p2m again -- so losing any altp2m 
permissions restrictions or remaps. This is a first cut at minimizing the 
likelihood of that happening unnecessarily. There's some discussion on this 
first cut between Tim and Ed going back to February or March. The intention 
continues to be that this might be further optimized if real-world use shows 
this first cut to be inefficient.

>>>>>Nor do I find my question answered why max can't be initialized to zero:
>>>>>You don't care whether max is a valid GFN when a certain GFN doesn't
>>>>>fall in the (then empty) [min, max] range. What am I missing?
>>>>
>>>> Since 0 is a valid GFN so we cannot initialize min or max to 0 -
>>>> since matching this condition (gfn_x(gfn) >= p2m->min_remapped_gfn
>>>> &&
>>>> gfn_x(gfn) <=
>>>> p2m->max_remapped_gfn) will cause a reset (throw away) of the altp2m
>>>> p2m->to
>>>> rebuild it from the hostp2m. So essentially what is being done here
>>>> is the range is the non-existent set to start with, unless some
>>>> altp2m changes occur, and then it is grown to be the smallest set
>>>> around the gfns
>>>affected.
>>>
>>>Again you just re-state what was already clear, yet you neglect
>>>answering the actual question. Taking what you wrote above, when max=0
>>>(and min=INVALID_GFN), then
>>>
>>>     gfn_x(gfn) >= p2m->min_remapped_gfn &&
>>>      gfn_x(gfn) <= p2m->max_remapped_gfn
>>>
>>>will be false for any value of gfn; in fact the "max" part won't even
>>>be
>> looked
>>>at because the "min" part will already be false for any valid gfn,
>>>i.e. only
>> the
>>>INVALID_GFN case would make it to the "max" part.
>>>
>>
>> You are suggesting an alternative which will work, but what we have
>> will also produce the same results, and the results are correct for
>> both cases - so can we go with the approach we have taken currently?
>
>Of course we can, but should we? I.e. why use sub-optimal code when there
>clearly is a way to improve it? Counter question - why are you insisting to 
>use a
>model requiring (in the earlier pointed out
>place) four comparisons when two can do? I realize this is only a small
>inefficiency here, but you should realize that they add up if we don't look to
>avoid them where possible.

We thought the code was easier to understand with min and max set to INVALID.
we can take your approach to avoid this inefficiency if you want.

Ravi

>
>Jan

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