From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <mark.rutland@arm.com>
Received-SPF: Pass (sender SPF authorized) identity=mailfrom;
 client-ip=217.140.101.70; helo=foss.arm.com;
 envelope-from=mark.rutland@arm.com; receiver=edk2-devel@ml01.01.org 
Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70])
 by ml01.01.org (Postfix) with ESMTP id D2752211B85E3
 for <edk2-devel@ml01.01.org>; Mon, 28 Jan 2019 02:23:17 -0800 (PST)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])
 by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 15C46EBD;
 Mon, 28 Jan 2019 02:23:17 -0800 (PST)
Received: from blommer (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70])
 by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 615873F589;
 Mon, 28 Jan 2019 02:23:15 -0800 (PST)
Date: Mon, 28 Jan 2019 10:23:12 +0000
From: Mark Rutland <mark.rutland@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>, Marc Zyngier <marc.zyngier@arm.com>,
 "edk2-devel@lists.01.org" <edk2-devel@ml01.01.org>,
 Christoffer Dall <christoffer.dall@arm.com>,
 Tanxiaojun <tanxiaojun@huawei.com>
Message-ID: <20190128102311.ailneth53n63tsbh@blommer>
References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org>
 <f2c9c8cc-ab0c-f094-f921-aa207a730d1b@redhat.com>
 <CAKv+Gu_sXc9JkY0NAUtRj=kfi-=7WBXz6=Rhm13Apr9GJsRG8w@mail.gmail.com>
 <2dd4294c-76f0-f433-cbd2-bf0b37114aee@redhat.com>
 <CAKv+Gu_Qvcr7mrJ215fQkO+V8wVE58Bi0-Qk=_wvhxGU5_oR-Q@mail.gmail.com>
 <12fa0861-e25d-eba7-48ea-2bd7d47d58fb@redhat.com>
 <CAKv+Gu9FKyhVawUP_KFg2fe9zwGUpgj4gMt0gJtyOkp+tBvp0w@mail.gmail.com>
MIME-Version: 1.0
In-Reply-To: <CAKv+Gu9FKyhVawUP_KFg2fe9zwGUpgj4gMt0gJtyOkp+tBvp0w@mail.gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU
X-BeenThere: edk2-devel@lists.01.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: EDK II Development  <edk2-devel.lists.01.org>
List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=unsubscribe>
List-Archive: <http://lists.01.org/pipermail/edk2-devel/>
List-Post: <mailto:edk2-devel@lists.01.org>
List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help>
List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=subscribe>
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jan 23, 2019 at 03:02:14PM +0100, Ard Biesheuvel wrote:
> On Wed, 23 Jan 2019 at 10:55, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 01/23/19 10:26, Ard Biesheuvel wrote:
> > > On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek <lersek@redhat.com> wrote:
> > >> On 01/22/19 16:37, Ard Biesheuvel wrote:
> >
> > >>> Is SetUefiImageMemoryAttributes() being
> > >>> called to remap the memory R-X ?
> > >>
> > >> No, it is not; the grub binary in question doesn't have the required
> > >> section alignment (... I hope at least that that's what your question
> > >> refers to):
> > >>
> > >>> ProtectUefiImageCommon - 0x3E6C54C0
> > >>>   - 0x000000013BEEF000 - 0x0000000000030600
> > >>> !!!!!!!!  ProtectUefiImageCommon - Section Alignment(0x200) is
> > >> incorrect  !!!!!!!!
> > >>
> > >
> > > This is puzzling, given that the exact same binary works on Mustang.
> >
> > And even on the original (unspecified) hardware, the same binary works
> > frequently. My understanding is that there are five VMs executing reboot
> > loops in parallel, on the same host, and 4 out of 5 may hit the issue in
> > a reasonable time period (300 reboots or so).
> >
> > > So when loaded, GRUB should cover the following regions:
> > >
> > > 0x13beef0000 - 0x13bf000000 (0x11000)
> > > 0x13bf000000 - 0x13bf01f600 (0x1f600)
> > >
> > > where neither covers a 2 MB block fully, which means that the TLB
> > > entry that we are hitting is stale.
> > >
> > > Since ProtectUefiImageCommon() does not do anything in this case, the
> > > stale translation must be the result of
> > > PcdDxeNxMemoryProtectionPolicy, which either sets the wrong
> > > permissions for EfiLoaderCode (relying on ProtectUefiImageCommon), or
> > > we don't flush the TLBs correctly after updating the permissions when
> > > converting the memory from EfiConventionalMemory to EfiLoaderCode
> > >
> > > Are you using the default value for PcdDxeNxMemoryProtectionPolicy?
> >
> > Yes, we have
> >
> > ArmVirtPkg/ArmVirt.dsc.inc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
> >
> > from commit 1acd7c54a724 ("ArmVirtPkg AARCH64: enable NX memory
> > protection for all platforms", 2017-03-01).
> >
> > The binary is from the RPM
> > "edk2-aarch64-20180508gitee3198e672e2-5.el8+1789+f0947240.noarch", which
> > is basically upstream ee3198e672e2 plus a small number of backports and
> > downstream customizations.
> >
> 
> This might help:
> 
> diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> index b7173e00b039..4c0b4b4efbd5 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> @@ -138,7 +138,7 @@ ASM_FUNC(ArmUpdateTranslationTableEntry)
> 
>  ASM_FUNC(ArmInvalidateTlb)
>     EL1_OR_EL2_OR_EL3(x0)
> -1: tlbi  vmalle1
> +1: tlbi  vmalle1is
>     b     4f
>  2: tlbi  alle2
>     b     4f
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index 90192df24f55..d54b1c19accf 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -34,7 +34,7 @@
> 
>    // flush the TLBs
>    .if   \el == 1
> -  tlbi  vmalle1
> +  tlbi  vmalle1is
>    .else
>    tlbi  alle\el
>    .endif

Assuming that hardware is working correctly, this change shouldn't be
necessary.

KVM sets HCR_EL2.FB, so all TLBI ops will behave as their *IS variant.
Likewise it sets HCR_EL2.BSU, so barriers apply to the inner shareable domain too.

On bare-metal, NSH should be sufficient.

Thanks,
Mark.