On 31/08/2026 13:01, Alperen Erkan wrote:
1.
Representing the bounce threshold on i386/PAE
The first thing that caught my attention was that the following fields
are defined as |unsigned long|:
|struct bus_dma_tag { unsigned long _bounce_thresh; }; unsigned long
_dm_bounce_thresh;|
I don't see this. The patch clearly defines these with type 'unsigned
long long'.
2.
The |panic()| calls in |bus_dmamap_load_mbuf()| and
|bus_dmamap_load_uio()|
Both functions call |panic()| when direct mapping fails:
|panic("bus_dmamap_load_mbuf bounce buffers"); |
and:
|panic("bus_dmamap_load_uio bounce buffers"); |
The comments state that Hurd does not currently use these interfaces.
However, since the change is being added to the general rump DMA
implementation, a driver using either path in the future could cause
the entire process to crash simply by encountering a normal buffer
above 4 GiB.
In the future is indeed the point. Developing "in the future" would show
that work needs to be done in this area should the case you mention arise.
If bounce support for these paths is not going to be implemented yet,
returning an appropriate error code may be safer than calling
|panic()|. Have you confirmed that neither of these paths can be
reached by any of the current Hurd drivers?
Possibly 'panic' is excessive but there are a number of cases in the
rump dma code that already call panic for unsupported cases. I was being
consistent with those. I have not checked whether rumpnet or rumpusbdisk
use these interfaces but I've not heard of those failing on systems with
3.5G or more of RAM. Only a device that is limited to a 32 bit PCI
addressing capability can be affected by this change.
As far as I can see, |bus_dmamap_load()|, |bus_dmamap_load_mbuf()|,
and |bus_dmamap_load_uio()| have been considered, but no equivalent
bounce-threshold check has been added to |bus_dmamap_load_raw()|.
If raw segments contain DMA addresses directly, a segment above the
32-bit limit may be able to bypass the new mechanism. Even if applying
a bounce buffer to the raw-loading path is not possible, should the
supplied segments not at least be checked against |_dm_bounce_thresh|,
with an error returned when they exceed it?
I don't know but will check.
4.
Allocating a bounce buffer for every map in advance
When a bounce threshold is enabled, |bus_dmamap_create()| immediately
allocates a bounce buffer alongside the cookie:
|error = _bus_dma_alloc_bouncebuf(t, map, size, flags); |
This appears to mean that every map created with the relevant DMA tag
allocates low-address memory, regardless of whether bouncing is
actually required.
This is what the x86 arch code does.
This may cause unnecessary memory consumption, particularly for
drivers that create a large number of DMA maps. It could also cause a
buffer that would otherwise be mapped directly to fail during
|bus_dmamap_create()| merely because an unused bounce buffer could not
be allocated.
Theoretically, yes, but Hurd does not allocate large numbers of such maps.
5.
Treating every mapping failure as a need to bounce
Within |bus_dmamap_load()|, if the first |_bus_dmamap_load_buffer()|
call fails, the operation is retried through the bounce buffer without
distinguishing the reason for the failure.
However, the first mapping attempt may fail not only because the DMA
address exceeds the threshold, but also because the segment limit has
been exceeded or because of another mapping constraint. Of course,
this may be intentional, since a bounce buffer can also resolve some
segment-fragmentation problems.
Nevertheless, it would be helpful to clarify whether bouncing is
intended only for threshold violations or for every mapping failure.
If only threshold violations are meant to trigger it, a distinct error
code or an explicit “bounce required” result may make the behaviour
clearer.
This is what the x86 arch code does. I'm not trying to improve on
NetBSD; merely making what is available under x86 available under rump/x86.
6.
Ensuring that the bounce buffer is allocated below the threshold
The bounce buffer is allocated through the following call:
|bus_dmamem_alloc(t, ...); |
bus_dmamem_alloc is implemented within Hurd by rumpcomp_pci_dmalloc()
which only returns pages within the 32 bit physical address space.
7.
Architectures other than x86
The |bus_dmamap_sync()| implementation ends with the following
condition for non-x86 architectures:
|#else #error "bus_dmamap_sync unsupported" #endif |
If this file is also compiled as a general rump component on other
architectures, the change may prevent every non-x86 build from
compiling. This may be acceptable if the patch will be maintained
solely for Hurd’s i386 and amd64 targets. However, if general
portability is meant to be preserved, the previous |membar_sync()|
behaviour could perhaps remain as the fallback for other architectures.
This is a Hurd specific patch. This patch will not be sent to NetBSD for
inclusion. I'm very aware that it is not suitable for that. I will be
contacting NetBSD people initially with a description of the problem and
an outline of my solution. Code suggestions will be made if they wish
based on their feedback.
Finally, I would be interested to know which scenarios you used to
test the patch. In particular, if you tested it on an actual i386/PAE
environment using physical addresses above 4 GiB, could you share the
results? Tests covering |PREWRITE|, |POSTREAD|, partial offset/length
synchronisation, and repeated load/unload cycles would also be useful.
I have tested that the code works on i386/PAE and verified that the
bounce buffer code is triggered and that physical pages from beyond the
32 bit address space were supplied in the call to rump_psysread().
Mike.
