Re: [Xen-devel] 2nd opinion on backportability of c35eefded2

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 13/05/16 10:32, Jan Beulich wrote:
> Hi George,
> 
> after quite a bit of debugging on 4.6.1 I learned that said commit
> ("x86/P2M: consolidate handling of types not requiring a valid MFN")
> is more than just cleanup: Since p2m_set_entry() happily performs
> arithmetic on the passed in MFN, shadow mode guests (verified) as
> well as HAP ones when 1Gb / 2Mb mappings are unavailable (not
> verified), if any of the MFNs below 1Gb are invalid (reported with
> e.g. "crashkernel=512M@16M"), p2m_pt_set_entry() would (in
> the context of guest_physmap_mark_populate_on_demand())
> produce invalid instead of PoD entries, resulting in subsequent
> attempts by the guest to use (e.g. balloon out) these pages to
> fail (the balloon failure results in a crash during boot).

I'm sorry, I'm having some trouble parsing this paragraph. :-)

But this is what I gather:

Before c35eefded2, the 4k-entry path didn't check whether the p2m entry
was PoD, it only checked that the mfn was valid.  The pod code would
always set this mfn to 0, which is (usually) valid; but p2m_set_entry()
might iterate over this, ending up in p2m_pt_set_entry() with non-zero
low values for mfn.  In certain circumstances these low-value mfns might
actually be invalid (for example, if they overlap a crash kernel).

In such a case, the resulting p2m entries would be (erroneously) marked
as invalid rather than PoD, resulting in a crash when the guest tried to
access it or populate it.

You observed this happening in the shadow case, and believe it would
happen if p2m 2MiB or 1GiB pages were disabled but haven't checked those
cases.

c35eefded2 fixes this by implicitly checking for the PoD type on the 4k
entry path.

> Now, while the backport to 4.6 is straightforward, I'm having the
> vague feeling that this change might depend on some earlier one,
> but I can't pinpoint anything. Hence I would appreciate if you
> could take a look and provide your judgment.

We talked about the PoD type thing in the context of a discussion about
660fd65 ("x86/p2m-pt: tighten conditions of IOMMU mapping updates") --
discussion here [1].  But it doesn't look like that's a prerequisite,
and nothing else really comes to mind.

 -George

[1] marc.info/?i=<560D261D02000078000A760A@xxxxxxxxxxxxxxxxxxxxxxx>



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