From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::244; helo=mail-io0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B19862118D92C for ; Wed, 20 Jun 2018 23:21:24 -0700 (PDT) Received: by mail-io0-x244.google.com with SMTP id g22-v6so1962056iob.7 for ; Wed, 20 Jun 2018 23:21:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5WfCExjSmKiCfwVRI7kQ7ue9YRmiL87oSJYJh3gHIp8=; b=QqTSeKp/9CA0fc07JKFT1jTMg9s1A9jSytqwPrwHNWIwHJxJqpQQcHsN2ZObI2Ubuk jDw7O5x5/6TVzM61ERP7CuHP0WKpliZGzLegrH4zEzOnqKXLbXa3C01hR+jDef4jq+B9 E3w3IbL2bW6UypT2zvBBFYyTschn6+L/dRteA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5WfCExjSmKiCfwVRI7kQ7ue9YRmiL87oSJYJh3gHIp8=; b=XF8bsxXb6bt6qzLY2rSF6GuOoVTFt7bmGRwXZ/JzIUi5HqzTG9qNjKTFYZ1FW/aw9o wU/C0LgyzIhymzbcltCmajT6vXlWwH7/9ewBYHsu+YMDCz9/9FL/+XmDYKZaa7WNTrQp 9MzCao8U7iy+190at8PcobBlNrhSsM9Pre9iiQSxKsbBJyNrsOTVxUTVSdCwqB/A5502 h+lcsCPhav8v10yuec+XCWbCq7jTj3S41XdOjXuC95UQP9u3/KcsSaKrEOLoXl/A5z1+ +KidiZzRhDZpnMm8fNOIeQiRY3UsXPI+Of3m5v4mDPefvhf3Rvs6rK5IexpF0D26jcVk Swlw== X-Gm-Message-State: APt69E26+3ULwMcmL7AsXAkIxKGDqPSl69YAQ+ZWEOfibz94vWHfi7Rd cXXW7R+OTF3k32t4kGW/Wd8yRlJv8RYYD36QwnPS+w== X-Google-Smtp-Source: ADUXVKKoQbzD4k5d+O4tmvkf7k0gbE/b4CBBRujyo5ETx+Cgazr2AoPEEv/n+hbeIhD50XXw/izWKR410+UD/lO0LHU= X-Received: by 2002:a6b:520d:: with SMTP id g13-v6mr20622480iob.60.1529562083683; Wed, 20 Jun 2018 23:21:23 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Wed, 20 Jun 2018 23:21:22 -0700 (PDT) In-Reply-To: <20180620220559.2g5wvickl6bgtnzi@bivouac.eciton.net> References: <20180620191007.1250-1-ard.biesheuvel@linaro.org> <20180620220559.2g5wvickl6bgtnzi@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 21 Jun 2018 08:21:22 +0200 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Chris Co Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: remove cache maintenance of block mapping contents X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jun 2018 06:21:24 -0000 Content-Type: text/plain; charset="UTF-8" On 21 June 2018 at 00:05, Leif Lindholm wrote: > On Wed, Jun 20, 2018 at 09:10:07PM +0200, Ard Biesheuvel wrote: >> Peculiarly enough, the current page table manipulation code takes it >> upon itself to write back and invalidate the memory contents covered >> by section mappings when their memory attributes change. It is not >> generally the case that data must be written back when such a change >> occurs, even when switching from cacheable to non-cacheable attributes, >> and in some cases, it is actually causing problems. (The cache >> maintenance is also performed on the PCIe MMIO regions as they get >> mapped by the PCI bus driver, and under virtualization, each cache >> maintenance operation on an emulated MMIO region triggers a round >> trip to the host and back) >> >> So let's just drop this code. > > I guess this is a remnant from pre-ARMv7 days, when translation table > walks weren't inner-cacheable. I think we've already determined that > ARMv7-A+ is required for PI compliance anyway, so I guess dropping > this should be fine. > No, this one is different. This does not flush the page containing the page table entry that is modified, but it flushes the memory that the page table entry maps (a 1 MB section in this case) > But if so, don't we also want to get rid of the other two instances of > WriteBackInvalidateDataCacheRange() in this file? > If we assume page table walks are coherent, then yes, we could remove some more code, but that should be a separate patch. >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel >> --- >> ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> index 9bf4ba03fd5b..d1bca4c601b8 100644 >> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> @@ -718,12 +718,6 @@ UpdateSectionEntries ( >> if (CurrentDescriptor != Descriptor) { >> Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT); >> >> - // Clean/invalidate the cache for this section, but only >> - // if we are modifying the memory type attributes >> - if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) { >> - WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB); >> - } >> - >> // Only need to update if we are changing the descriptor >> FirstLevelTable[FirstLevelIdx + i] = Descriptor; >> ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva); >> -- >> 2.17.1 >>