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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.11.18 at 13:03, <george.dunlap@citrix.com> wrote:
> 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?

So with the field getting updated right in ept_set_entry(), as long as
no copying of entries exists which does not go through that function,
I then agree that it shouldn't really matter whether the field gets
copied when setting up a new altp2m.

However, fair parts of your further response are confusing to me,
rather than clarifying. That's for one because you talk about the
max_remapped_gfn field, but you never mention its
min_remapped_gfn sibling. The only place I could find where
current code consumes these two is p2m_altp2m_propagate_change().
This suggests to me that both fields really only exist for optimization
purposes.

Furthermore I in particular ...

> 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().

... cannot identify any of the three cases above where I understand
you say a max_mapped_pfn == max_remapped_gfn comparison
happens. But as you say - the code is complicated  enough, so I may
easily overlook something.

Jan



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