[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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 31 Aug 2021 15:14:06 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=UFF4fJKbHp6YIArCTDYlMsJMmLDJ5i1iQ7p6he4/FCI=; b=b105gyk1hgUds2kEKdgu3ThnSpEN8SU51CngqJDVOHGe169zYz2SB/bxXGmg9XvsdCw17y1H0/uhQVimp1tn8flzynaUs94q73vkDaFw2bJXfVdO7pwnZLStG8KYMPJSh/B5nkRoQ6gVH3G041AqxaR52QkVgRZEPjY/6ElSPd49Bd6y3LN/JKvCToEVNuueHXXdD94eumj52z+yhdz0+3UA0whlyLtCdjqSchz45uAy8XDKxjKUAFW+1dx90cIgcn/doHJOuoN2mr77yzECmOzLLCs15cRbKsWSh1uHxvmx5hJAyCsnATIP6+dKeneAbU1XChqBlf1SNgrDQyntVA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=a2ePx6cxZVxQSj1YxzSCZDEUTINh5yDh1WJ10oM4cC9PXKGnEcaO9RvZgXSxG1fs3WtqlxEWKAhEJIQDnNOAC9flp3h8R2U2FxK38uy9UPZijpMvVB2YzkVnO1khaWsWf4UEHLwqh1I169FLjpxME5qEN0NK2itOBiYYTBaXc1B/iVan+7EL6Pn5WJpEKmwVQa6in9iKPgAsGU5m+npX/9vF9nuG82EOehUi+PH3o9/SwTPQixxMEQqIhTZKNFNqtjWfjsHhEHWBk7jPLPBalfucv3t3M/7JjORl/BuBi++vPZ5oR5/Cdb/nqFmIvT1qy4YqhAhz9xlwMnxt1HkW/Q==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 31 Aug 2021 13:14:27 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 31.08.2021 15:02, Andrew Cooper wrote:
> 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.

So it seems to me.

> 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
> [1M-4G)

I guess we'll want to wait for Roger to return to shed some light on

>> @@ -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.

As long as we have this special treatment of the low Mb, the boundary
is meaningful imo. I'd see the comment go away when the handling in
general gets streamlined.

> Preferably with this removed, Acked-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>, but only because this is an emergency fix.

Thanks. I see. You'll like patch 2 even less; at least I do. And I'm
not really certain that change is enough to cover all possible

> I really don't think it is an improvement to the logic.

Yet I suppose you also have no immediate suggestions towards doing
better? Of course right here a full rework is out of scope. But if
there were smaller bits that - if adjusted - would make you feel
better about the change as a whole, I'd be happy to consider making




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