[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Fri, 16 Nov 2018 15:05:19 +0000
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= xsFNBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABzSRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT7CwYAEEwEKACoCGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEFAlpk2IEFCQo9I54ACgkQpjY8MQWQtG1A1BAAnc0oX3+M/jyv4j/ESJTO U2JhuWUWV6NFuzU10pUmMqpgQtiVEVU2QbCvTcZS1U/S6bqAUoiWQreDMSSgGH3a3BmRNi8n HKtarJqyK81aERM2HrjYkC1ZlRYG+jS8oWzzQrCQiTwn3eFLJrHjqowTbwahoiMw/nJ+OrZO /VXLfNeaxA5GF6emwgbpshwaUtESQ/MC5hFAFmUBZKAxp9CXG2ZhTP6ROV4fwhpnHaz8z+BT NQz8YwA4gkmFJbDUA9I0Cm9D/EZscrCGMeaVvcyldbMhWS+aH8nbqv6brhgbJEQS22eKCZDD J/ng5ea25QnS0fqu3bMrH39tDqeh7rVnt8Yu/YgOwc3XmgzmAhIDyzSinYEWJ1FkOVpIbGl9 uR6seRsfJmUK84KCScjkBhMKTOixWgNEQ/zTcLUsfTh6KQdLTn083Q5aFxWOIal2hiy9UyqR VQydowXy4Xx58rqvZjuYzdGDdAUlZ+D2O3Jp28ez5SikA/ZaaoGI9S1VWvQsQdzNfD2D+xfL qfd9yv7gko9eTJzv5zFr2MedtRb/nCrMTnvLkwNX4abB5+19JGneeRU4jy7yDYAhUXcI/waS /hHioT9MOjMh+DoLCgeZJYaOcgQdORY/IclLiLq4yFnG+4Ocft8igp79dbYYHkAkmC9te/2x Kq9nEd0Hg288EO/OwE0EVFq6vQEIAO2idItaUEplEemV2Q9mBA8YmtgckdLmaE0uzdDWL9To 1PL+qdNe7tBXKOfkKI7v32fe0nB4aecRlQJOZMWQRQ0+KLyXdJyHkq9221sHzcxsdcGs7X3c 17ep9zASq+wIYqAdZvr7pN9a3nVHZ4W7bzezuNDAvn4EpOf/o0RsWNyDlT6KECs1DuzOdRqD oOMJfYmtx9hMzqBoTdr6U20/KgnC/dmWWcJAUZXaAFp+3NYRCkk7k939VaUpoY519CeLrymd Vdke66KCiWBQXMkgtMGvGk5gLQLy4H3KXvpXoDrYKgysy7jeOccxI8owoiOdtbfM8TTDyWPR Ygjzb9LApA8AEQEAAcLBZQQYAQoADwIbDAUCWmTXMwUJB+tP9gAKCRCmNjwxBZC0bb+2D/9h jn1k5WcRHlu19WGuH6q0Kgm1LRT7PnnSz904igHNElMB5a7wRjw5kdNwU3sRm2nnmHeOJH8k Yj2Hn1QgX5SqQsysWTHWOEseGeoXydx9zZZkt3oQJM+9NV1VjK0bOXwqhiQyEUWz5/9l467F S/k4FJ5CHNRumvhLa0l2HEEu5pxq463HQZHDt4YE/9Y74eXOnYCB4nrYxQD/GSXEZvWryEWr eDoaFqzq1TKtzHhFgQG7yFUEepxLRUUtYsEpT6Rks2l4LCqG3hVD0URFIiTyuxJx3VC2Ta4L H3hxQtiaIpuXqq2D4z63h6vCx2wxfZc/WRHGbr4NAlB81l35Q/UHyMocVuYLj0llF0rwU4Aj iKZ5qWNSEdvEpL43fTvZYxQhDCjQTKbb38omu5P4kOf1HT7s+kmQKRtiLBlqHzK17D4K/180 ADw7a3gnmr5RumcZP3NGSSZA6jP5vNqQpNu4gqrPFWNQKQcW8HBiYFgq6SoLQQWbRxJDHvTR YJ2ms7oCe870gh4D1wFFqTLeyXiVqjddENGNaP8ZlCDw6EU82N8Bn5LXKjR1GWo2UK3CjrkH pTt3YYZvrhS2MO2EYEcWjyu6LALF/lS6z6LKeQZ+t9AdQUcILlrx9IxqXv6GvAoBLJY1jjGB q+/kRPrWXpoaQn7FXWGfMqU+NkY9enyrlw==
  • Cc: Kevin Tian <kevin.tian@xxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 16 Nov 2018 15:05:31 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 11/16/18 12:31 PM, Jan Beulich wrote:
>>>> On 16.11.18 at 13:03, <george.dunlap@xxxxxxxxxx> 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.

Sorry, it seems I took too many shortcuts explaining things. :-)  I was
using max_remapped_gfn as a shorthand for, "the highest gfn mapped in
the altp2m" (since that's what it will be equal to).  Not that an actual
comparison will happen there, but we're considering what will happen,
based on various values of altp2m->max_mapped_pfn, when a gfn that is
higher than the highest *remapped* gfn is encountered.

So in the situation we're considering, the following are always true:
- gfn > altp2m->max_remapped_gfn
- altp2m->max_remapped_gfn <= altp2m->max_mapped_pfn <=
hostp2m->max_mapped_pfn

And we're comparing the results in the following cases:

A: altp2m->max_mapped_pfn == alt2m->max_remapped_gfn
B: altp2m->max_mapped_pfn > altp2m->max_remapped_gfn
  (Perhaps == hostp2m->max_mapped_pfn, perhaps something in between).

Take ept_get_entry().  At this level, it's below all the mem_access /
mem_sharing / mem_paging / altp2m abstractions.  Apart from
resolve_misconfig and pod, it should return the actual contents of the
particular p2m it's given.  In the case of an altp2m, any entries above
what's currently been remapped should return empty.

In case A, it will do this, because the first conditional will find that
gfn > altp2m->max_mapped_pfn.

In case B, it will also do this, because although it passes the first
conditional, when it actually reads the table it will find an empty
entry and return that.

Both results are correct, but A is a tiny bit faster.

Now take change_type_range.  The global effect of change_type_range
should be that reads of the p2m which happen afterwards should have the
new, changed value.

In case A, change_type_range will write invalid entries up to
max_remapped_pfn, leaving the range between max_remapped_pfn and
hostp2m->max_mapped_pfn invalid.  When a gfn in this range is read, an
EPT fault will happen, p2m_altp2m_lazy_copy() will be called, and the
new (correct) value copied from the hostp2m.

In case B, change_type_range will write invalid entries up until
hostp2m->max_mapped_pfn.  When a gfn in this range is accessed, a
MISCONFIG fault will happen, and the correct value will be calculated in
resolve_misconfig.

And at this point, I realize that my previous analysis was probably
wrong, because at this point altp2m->max_remapped_gfn will be wrong:
entries above max_remapped_gfn will have become valid without going
through p2m_altp2m_lazy_copy().

 -George

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.