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

Re: [PATCH 1/4] x86/PVH: de-duplicate mappings for first Mb of Dom0 memory

  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 31 Aug 2021 14:02:49 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sC6FxcV4fWhBGy+WBcSzHx34FejOsk/gvQo5l1SaVX0=; b=eBHhQB3N3l6brkDNbJB4YpGaJBsULS0fMDCDJpWL1TQ+KcN4n+pk+K8L/WAZ1QGzD8ZRCDmmPtp1YJOX0P/tnrnwiiZvdgvx9tPCISI9g6LsA1hrSiYN3ZopDXh7sVhVJ/QveQXx/FYuLtVF4NOuAikhhTEYXttTZYXlgDzK1pMTkibyeJSNXes5CRxeGFnpzeHeZBrr2ynpWw0AYql66qM02bq4zNnabKkqeDw5rMtvpQBWw33i7g4vj29BpY2UFKWeYZf61rX38MeMLqWjsaCXTiHm9CMJ11PBg8TfKSBU1JgUfrNQEIhuL2THkztC9RqyqnBBPEls+C4LiSRAyg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Af+/RZt9JWibd+A4agowti3JGF3LqKhKSS/6hwh9Yov1sM00YCESTA4lJPEWpHNl5oSh6cP8T2/CgtIc3ZVLa2pJrqaFV/4CyPl8QfjvbH/L9gnjSnnUY48XKJWNxXLes+rwbsT4NWgnfWo40cPKXRuXMJHVdS8ZUV52kHfqhrbiYqIoh4zGK5nOTiT36/V27caUKwMgpjYbP1vf8dCgC5Z3wJH2Y7iYy6/4CMR5Zi/N5dYsmLPDCXlBeJKkB8eZ9fB1JZ/sh9/KPgXKfYgZNaGhFop+MDoGr4Z/iBzAUrgzvJeLgFiucbyI+n4MzG+35eFMSW0Bd1Goj1xSpiHIqw==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 31 Aug 2021 13:03:01 +0000
  • Ironport-hdrordr: A9a23:csAAJKG8/P/+A/QSpLqFZpHXdLJyesId70hD6qkvc3Nom52j+/ xGws536faVslcssHFJo6HkBEDyewKiyXcT2/hsAV7CZniahILMFu9fBOTZskXd8kHFh4lgPO JbAtJD4b7LfChHZKTBkXCF+r8bqbHtmsDY5pat854ud3APV0gJ1XYJNu/xKDwReOApP+taKH PR3Ls9m9L2Ek5nEPhTS0N1ENTrlpnurtbLcBQGDxko5E2nii6p0qfzF1y90g0FWz1C7L8++S yd+jaJp5mLgrWe8FvxxmXT55NZlJ/IzcZCPtWFjowwJi/3ggilSYx9U/mpvSwzosuo9FE2+e O87SsIDoBW0Tf8b2u1qRzi103J1ysv0WbrzRuijX7qsaXCNXgHIvsEobgcXgrS6kImst05+r lMxXilu51eCg6FtDjh5vDTPisa1HackD4Hq6o+nnZfWYwRZPt6tooE5n5YF58GAWbT9J0nKu 9zF8vRjcwmMm9yV0qp+lWH/ebcGUjaRny9Mw4/U42uonhrdUlCvg4lLJd1pAZYyHpVIKM0lN gtMcxT5fpzp4EtHPpA7Epoe7rANoX3e2O5DIulGyWuKEg2AQO5l3fJ2sRD2AiLQu1E8HJgou WMbLtn3VRCMn4GT/f+h6F2zg==
  • Ironport-sdr: ypViPJkEV8AWID0kPNAw1xV91BFgasGQA4yJ/mZh1mmpIwpsNbsOZ5cjtOs/z+B7jXADhi3ov7 Z59yxkyEbr/wIo+u4GsXedCDxrY8RFrcoxmpnUcEUOhHAMGHhZ2kRZAnthaadcm2ub+7XgZycP BkdPQN/hFjBiqOG46ezbhC3uHId+YZVhzrBgpNtpoWHofdTwcM9DlOCtYLQeRgMBS1uQkW6YG9 yMqBRxgtElzdOhY+5VTG6ODfHhcMs2HviHVTM4DecT1LNkTNdTZJBuEvL3AtYVz2NuLO19/qz6 BHspB5hH0bmBRlIHgm/j90Hu
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 30/08/2021 14:02, Jan Beulich wrote:
> One of the changes comprising the fixes for XSA-378 disallows replacing
> MMIO mappings by unintended (for this purpose) code paths.

I'd drop the brackets.  All it does is confuse the sentence.

>  This means we
> need to be more careful about the mappings put in place in this range -
> mappings should be created exactly once:
> - iommu_hwdom_init() comes first; it should avoid the first Mb,
> - pvh_populate_p2m() should insert identity mappings only into ranges
>   not populated as RAM,
> - pvh_setup_acpi() should again avoid the first Mb, which was already
>   dealt with at that point.

This really is a mess.  It also seems very fragile.

Why is iommu_hwdom_init() separate in the first place?  It only does
mappings up to 4G in the first place, and with this change, it is now

All IOMMU modifications should be as a side effect of regular p2m/guest
physmap operations.  I suppose this is another breakage from the PV side
of things.

> @@ -1095,6 +1101,17 @@ static int __init pvh_setup_acpi(struct
>          nr_pages = PFN_UP((d->arch.e820[i].addr & ~PAGE_MASK) +
>                            d->arch.e820[i].size);
> +        /* Memory below 1MB has been dealt with by pvh_populate_p2m(). */
> +        if ( pfn < PFN_DOWN(MB(1)) )
> +        {
> +            if ( pfn + nr_pages <= PFN_DOWN(MB(1)) )
> +                continue;
> +
> +            /* This shouldn't happen, but is easy to deal with. */

I'm not sure this comment is helpful.

Under PVH, there is nothing special about the 1M boundary, and we can
reasonably have something else here or crossing the boundary.

Preferably with this removed, Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>, but only because this is an emergency fix. 
I really don't think it is an improvement to the logic.




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