Hello mate,
I have reviewed the patch you sent in detail. First of all, let me say that
you have done a great job :) The patch addresses a genuinely important
problem. In particular, enabling controllers that support only 32-bit PCI
addressing to operate transparently on systems with physical memory above 4
GiB is very valuable.
That said, I noticed a few potentially critical points during my review. It
is entirely possible that I have missed some details of the design, so
please consider these as questions requiring clarification rather than
definitive conclusions.
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;
However, unsigned long is 32 bits on i386. If the threshold representing
the 32-bit PCI DMA addressing limit is 0x100000000, or exactly 4 GiB, this
value cannot be represented by a 32-bit unsigned long.
If the value wraps to zero, the following check may effectively be disabled:
if (map->_dm_bounce_thresh != 0 &&
machaddr >= map->_dm_bounce_thresh)
In that case, the hurd-i386/PAE scenario that the patch specifically aims
to support may not work. I was therefore wondering why unsigned long was
chosen here. Would it be safer to retain bus_addr_t for these two fields,
or use an explicitly 64-bit type?
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.
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?
3.
Threshold checking in bus_dmamap_load_raw()
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?
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 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.
Was this eager allocation an intentional choice? It might be preferable to
allocate it in advance only when an option such as BUS_DMA_ALLOCNOW is
specified, and otherwise defer the allocation until it is actually needed.
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.
6.
Ensuring that the bounce buffer is allocated below the threshold
The bounce buffer is allocated through the following call:
bus_dmamem_alloc(t, ...);
However, from the changes in the patch, it is not immediately apparent
whether bus_dmamem_alloc() takes t->_bounce_thresh into account.
If this function can allocate the bounce pages above 4 GiB on amd64 or
i386/PAE, the bounce buffer itself will remain inaccessible to the device.
Is there an existing guarantee that the bounce memory will always be
allocated below the threshold?
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.
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.
Overall, I really like the approach and the problem you are trying to
solve. The copying flow for linear buffers is particularly clear. From my
perspective, the most critical point is the use of unsigned long and
whether it can genuinely represent the 4 GiB threshold on i386/PAE. Some of
the other points may simply be deliberate scope decisions, and I would be
interested to hear the reasoning behind them.
Great work again, mate :)
