| Message ID | 20201021123437.21538-1-nsaenzjulienne@suse.de |
|---|---|
| Headers | show |
| Series | arm64: Default to 32-bit wide ZONE_DMA | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi Catalin, On Thu, 2020-10-22 at 19:06 +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 02:34:35PM +0200, Nicolas Saenz Julienne wrote: > > @@ -188,9 +186,11 @@ static phys_addr_t __init max_zone_phys(unsigned int zone_bits) > > static void __init zone_sizes_init(unsigned long min, unsigned long max) > > { > > unsigned long max_zone_pfns[MAX_NR_ZONES] = {0}; > > + unsigned int __maybe_unused dt_zone_dma_bits; > > > > #ifdef CONFIG_ZONE_DMA > > - zone_dma_bits = ARM64_ZONE_DMA_BITS; > > + dt_zone_dma_bits = ilog2(of_dma_get_max_cpu_address(NULL)); > > + zone_dma_bits = min(32U, dt_zone_dma_bits); > > A thought: can we remove the min here and expand ZONE_DMA to whatever > dt_zone_dma_bits says? More on this below. On most platforms we'd get PHYS_ADDR_MAX, or something bigger than the actual amount of RAM. Which would ultimately create a system wide ZONE_DMA. At first sight, I don't see it breaking dma-direct in any way. On the other hand, there is a big amount of MMIO devices out there that can only handle 32-bit addressing. Be it PCI cards or actual IP cores. To make things worse, this limitation is often expressed in the driver, not FW (with dma_set_mask() and friends). If those devices aren't behind an IOMMU we have be able to provide at least 32-bit addressable memory. See this comment from dma_direct_supported(): /* * Because 32-bit DMA masks are so common we expect every architecture * to be able to satisfy them - either by not supporting more physical * memory, or by providing a ZONE_DMA32. If neither is the case, the * architecture needs to use an IOMMU instead of the direct mapping. */ I think, for the common case, we're stuck with at least one zone spanning the 32-bit address space. > > arm64_dma_phys_limit = max_zone_phys(zone_dma_bits); > > max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit); > > #endif > > I was talking earlier to Ard and Robin on the ZONE_DMA32 history and the > need for max_zone_phys(). This was rather theoretical, the Seattle > platform has all RAM starting above 4GB and that led to an empty > ZONE_DMA32 originally. The max_zone_phys() hack was meant to lift > ZONE_DMA32 into the bottom of the RAM, on the assumption that such > 32-bit devices would have a DMA offset hardwired. We are not aware of > any such case on arm64 systems and even on Seattle, IIUC 32-bit devices > only work if they are behind an SMMU (so no hardwired offset). > > In hindsight, it would have made more sense on platforms with RAM above > 4GB to expand ZONE_DMA32 to cover the whole memory (so empty > ZONE_NORMAL). Something like: > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index a53c1e0fb017..7d5e3dd85617 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -187,8 +187,12 @@ static void __init reserve_elfcorehdr(void) > */ > static phys_addr_t __init max_zone_phys(unsigned int zone_bits) > { > - phys_addr_t offset = memblock_start_of_DRAM() & GENMASK_ULL(63, zone_bits); > - return min(offset + (1ULL << zone_bits), memblock_end_of_DRAM()); > + phys_addr_t zone_mask = 1ULL << zone_bits; > + > + if (!(memblock_start_of_DRAM() & zone_mask)) > + zone_mask = PHYS_ADDR_MAX; > + > + return min(zone_mask, memblock_end_of_DRAM()); > } > > static void __init zone_sizes_init(unsigned long min, unsigned long max) > > I don't think this makes any difference for ZONE_DMA unless a > broken DT or IORT reports the max CPU address below the start of DRAM. > > There's a minor issue if of_dma_get_max_cpu_address() matches > memblock_end_of_DRAM() but they are not a power of 2. We'd be left with > a bit of RAM at the end in ZONE_NORMAL due to ilog2 truncation. I agree it makes no sense to create more than one zone when the beginning of RAM is located above the 32-bit address space. I'm all for disregarding the possibility of hardwired offsets. As a bonus, as we already discussed some time ago, this is something that never played well with current dma-direct code[1]. Regards, Nicolas [1] https://lkml.org/lkml/2020/9/8/377
On Fri, Oct 23, 2020 at 05:27:49PM +0200, Nicolas Saenz Julienne wrote: > On Thu, 2020-10-22 at 19:06 +0100, Catalin Marinas wrote: > > On Wed, Oct 21, 2020 at 02:34:35PM +0200, Nicolas Saenz Julienne wrote: > > > @@ -188,9 +186,11 @@ static phys_addr_t __init max_zone_phys(unsigned int zone_bits) > > > static void __init zone_sizes_init(unsigned long min, unsigned long max) > > > { > > > unsigned long max_zone_pfns[MAX_NR_ZONES] = {0}; > > > + unsigned int __maybe_unused dt_zone_dma_bits; > > > > > > #ifdef CONFIG_ZONE_DMA > > > - zone_dma_bits = ARM64_ZONE_DMA_BITS; > > > + dt_zone_dma_bits = ilog2(of_dma_get_max_cpu_address(NULL)); > > > + zone_dma_bits = min(32U, dt_zone_dma_bits); > > > > A thought: can we remove the min here and expand ZONE_DMA to whatever > > dt_zone_dma_bits says? More on this below. > > On most platforms we'd get PHYS_ADDR_MAX, or something bigger than the actual > amount of RAM. Which would ultimately create a system wide ZONE_DMA. At first > sight, I don't see it breaking dma-direct in any way. > > On the other hand, there is a big amount of MMIO devices out there that can > only handle 32-bit addressing. Be it PCI cards or actual IP cores. To make > things worse, this limitation is often expressed in the driver, not FW (with > dma_set_mask() and friends). If those devices aren't behind an IOMMU we have be > able to provide at least 32-bit addressable memory. See this comment from > dma_direct_supported(): > > /* > * Because 32-bit DMA masks are so common we expect every architecture > * to be able to satisfy them - either by not supporting more physical > * memory, or by providing a ZONE_DMA32. If neither is the case, the > * architecture needs to use an IOMMU instead of the direct mapping. > */ > > I think, for the common case, we're stuck with at least one zone spanning the > 32-bit address space. You are right, I guess it makes sense to keep a 32-bit zone as not all devices would be described as such. > > > arm64_dma_phys_limit = max_zone_phys(zone_dma_bits); > > > max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit); > > > #endif > > > > I was talking earlier to Ard and Robin on the ZONE_DMA32 history and the > > need for max_zone_phys(). This was rather theoretical, the Seattle > > platform has all RAM starting above 4GB and that led to an empty > > ZONE_DMA32 originally. The max_zone_phys() hack was meant to lift > > ZONE_DMA32 into the bottom of the RAM, on the assumption that such > > 32-bit devices would have a DMA offset hardwired. We are not aware of > > any such case on arm64 systems and even on Seattle, IIUC 32-bit devices > > only work if they are behind an SMMU (so no hardwired offset). > > > > In hindsight, it would have made more sense on platforms with RAM above > > 4GB to expand ZONE_DMA32 to cover the whole memory (so empty > > ZONE_NORMAL). Something like: > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > > index a53c1e0fb017..7d5e3dd85617 100644 > > --- a/arch/arm64/mm/init.c > > +++ b/arch/arm64/mm/init.c > > @@ -187,8 +187,12 @@ static void __init reserve_elfcorehdr(void) > > */ > > static phys_addr_t __init max_zone_phys(unsigned int zone_bits) > > { > > - phys_addr_t offset = memblock_start_of_DRAM() & GENMASK_ULL(63, zone_bits); > > - return min(offset + (1ULL << zone_bits), memblock_end_of_DRAM()); > > + phys_addr_t zone_mask = 1ULL << zone_bits; > > + > > + if (!(memblock_start_of_DRAM() & zone_mask)) > > + zone_mask = PHYS_ADDR_MAX; > > + > > + return min(zone_mask, memblock_end_of_DRAM()); > > } > > > > static void __init zone_sizes_init(unsigned long min, unsigned long max) > > > > I don't think this makes any difference for ZONE_DMA unless a > > broken DT or IORT reports the max CPU address below the start of DRAM. > > > > There's a minor issue if of_dma_get_max_cpu_address() matches > > memblock_end_of_DRAM() but they are not a power of 2. We'd be left with > > a bit of RAM at the end in ZONE_NORMAL due to ilog2 truncation. > > I agree it makes no sense to create more than one zone when the beginning of > RAM is located above the 32-bit address space. I'm all for disregarding the > possibility of hardwired offsets. As a bonus, as we already discussed some time > ago, this is something that never played well with current dma-direct code[1]. > > [1] https://lkml.org/lkml/2020/9/8/377 Maybe this one is still worth fixing, at least for consistency. But it's not urgent. My diff above has a side-effect that if dt_zone_dma_bits is below the start of DRAM, ZONE_DMA gets expanded to PHYS_ADDR_MAX. If this was 32-bit, that's fine but if it was, say, 30-bit because of some firmware misdescription with RAM starting at 2GB, we end up with no ZONE_DMA32. I think max_zone_phys() could cap this at 32, as a safety mechanism: static phys_addr_t __init max_zone_phys(unsigned int zone_bits) { phys_addr_t zone_mask = (1ULL << zone_bits) - 1; phys_addr_t phys_start = memblock_start_of_DRAM(); if (!(phys_start & U32_MAX)) zone_mask = PHYS_ADDR_MAX; else if (!(phys_start & zone_mask)) zone_mask = U32_MAX; return min(zone_mask + 1, memblock_end_of_DRAM()); } Assuming I got the shifting right, arm64_dma_phys_limit becomes: arm64_dma_phys_limit = max_zone_phys(zone_dma_bits, 32);
Hi, On 10/21/20 7:34 AM, Nicolas Saenz Julienne wrote: > Using two distinct DMA zones turned out to be problematic. Here's an > attempt go back to a saner default. > > I tested this on both a RPi4 and QEMU. I've tested this in ACPI mode on the rpi4 (4+8G with/without the 3G limiter) as well, with Ard's IORT patch. Nothing seems to have regressed. Thanks, Tested-by: Jeremy Linton <jeremy.linton@arm.com> > > --- > > Changes since v3: > - Drop patch adding define in dma-mapping > - Address small review changes > - Update Ard's patch > - Add new patch removing examples from mmzone.h > > Changes since v2: > - Introduce Ard's patch > - Improve OF dma-ranges parsing function > - Add unit test for OF function > - Address small changes > - Move crashkernel reservation later in boot process > > Changes since v1: > - Parse dma-ranges instead of using machine compatible string > > Ard Biesheuvel (1): > arm64: mm: Set ZONE_DMA size based on early IORT scan > > Nicolas Saenz Julienne (6): > arm64: mm: Move reserve_crashkernel() into mem_init() > arm64: mm: Move zone_dma_bits initialization into zone_sizes_init() > of/address: Introduce of_dma_get_max_cpu_address() > of: unittest: Add test for of_dma_get_max_cpu_address() > arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges > mm: Remove examples from enum zone_type comment > > arch/arm64/mm/init.c | 16 ++++++------ > drivers/acpi/arm64/iort.c | 52 +++++++++++++++++++++++++++++++++++++++ > drivers/of/address.c | 42 +++++++++++++++++++++++++++++++ > drivers/of/unittest.c | 18 ++++++++++++++ > include/linux/acpi_iort.h | 4 +++ > include/linux/mmzone.h | 20 --------------- > include/linux/of.h | 7 ++++++ > 7 files changed, 130 insertions(+), 29 deletions(-) >
On Wed, Oct 21, 2020 at 02:34:36PM +0200, Nicolas Saenz Julienne wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > We recently introduced a 1 GB sized ZONE_DMA to cater for platforms > incorporating masters that can address less than 32 bits of DMA, in > particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has > peripherals that can only address up to 1 GB (and its PCIe host > bridge can only access the bottom 3 GB) > > Instructing the DMA layer about these limitations is straight-forward, > even though we had to fix some issues regarding memory limits set in > the IORT for named components, and regarding the handling of ACPI _DMA > methods. However, the DMA layer also needs to be able to allocate > memory that is guaranteed to meet those DMA constraints, for bounce > buffering as well as allocating the backing for consistent mappings. > > This is why the 1 GB ZONE_DMA was introduced recently. Unfortunately, > it turns out the having a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes > problems with kdump, and potentially in other places where allocations > cannot cross zone boundaries. Therefore, we should avoid having two > separate DMA zones when possible. > > So let's do an early scan of the IORT, and only create the ZONE_DMA > if we encounter any devices that need it. This puts the burden on > the firmware to describe such limitations in the IORT, which may be > redundant (and less precise) if _DMA methods are also being provided. > However, it should be noted that this situation is highly unusual for > arm64 ACPI machines. Also, the DMA subsystem still gives precedence to > the _DMA method if implemented, and so we will not lose the ability to > perform streaming DMA outside the ZONE_DMA if the _DMA method permits > it. > > Cc: Jeremy Linton <jeremy.linton@arm.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > [nsaenz: Rebased, removed documentation change and add declaration in acpi_iort.h] > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > Changes since v3: > - Use min_not_zero() > - Check ACPI revision > - Remove unnecessary #ifdef in zone_sizes_init() > > arch/arm64/mm/init.c | 3 ++- > drivers/acpi/arm64/iort.c | 52 +++++++++++++++++++++++++++++++++++++++ > include/linux/acpi_iort.h | 4 +++ > 3 files changed, 58 insertions(+), 1 deletion(-) Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 94e38f99748b..f5d4f85506e4 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -29,6 +29,7 @@ > #include <linux/kexec.h> > #include <linux/crash_dump.h> > #include <linux/hugetlb.h> > +#include <linux/acpi_iort.h> > > #include <asm/boot.h> > #include <asm/fixmap.h> > @@ -190,7 +191,7 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max) > > #ifdef CONFIG_ZONE_DMA > dt_zone_dma_bits = ilog2(of_dma_get_max_cpu_address(NULL)); > - zone_dma_bits = min(32U, dt_zone_dma_bits); > + zone_dma_bits = min3(32U, dt_zone_dma_bits, acpi_iort_get_zone_dma_size()); > arm64_dma_phys_limit = max_zone_phys(zone_dma_bits); > max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit); > #endif > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 9929ff50c0c0..05fe4a076bab 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1718,3 +1718,55 @@ void __init acpi_iort_init(void) > > iort_init_platform_devices(); > } > + > +#ifdef CONFIG_ZONE_DMA > +/* > + * Check the IORT whether any devices exist whose DMA mask is < 32 bits. > + * If so, return the smallest value encountered, or 32 otherwise. > + */ > +unsigned int __init acpi_iort_get_zone_dma_size(void) > +{ > + struct acpi_table_iort *iort; > + struct acpi_iort_node *node, *end; > + acpi_status status; > + u8 limit = 32; > + int i; > + > + if (acpi_disabled) > + return limit; > + > + status = acpi_get_table(ACPI_SIG_IORT, 0, > + (struct acpi_table_header **)&iort); > + if (ACPI_FAILURE(status)) > + return limit; > + > + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset); > + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length); > + > + for (i = 0; i < iort->node_count; i++) { > + if (node >= end) > + break; > + > + switch (node->type) { > + struct acpi_iort_named_component *ncomp; > + struct acpi_iort_root_complex *rc; > + > + case ACPI_IORT_NODE_NAMED_COMPONENT: > + ncomp = (struct acpi_iort_named_component *)node->node_data; > + limit = min_not_zero(limit, ncomp->memory_address_limit); > + break; > + > + case ACPI_IORT_NODE_PCI_ROOT_COMPLEX: > + if (node->revision < 1) > + break; > + > + rc = (struct acpi_iort_root_complex *)node->node_data; > + limit = min_not_zero(limit, rc->memory_address_limit); > + break; > + } > + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); > + } > + acpi_put_table(&iort->header); > + return limit; > +} > +#endif > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > index 20a32120bb88..7d2e184f0d4d 100644 > --- a/include/linux/acpi_iort.h > +++ b/include/linux/acpi_iort.h > @@ -38,6 +38,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size); > const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > const u32 *id_in); > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head); > +unsigned int acpi_iort_get_zone_dma_size(void); > #else > static inline void acpi_iort_init(void) { } > static inline u32 iort_msi_map_id(struct device *dev, u32 id) > @@ -55,6 +56,9 @@ static inline const struct iommu_ops *iort_iommu_configure_id( > static inline > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head) > { return 0; } > + > +static inline unsigned int acpi_iort_get_zone_dma_size(void) > +{ return 32; } > #endif > > #endif /* __ACPI_IORT_H__ */ > -- > 2.28.0 >
On Fri, 2020-10-23 at 14:05 -0500, Jeremy Linton wrote: > Hi, > > On 10/21/20 7:34 AM, Nicolas Saenz Julienne wrote: > > Using two distinct DMA zones turned out to be problematic. Here's an > > attempt go back to a saner default. > > > > I tested this on both a RPi4 and QEMU. > > I've tested this in ACPI mode on the rpi4 (4+8G with/without the 3G > limiter) as well, with Ard's IORT patch. Nothing seems to have regressed. > > Thanks, > > Tested-by: Jeremy Linton <jeremy.linton@arm.com> Thanks!
On 2020/10/21 20:34, Nicolas Saenz Julienne wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > We recently introduced a 1 GB sized ZONE_DMA to cater for platforms > incorporating masters that can address less than 32 bits of DMA, in > particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has > peripherals that can only address up to 1 GB (and its PCIe host > bridge can only access the bottom 3 GB) > > Instructing the DMA layer about these limitations is straight-forward, > even though we had to fix some issues regarding memory limits set in > the IORT for named components, and regarding the handling of ACPI _DMA > methods. However, the DMA layer also needs to be able to allocate > memory that is guaranteed to meet those DMA constraints, for bounce > buffering as well as allocating the backing for consistent mappings. > > This is why the 1 GB ZONE_DMA was introduced recently. Unfortunately, > it turns out the having a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes > problems with kdump, and potentially in other places where allocations > cannot cross zone boundaries. Therefore, we should avoid having two > separate DMA zones when possible. > > So let's do an early scan of the IORT, and only create the ZONE_DMA > if we encounter any devices that need it. This puts the burden on > the firmware to describe such limitations in the IORT, which may be > redundant (and less precise) if _DMA methods are also being provided. > However, it should be noted that this situation is highly unusual for > arm64 ACPI machines. Also, the DMA subsystem still gives precedence to > the _DMA method if implemented, and so we will not lose the ability to > perform streaming DMA outside the ZONE_DMA if the _DMA method permits > it. > > Cc: Jeremy Linton <jeremy.linton@arm.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > [nsaenz: Rebased, removed documentation change and add declaration in acpi_iort.h] > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > Changes since v3: > - Use min_not_zero() > - Check ACPI revision > - Remove unnecessary #ifdef in zone_sizes_init() > > arch/arm64/mm/init.c | 3 ++- > drivers/acpi/arm64/iort.c | 52 +++++++++++++++++++++++++++++++++++++++ > include/linux/acpi_iort.h | 4 +++ Acked-by: Hanjun Guo <guohanjun@huawei.com>