Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/16/18 2:03 PM, George Dunlap wrote:
> On 11/16/18 10:08 AM, Jan Beulich wrote:
>>>>> On 15.11.18 at 18:54, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 11/15/18 7:34 PM, George Dunlap wrote:
>>>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 
>>>>> wrote:
>>>>> @@ -1440,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d, 
>>>>> unsigned int i)
>>>>>     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>     struct ept_data *ept;
>>>>>
>>>>> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>>>>
>>>> Why we need to copy this value?
>>>>
>>>> I’ve just spent the afternoon tracing around the source code, and while 
>>>> I’m 
>>> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
>>>
>>> I think I did that because I assumed that it would matter for
>>> ept_get_entry() and misconfigurations, when:
>>>
>>>  954     /* This pfn is higher than the highest the p2m map currently
>>> holds */
>>>  955     if ( gfn > p2m->max_mapped_pfn )
>>
>> The question is whether under any condition such checks require that
>> the accumulated value be in sync between the host and the various
>> alt p2m-s.
>>
>>>  956     {
>>>  957         for ( i = ept->wl; i > 0; --i )
>>>  958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
>>>  959                  p2m->max_mapped_pfn )
>>>  960                 break;
>>>  961         goto out;
>>>  962     }
>>>
>>> but I am not certain it is required either. I can certainly remove this
>>> assignment and see if anything breaks.
>>
>> I've seen you mention such or alike a couple of times now while
>> discussing this series, and I have to admit that I'm a little frightened:
>> Making a change just based on the observation that nothing breaks
>> is setting us up for breakage in some corner case. That is - seeing
>> something break is a good indication that a change was wrong, but
>> not seeing any breakage doesn't alone justify a change.
>>
>> In the particular case here I think whether the copying of the field
>> is needed depends on what else is being copied (I'm sorry, I'm not
>> that familiar with altp2m): If mappings get cloned from the host p2m,
>> then this value ought to be cloned too. For any mappings that
>> might be kept in sync between p2m-s, I think this field also would
>> need to be updated in sync.
> 
> So the basic question is, does "max_mapped_pfn" mean, "Maximum pfn _for
> the domain_", or "Maximum pfn _for this p2m_".  When the element was
> added to the p2m struct those were the same thing.  Which do the various
> use cases expect it to be, and which would be the most robust going forward?
> 
> I spent a bunch of time going through the code yesterday, and I'm pretty
> sure that as long as the value in the p2m is one or the other, there
> will be at the moment no _correctness_ issues.  It looked to me like in
> the case where altp2m->max_mapped_pfn > altp2m->max_remapped_gfn, then
> the p2m machinery would do a certain amount of unnecessary work, but
> that's all.
> 
> It also looked to me like before this patch, the value mostly ends up
> being  "maximum pfn ever mapped in this p2m (even across altp2m
> desroys)".  That's because when the altp2m is allocated, it starts as 0;
> every time an entry is propagated from the hostp2m to the altp2m,
> ept_set_entry() updates max_mapped_pfn; but nothing sets it back to zero.
> 
> Also, hostp2m->max_mapped_pfn is never decreased, only increased.
> 
> So either before or after this patch, altp2m->max_remapped_gfn <=
> altp2m->max_mapped_pfn <= hostp2m->max_mapped_pfn.
> 
> In the vast majority of cases, max_mapped_pfn is explicitly being read
> from the hostp2m.
> 
> Below are the cases where it may be read from an altp2m:
> 
>  - ept_get_entry(), ept_walk_tables(): If ==max_remapped_gfn, it will
> return INVALID_MFN early.  If higher than max_remapped_gfn, it falls
> back to walking the altp2m's ept tables, which will give you the same
> answer, just a bit more slowly.
> 
>  - ept_dump_p2m_tables(): If ==max_remapped_gfn, it will dump only up to
> the current map; if >max_remapped_gfn, it will dump a number of
> unnecessary INVALID_MFN entries, but no more entries than the hostp2m.
> 
>  - p2m.c:change_type_range(): If ==max_remapped_gfn, it will only
> invalidate entries in the altp2m as high have been currently remapped.
> If >max_remapped_gfn, it will write invalid ept entries that *haven't*
> yet been copied over.  But I don't think either one should cause a
> correctness issue: either way, accessing a gfn > max_remapped_gfn will
> cause a fault, at which point either the correct value will be copied
> from the hostp2m (perhaps going through resolve_misconfig() on the
> hostp2m in the process) or the correct value will be calculated via
> resolve_misconfig().
> 
> So, it seemed from my investigation like it would be more useful if we
> got rid of altp2m->max_remapped_gfn, and used atlp2m->max_mapped_pfn
> instead.  If people want to know the maximum gfn mapped by the domain as
> a whole, they should just use hostp2m->max_mapped_pfn.
> 
> The code is definitely complicated enough, though, that I may have
> missed something, which is why I asked Razvan if there was a reason he
> changed it.
> 
> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
> max_remapped_pfn" for a future clean-up series.

Thank you for the detailed analysis and the suggestions.

> That said -- Razvan, I realize the code is quite complex, but it's still
> important that when you modify things you work to have a clear
> understanding of why you're making changes.  The analysis I did above
> with how max_mapped_pfn is used and what the effects would be one way or
> the other should have been something you did before modifying it.
> Otherwise things will only get more incomprehensible and buggy.

Point taken, and I agree.

I have in fact done an analysis and reached some of the same
conclusions, but the reasons I have deferred to your call are:

1. the code being as complex as it is I thought the patch needed the
approval of someone with a much better overview of p2m,

2. most of the detailed analysis I've done happened months ago when the
series first got started, which means I'd have to redo at least part of
it to reach certainty - so couldn't reply on the spot more than I did,

and 3. I had misinterpreted the question to mean that the code is
probably incorrect (which is apparent from my initial reply), which of
course makes even the initial analysis seem wrong and ends up being a
hunt for judgment errors rather than just an analysis of the logic.

Thanks again for your help, and sorry for any accidental inconvenience.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel