| Message ID | 20210506095034.15246-3-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Free up a couple of MAIRs | expand |
On Thu, 6 May 2021 at 11:50, Will Deacon <will@kernel.org> wrote: > > The only user we have of Normal Write-Through memory is in the ACPI code > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > CPUs treat write-through as non-cacheable under the hood, don't bother > with the extra memory type here and just treat EFI_MEMORY_WT the same way > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Will Deacon <will@kernel.org> I don't have any objections to this change per se, but I will point out that the UEFI spec describes the MAIR encodings, paragraph 2.3.6.1 (in revision 2.8B). However, the paragraph in question provides no context whatsoever, and so it is not clear whether it is normative, and whether it applies to the boot time firmware only or to the OS as well. So in summary, given that EFI_MEMORY_WT (which I have never seen being used on ARM) should behave as expected when using the same MAIR attributes as EFI_MEMORY_WC, with only a theoretical performance impact, the change looks reasonable to me. Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm64/kernel/acpi.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index cada0b816c8a..aca5ee2a9e64 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -246,7 +246,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) > * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is > * mapped to a corresponding MAIR attribute encoding. > * The EFI memory attribute advises all possible capabilities > - * of a memory region. We use the most efficient capability. > + * of a memory region. > */ > > u64 attr; > @@ -254,9 +254,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) > attr = efi_mem_attributes(addr); > if (attr & EFI_MEMORY_WB) > return PAGE_KERNEL; > - if (attr & EFI_MEMORY_WT) > - return __pgprot(PROT_NORMAL_WT); > - if (attr & EFI_MEMORY_WC) > + if (attr & (EFI_MEMORY_WC | EFI_MEMORY_WT)) > return __pgprot(PROT_NORMAL_NC); > return __pgprot(PROT_DEVICE_nGnRnE); > } > @@ -340,9 +338,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > default: > if (region->attribute & EFI_MEMORY_WB) > prot = PAGE_KERNEL; > - else if (region->attribute & EFI_MEMORY_WT) > - prot = __pgprot(PROT_NORMAL_WT); > - else if (region->attribute & EFI_MEMORY_WC) > + else if (region->attribute & (EFI_MEMORY_WC | EFI_MEMORY_WT)) > prot = __pgprot(PROT_NORMAL_NC); > } > } > -- > 2.31.1.607.g51e8a6a459-goog >
On Thu, May 06, 2021 at 10:50:33AM +0100, Will Deacon wrote: > The only user we have of Normal Write-Through memory is in the ACPI code > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > CPUs treat write-through as non-cacheable under the hood, don't bother > with the extra memory type here and just treat EFI_MEMORY_WT the same way > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. The UEFI spec explicitly defines EFI_MEMORY_WT as Normal Outer-WT Inner-WT (and even explicitly specifies the MAIR.Attr<n> value). In the UEFI 2.9 spec, see section 2.3.6.1 "Memory types", Table 2-5 "Map: EFI Cacheability Attributes to AArch64 Memory Types". The UEFI 2.9 spec can be found at: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf Given that is specified explicitly, and given that we don't know how future CPUs will treat this equivalently, I don't think this change is architecturally sound and I don't think there's wiggle-room to read the spec as permitting this. Thanks, Mark. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/acpi.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index cada0b816c8a..aca5ee2a9e64 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -246,7 +246,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) > * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is > * mapped to a corresponding MAIR attribute encoding. > * The EFI memory attribute advises all possible capabilities > - * of a memory region. We use the most efficient capability. > + * of a memory region. > */ > > u64 attr; > @@ -254,9 +254,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) > attr = efi_mem_attributes(addr); > if (attr & EFI_MEMORY_WB) > return PAGE_KERNEL; > - if (attr & EFI_MEMORY_WT) > - return __pgprot(PROT_NORMAL_WT); > - if (attr & EFI_MEMORY_WC) > + if (attr & (EFI_MEMORY_WC | EFI_MEMORY_WT)) > return __pgprot(PROT_NORMAL_NC); > return __pgprot(PROT_DEVICE_nGnRnE); > } > @@ -340,9 +338,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > default: > if (region->attribute & EFI_MEMORY_WB) > prot = PAGE_KERNEL; > - else if (region->attribute & EFI_MEMORY_WT) > - prot = __pgprot(PROT_NORMAL_WT); > - else if (region->attribute & EFI_MEMORY_WC) > + else if (region->attribute & (EFI_MEMORY_WC | EFI_MEMORY_WT)) > prot = __pgprot(PROT_NORMAL_NC); > } > } > -- > 2.31.1.607.g51e8a6a459-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, May 06, 2021 at 11:20:45AM +0100, Mark Rutland wrote: > On Thu, May 06, 2021 at 10:50:33AM +0100, Will Deacon wrote: > > The only user we have of Normal Write-Through memory is in the ACPI code > > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > > CPUs treat write-through as non-cacheable under the hood, don't bother > > with the extra memory type here and just treat EFI_MEMORY_WT the same way > > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > The UEFI spec explicitly defines EFI_MEMORY_WT as Normal Outer-WT > Inner-WT (and even explicitly specifies the MAIR.Attr<n> value). I wonder if they just did that because the names match :( > In the UEFI 2.9 spec, see section 2.3.6.1 "Memory types", Table 2-5 > "Map: EFI Cacheability Attributes to AArch64 Memory Types". > > The UEFI 2.9 spec can be found at: > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf > > Given that is specified explicitly, and given that we don't know how > future CPUs will treat this equivalently, I don't think this change is > architecturally sound and I don't think there's wiggle-room to read the > spec as permitting this. At the same time, allocating a MAIR for this memory type just because the UEFI spec permits some theoretical future firmware to use it on some theoretical CPU design is pretty farcical in my opinion. Looking through current TRMs I've not been able to find a CPU that doesn't just emit Normal-NC for this memory type. How about I add a pr_warn() in this case, so that we can revisit it in the unlikely event that it ever comes up as an issue? Will
On Thu, May 06, 2021 at 11:46:41AM +0100, Will Deacon wrote: > On Thu, May 06, 2021 at 11:20:45AM +0100, Mark Rutland wrote: > > On Thu, May 06, 2021 at 10:50:33AM +0100, Will Deacon wrote: > > > The only user we have of Normal Write-Through memory is in the ACPI code > > > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > > > CPUs treat write-through as non-cacheable under the hood, don't bother > > > with the extra memory type here and just treat EFI_MEMORY_WT the same way > > > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > > > The UEFI spec explicitly defines EFI_MEMORY_WT as Normal Outer-WT > > Inner-WT (and even explicitly specifies the MAIR.Attr<n> value). > > I wonder if they just did that because the names match :( > > > In the UEFI 2.9 spec, see section 2.3.6.1 "Memory types", Table 2-5 > > "Map: EFI Cacheability Attributes to AArch64 Memory Types". > > > > The UEFI 2.9 spec can be found at: > > > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf > > > > Given that is specified explicitly, and given that we don't know how > > future CPUs will treat this equivalently, I don't think this change is > > architecturally sound and I don't think there's wiggle-room to read the > > spec as permitting this. > > At the same time, allocating a MAIR for this memory type just because the > UEFI spec permits some theoretical future firmware to use it on some > theoretical CPU design is pretty farcical in my opinion. Looking through > current TRMs I've not been able to find a CPU that doesn't just emit > Normal-NC for this memory type. > > How about I add a pr_warn() in this case, so that we can revisit it in the > unlikely event that it ever comes up as an issue? If we also could avoid mapping EFI_MEMORY_WT to anything, that'd be nicer, but just the warning is probably good enough, yes. Thanks, Mark.
On Thu, 6 May 2021 at 13:00, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, May 06, 2021 at 11:46:41AM +0100, Will Deacon wrote: > > On Thu, May 06, 2021 at 11:20:45AM +0100, Mark Rutland wrote: > > > On Thu, May 06, 2021 at 10:50:33AM +0100, Will Deacon wrote: > > > > The only user we have of Normal Write-Through memory is in the ACPI code > > > > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > > > > CPUs treat write-through as non-cacheable under the hood, don't bother > > > > with the extra memory type here and just treat EFI_MEMORY_WT the same way > > > > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > > > > > The UEFI spec explicitly defines EFI_MEMORY_WT as Normal Outer-WT > > > Inner-WT (and even explicitly specifies the MAIR.Attr<n> value). > > > > I wonder if they just did that because the names match :( > > > > > In the UEFI 2.9 spec, see section 2.3.6.1 "Memory types", Table 2-5 > > > "Map: EFI Cacheability Attributes to AArch64 Memory Types". > > > > > > The UEFI 2.9 spec can be found at: > > > > > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf > > > > > > Given that is specified explicitly, and given that we don't know how > > > future CPUs will treat this equivalently, I don't think this change is > > > architecturally sound and I don't think there's wiggle-room to read the > > > spec as permitting this. > > > > At the same time, allocating a MAIR for this memory type just because the > > UEFI spec permits some theoretical future firmware to use it on some > > theoretical CPU design is pretty farcical in my opinion. Looking through > > current TRMs I've not been able to find a CPU that doesn't just emit > > Normal-NC for this memory type. > > > > How about I add a pr_warn() in this case, so that we can revisit it in the > > unlikely event that it ever comes up as an issue? > > If we also could avoid mapping EFI_MEMORY_WT to anything, that'd be > nicer, but just the warning is probably good enough, yes. > EFI memory types are not exclusive. Most implementations use WB|WT|WC for all memory, and some even use WB|WT|WC|UC, which means that the OS can use whichever attributes it wants. So we should only warn in cases where WT is specified but WC is not, and I've never seen firmware that does this.
On Thu, May 06, 2021 at 12:14:55PM +0200, Ard Biesheuvel wrote: > On Thu, 6 May 2021 at 11:50, Will Deacon <will@kernel.org> wrote: > > > > The only user we have of Normal Write-Through memory is in the ACPI code > > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > > CPUs treat write-through as non-cacheable under the hood, don't bother > > with the extra memory type here and just treat EFI_MEMORY_WT the same way > > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Signed-off-by: Will Deacon <will@kernel.org> > > I don't have any objections to this change per se, but I will point > out that the UEFI spec describes the MAIR encodings, paragraph 2.3.6.1 > (in revision 2.8B). However, the paragraph in question provides no > context whatsoever, and so it is not clear whether it is normative, > and whether it applies to the boot time firmware only or to the OS as > well. > > So in summary, given that EFI_MEMORY_WT (which I have never seen being > used on ARM) should behave as expected when using the same MAIR > attributes as EFI_MEMORY_WC, with only a theoretical performance > impact, the change looks reasonable to me. In theory there's a slight difference between WT and WC/Normal-NC as reads are allowed to hit into the cache while for WC/Normal-NC they aren't (somehow implied on page B2-168 in the ARMv8 ARM G.a). Whether they must hit in the cache is not entirely clear, I don't think they have to. The mismatched aliases section doesn't guarantee coherency between accesses using different WC and WB attributes (point 1, attributes must be the same for both reads and writes). Appendix K4.1.1 also suggest that WT could be implemented as Normal-NC. So I don't think EFI can rely on any specific WT behaviour other than maybe slightly better performance. So: Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On Thu, 6 May 2021 at 15:25, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, May 06, 2021 at 12:14:55PM +0200, Ard Biesheuvel wrote: > > On Thu, 6 May 2021 at 11:50, Will Deacon <will@kernel.org> wrote: > > > > > > The only user we have of Normal Write-Through memory is in the ACPI code > > > when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) > > > CPUs treat write-through as non-cacheable under the hood, don't bother > > > with the extra memory type here and just treat EFI_MEMORY_WT the same way > > > as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Christoph Hellwig <hch@lst.de> > > > Signed-off-by: Will Deacon <will@kernel.org> > > > > I don't have any objections to this change per se, but I will point > > out that the UEFI spec describes the MAIR encodings, paragraph 2.3.6.1 > > (in revision 2.8B). However, the paragraph in question provides no > > context whatsoever, and so it is not clear whether it is normative, > > and whether it applies to the boot time firmware only or to the OS as > > well. > > > > So in summary, given that EFI_MEMORY_WT (which I have never seen being > > used on ARM) should behave as expected when using the same MAIR > > attributes as EFI_MEMORY_WC, with only a theoretical performance > > impact, the change looks reasonable to me. > > In theory there's a slight difference between WT and WC/Normal-NC as > reads are allowed to hit into the cache while for WC/Normal-NC they > aren't (somehow implied on page B2-168 in the ARMv8 ARM G.a). Whether > they must hit in the cache is not entirely clear, I don't think they > have to. The mismatched aliases section doesn't guarantee coherency > between accesses using different WC and WB attributes (point 1, > attributes must be the same for both reads and writes). Appendix K4.1.1 > also suggest that WT could be implemented as Normal-NC. > > So I don't think EFI can rely on any specific WT behaviour other than > maybe slightly better performance. > For the record, this mostly affects mappings created by the ACPI core, for things like RAS handling in firmware. EFI itself is not really a part of this, but given that ACPI lacks any way to annotate memory types for OpRegions (as x86 does not need this), we cross reference ACPI memory map requests with the EFI memory map to infer the right type. For RAS in particular, I can see how firmware might rely on non-cacheable behavior for correctness (e.g., for publishing error records in a way that ensures they are visible to other agents, such as a BMC), but I still don't think treating WT as WC will make a material difference here. > So: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index cada0b816c8a..aca5ee2a9e64 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -246,7 +246,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is * mapped to a corresponding MAIR attribute encoding. * The EFI memory attribute advises all possible capabilities - * of a memory region. We use the most efficient capability. + * of a memory region. */ u64 attr; @@ -254,9 +254,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr) attr = efi_mem_attributes(addr); if (attr & EFI_MEMORY_WB) return PAGE_KERNEL; - if (attr & EFI_MEMORY_WT) - return __pgprot(PROT_NORMAL_WT); - if (attr & EFI_MEMORY_WC) + if (attr & (EFI_MEMORY_WC | EFI_MEMORY_WT)) return __pgprot(PROT_NORMAL_NC); return __pgprot(PROT_DEVICE_nGnRnE); } @@ -340,9 +338,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) default: if (region->attribute & EFI_MEMORY_WB) prot = PAGE_KERNEL; - else if (region->attribute & EFI_MEMORY_WT) - prot = __pgprot(PROT_NORMAL_WT); - else if (region->attribute & EFI_MEMORY_WC) + else if (region->attribute & (EFI_MEMORY_WC | EFI_MEMORY_WT)) prot = __pgprot(PROT_NORMAL_NC); } }
The only user we have of Normal Write-Through memory is in the ACPI code when mapping memory regions advertised as EFI_MEMORY_WT. Since most (all?) CPUs treat write-through as non-cacheable under the hood, don't bother with the extra memory type here and just treat EFI_MEMORY_WT the same way as EFI_MEMORY_WC by mapping it to the Normal-NC memory type instead. Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/acpi.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)