Re: Adding a "-1" into https://reviews.freebsd.org/D25219 's code looks to make uefi/ACPI handle USB3 SSD reliably

On 2020-Oct-7, at 00:26, Mark Millard <marklmi at yahoo.com> wrote:

> https://reviews.freebsd.org/D25219 has (in part)
> 
>> +	if (bus_dma_tag_create(NULL, 1, 0,
>> +		limits.lowaddr, BUS_SPACE_MAXADDR, NULL, NULL,
> 
> Based on sysctl hw.busdma output and testing a change for the u-boot/DTB/fdt code context that worked in testing so far, I've tried using limits.lowaddr-1 in the above ACPI handling code instead.
> 
> It has worked so far.
> 
> So both existing implementations have the same basic
> problem as far as I can tell: limits.lowaddr too large
> by one, so identifying the wrong page.
> 
> 
> 
> What sysctl showed me was the likes of (before
> changes that lead to lack of zone2 for u-boot/dtb/fdt):
> 
> . . .
> hw.busdma.zone2.lowaddr: 0x3c000fff
> . . .
> hw.busdma.zone1.lowaddr: 0x3fffffff
> . . .
> hw.busdma.zone0.lowaddr: 0xffffffff
> . . .
> 
> So I've guessed that lowaddr should identify the
> end page of the possibly-use-it-directly region,
> not the first do-not-use-it-directly page. If I've
> guessed wrong, at most it would bounce one page
> that it could avoid bouncing. But, if I guessed
> correct, it might bounce a page that it should
> instead of not doing so. Thus the "-1" addition.
> 
> For reference, after the first duplicate-and-diff test
> for uefi/ACPI:
> 
> # sysctl hw.busdma
> hw.busdma.zone0.alignment: 4096
> hw.busdma.zone0.lowaddr: 0xbfffffff
> hw.busdma.zone0.total_deferred: 0
> hw.busdma.zone0.total_bounced: 762568
> hw.busdma.zone0.active_bpages: 12
> hw.busdma.zone0.reserved_bpages: 0
> hw.busdma.zone0.free_bpages: 824
> hw.busdma.zone0.total_bpages: 836
> hw.busdma.total_bpages: 836
> 
> I'll note that "total_bounced" need not be the
> a page count: it is incremented by 1 after
> the loop for a bounce, not inside the loop.
> Lots of pages of data were bounced.

For reference, the patch that I'm running is below.
It has a comment and the addition of the "-1" compared
to what is in https://reviews.freebsd.org/D25219 .
I've not observed any problems so far in my use.
(E-mail may make whitespace odd below.)

# svnlite diff /usr/src/sys/dev/acpica/acpi.c
Index: /usr/src/sys/dev/acpica/acpi.c
===================================================================
--- /usr/src/sys/dev/acpica/acpi.c	(revision 365932)
+++ /usr/src/sys/dev/acpica/acpi.c	(working copy)
@@ -184,6 +184,7 @@
 static void	acpi_hint_device_unit(device_t acdev, device_t child,
 		    const char *name, int *unitp);
 static void	acpi_reset_interfaces(device_t dev);
+static bus_dma_tag_t acpi_get_dma_tag(device_t dev, device_t child);
 
 static device_method_t acpi_methods[] = {
     /* Device interface */
@@ -218,6 +219,7 @@
     DEVMETHOD(bus_hint_device_unit,	acpi_hint_device_unit),
     DEVMETHOD(bus_get_cpus,		acpi_get_cpus),
     DEVMETHOD(bus_get_domain,		acpi_get_domain),
+    DEVMETHOD(bus_get_dma_tag,		acpi_get_dma_tag),
 
     /* ACPI bus */
     DEVMETHOD(acpi_id_probe,		acpi_device_id_probe),
@@ -431,6 +433,123 @@
     return (0);
 }
 
+struct dma_limits {
+	bus_addr_t lowaddr;
+};
+
+static ACPI_STATUS
+dma_on_resource(ACPI_RESOURCE *res, void *arg)
+{
+	struct dma_limits *limits = arg;
+	bus_addr_t min, len;
+
+	/*
+	 * The minimum and maximum are device-side. To get the CPU-side minimum,
+	 * we add the translation offset. This can overflow to signify lower addresses
+	 * on the CPU than the device, e.g. "Bus 0xC0000000 -> CPU 0x00000000"
+	 * on the RPi4 is represented as 0xC0000000 min + 0xFFFFFFFF40000000 offset.
+	 */
+
+	switch (res->Type) {
+	case ACPI_RESOURCE_TYPE_ADDRESS16:
+		min = (uint16_t)(res->Data.Address16.Address.Minimum +
+		    res->Data.Address16.Address.TranslationOffset);
+		len = res->Data.Address16.Address.AddressLength;
+		break;
+	case ACPI_RESOURCE_TYPE_ADDRESS32:
+		min = (uint32_t)(res->Data.Address32.Address.Minimum +
+		    res->Data.Address32.Address.TranslationOffset);
+		len = res->Data.Address32.Address.AddressLength;
+		break;
+	case ACPI_RESOURCE_TYPE_ADDRESS64:
+		min = (uint64_t)(res->Data.Address64.Address.Minimum +
+		    res->Data.Address64.Address.TranslationOffset);
+		len = res->Data.Address64.Address.AddressLength;
+		break;
+	case ACPI_RESOURCE_TYPE_END_TAG:
+		return (AE_OK);
+	default:
+		printf("ACPI: warning: DMA limit with unsupported resource type %d\n",
+			res->Type);
+		return (AE_OK);
+	}
+
+	if (min != 0)
+		printf("ACPI: warning: DMA limit with non-zero minimum address"
+		    " not supported yet\n");
+
+	limits->lowaddr = MIN(limits->lowaddr, min + len);
+
+	return (AE_OK);
+}
+
+static int
+get_dma_tag(ACPI_HANDLE handle, bus_dma_tag_t *result)
+{
+	ACPI_HANDLE parent;
+	unsigned int coherent;
+	struct dma_limits limits = {
+		.lowaddr = BUS_SPACE_MAXADDR,
+	};
+
+	if (ACPI_FAILURE(AcpiWalkResources(handle, "_DMA",
+	    dma_on_resource, (void *)&limits))) {
+		/* Inherit resources from parent handle if we don't have our own */
+		if (ACPI_SUCCESS(AcpiGetParent(handle, &parent)))
+			return (get_dma_tag(parent, result));
+
+		/* The root (which has no parent) has no restrictions */
+		*result = NULL;
+		return (0);
+	}
+
+	if (ACPI_FAILURE(acpi_GetInteger(handle, "_CCA", &coherent)))
+		coherent = 0;
+
+	/*
+	 * First off, a note about lowaddr values. What sysctl showed
+	 * me was (during investigation of u-boot/dtb/fdt oddities
+	 * that had the same style of solution that I am trying here):
+	 *
+	 * . . .
+	 * hw.busdma.zone2.lowaddr: 0x3c000fff
+	 * . . .
+	 * hw.busdma.zone1.lowaddr: 0x3fffffff
+	 * . . .
+	 * hw.busdma.zone0.lowaddr: 0xffffffff
+	 * . . .
+	 *
+	 * So I've guessed that lowaddr should identify the
+	 * end page of the possibly-use-it region, not the
+	 * first do-not-use-it page. If I've guessed wrong,
+	 * at most it would bounce one page that it could
+	 * avoid bouncing.  But, if I guessed correct, it
+	 * might bounce a page that it should instead of
+	 * not doing so. Thus the "-1" below.
+	 */
+	if (bus_dma_tag_create(NULL, 1, 0,
+		limits.lowaddr-1, BUS_SPACE_MAXADDR, NULL, NULL,
+		BUS_SPACE_MAXSIZE, BUS_SPACE_UNRESTRICTED, BUS_SPACE_MAXSIZE,
+		coherent ? BUS_DMA_COHERENT : 0, NULL, NULL,
+		result) != 0)
+		return (ENOMEM);
+
+	return (0);
+}
+
+static bus_dma_tag_t
+acpi_get_dma_tag(device_t dev, device_t child)
+{
+	bus_dma_tag_t result;
+
+	if (get_dma_tag(acpi_get_handle(child), &result) != 0) {
+		device_printf(child, "could not get ACPI DMA limits\n");
+		return (NULL);
+	}
+
+	return (result);
+}
+
 /*
  * Fetch some descriptive data from ACPI to put in our attach message.
  */

I later found the code that confirms the need to do the "-1" . . .

sys/arm64/arm64/busdma_machdep.c 's common_bus_dma_tag_create has:

common->lowaddr = trunc_page((vm_paddr_t)lowaddr) + (PAGE_SIZE - 1);
common->highaddr = trunc_page((vm_paddr_t)highaddr) + (PAGE_SIZE - 1);

and so forces reference to the last byte of the page identified by lowaddr
(and highaddr). Thus lowaddr needs to identify the last page that can avoid
bouncing (or earlier).

===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)

_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arm
To unsubscribe, send any mail to "[email protected]"