Page MenuHomeFreeBSD

Add a wrapper API for static DMA allocations to bus_dma(9).
AcceptedPublic

Authored by jhb on Mar 22 2016, 1:12 PM.

Details

Reviewers
kib
scottl
imp
cem
Summary

Add a wrapper API for static DMA allocations to bus_dma(9).

Currently allocating a static DMA allocation (e.g. for a descriptor ring)
requires three steps: creating a tag, allocating memory, and loading the
memory to obtain the bus address. Freeing an allocation requires three
steps as well (map unload, memory free, and tag destroy). This results in
a lot of boilerplate code in many drivers. This wrapper API aims to
simplify the interface by using a single function for each call. Note that
the wrapper API only supports single-segment allocations. This is
sufficient for the majority of drivers. Drivers needing multiple segments
can still use the existing API.

First, a 'struct bus_dmamem' is added which contains all of the information
about a static DMA mapping including the tag, map, virtual address, and
bus (DMA) address.

Second, a 'struct bus_dmamem_args' structure is defined to hold optional
restrictions on a mapping such as address restrictions, alignment, and
boundary. This structure follows the model of the recently added
make_dev_args structure used with make_dev_s() so that it can be
extended in the future to add additional constraints while preserving the
ABI. I think it also makes the code more readable as it allows you to
say:

args.dma_alignment = 64;

instead of having to remember that the Nth argument to bus_dma_tag_create()
is alignment. Also, the argument structure is initialized to specify no
restrictions. The caller is only required to explicitly set fields to
enforce needed restrictions. A NULL args pointer can be passed to
bus_dma_mem_alloc() to indicate that no restrictions are needed beyond those
in the parent tag.

Third, bus_dma_mem_alloc() allocates memory for a static DMA allocation
and saves the relevant information in the caller-supplied
'struct bus_dmamem'.

Fourth, bus_dma_mem_free() frees the memory and resources (map and tag)
used by a static DMA allocation. Note that if the 'struct bus_dmamem' is
initialized to zero before allocation (which happens automatically if it
is embedded in a softc), then this function can be safely called
unconditionally.

Finally, a simple bus_dma_mem_sync() wrapper has been added around
bus_dma_map_sync() that uses the map and tag stored in a 'struct bus_dmamem'.

This diff includes sample conversions of the ndis(4) layer and the xl(4)
driver. Note that the code to allocate a ring is now much shorter and
the ring-specific restrictions are now more obvious in the code:

/*

  • Now allocate a chunk of DMA-able memory for the DMA
  • descriptor lists. All of our lists are allocated as a
  • contiguous block of memory.
	 */

bus_dma_mem_args_init(&args);
args.dma_alignment = 8;
args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_RX_LIST_SZ, 0, &args,

	    &sc->xl_ldata.xl_rx_ring);

if (error) {

		device_printf(dev, "failed to allocate rx ring\n");
		goto fail;

}

error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_TX_LIST_SZ, 0, &args,

	    &sc->xl_ldata.xl_tx_ring);

if (error) {

		device_printf(dev, "failed to allocate tx ring\n");
		goto fail;

}

If this API is liked, we may want to implement an alternate version of
bus_dma_tag_create() that accepts an args structure of restrictions and
reimplement bus_dma_tag_create() in terms of that. I think it would yield
similar readability improvements in terms of only having to specify
restrictions that are used while also provide room for adding additional
constraints in the future (such as for NUMA). This would be a bit of a
different take than doing setter/getters on tags directly, but if attributes
are only needed by consumers during tag creation the args approach can work
fine. (Note that this is similar to what pthreads does with all of its
attr objects.)

Test Plan
  • Only compile tested so far. I have used an older version of this API in an out-of-tree driver though.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 2975
Build 3003: arc lint + arc unit

Event Timeline

jhb retitled this revision from to Add a wrapper API for static DMA allocations to bus_dma(9)..Mar 22 2016, 1:12 PM
jhb updated this object.
jhb edited the test plan for this revision. (Show Details)
jhb added reviewers: kib, scottl, imp.
jhb updated this revision to Diff 14514.
jhb updated this revision to Diff 14522.Mar 22 2016, 5:42 PM
  • Initialize xl_rx_list and xl_tx_list.
kib edited edge metadata.Mar 23 2016, 7:20 AM
kib accepted this revision.

IMO this is a useful and welcomed drivers KPI addition. I like it, but also I much more like and anticipate tag creation change you suggested to use an expandable structure.

For commit, or shortly after, the new KPI needs man page update.

This revision is now accepted and ready to land.Mar 23 2016, 7:20 AM
scottl edited edge metadata.Mar 23 2016, 10:17 PM

I like the general idea a lot. I'd like to improve it further, though. You're basically hiding the tag away from the new API, which is good IMHO, but you don't quite go far enough. For your example:

bus_dma_mem_args_init(&args);
args.dma_alignment = 8;
args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_RX_LIST_SZ, 0, &args,
	    &sc->xl_ldata.xl_rx_ring);

How about this:

bus_dma_mem_args_init(dev, &args);
args.dma_alignment = 8;
args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
error = bus_dma_mem_alloc(&args, XL_RX_LIST_SZ, 0,
	    &sc->xl_ldata.xl_rx_ring);

As for dealing with the tag in general, I had envisioned dynamic keyword getters and setters, i.e.:

error = bus_dma_get_tag_from_dev(dev, &tag);

or

error = bus_dma_get_tag_from_parent(parent, &tag);

and then

error = bus_dma_set_value(&tag, "alignment", 8);

or

struct {
        char *key;
        uintptr_t val;
} dma_attrs[] = {{ "alignment", 8}, {"dma_lowaddr", BUS_SPACE_MAXADDR_32BIT}};
error = bus_dma_set_values(&tag, dma_attrs, 2);

The API would then parse the keywords and set/get the appropriate value as either a literal or as a pointer (the name of the keyword would imply the data type to return). In both busdma and in CAM, I'm looking to get rid of the gigantic argument lists and attribute structures that are a huge headache for both programming and for API/ABI compatibility. I personally like a KVP style API for something that's not performance sensitive because it makes the API infinitely flexible without having to worry about headers anymore.

jhb added a comment.Aug 29 2016, 2:22 AM

I like the general idea a lot. I'd like to improve it further, though. You're basically hiding the tag away from the new API, which is good IMHO, but you don't quite go far enough. For your example:

bus_dma_mem_args_init(&args);
args.dma_alignment = 8;
args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_RX_LIST_SZ, 0, &args,
	    &sc->xl_ldata.xl_rx_ring);

How about this:

bus_dma_mem_args_init(dev, &args);
args.dma_alignment = 8;
args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
error = bus_dma_mem_alloc(&args, XL_RX_LIST_SZ, 0,
	    &sc->xl_ldata.xl_rx_ring);

One issue with this is that not all static allocations use bus_get_dma_tag() as the parent tag. Some device drivers create a device-wide tag that the static allocations inherit from, so I think the tag should still be explicit. (I would probably like to convert all in-tree consumers to the new API eventually so would like to be able to handle all of the use cases of the current API.)

As for dealing with the tag in general, I had envisioned dynamic keyword getters and setters, i.e.:

error = bus_dma_get_tag_from_dev(dev, &tag);

or

error = bus_dma_get_tag_from_parent(parent, &tag);

and then

error = bus_dma_set_value(&tag, "alignment", 8);

or

struct {
        char *key;
        uintptr_t val;
} dma_attrs[] = {{ "alignment", 8}, {"dma_lowaddr", BUS_SPACE_MAXADDR_32BIT}};
error = bus_dma_set_values(&tag, dma_attrs, 2);

The API would then parse the keywords and set/get the appropriate value as either a literal or as a pointer (the name of the keyword would imply the data type to return). In both busdma and in CAM, I'm looking to get rid of the gigantic argument lists and attribute structures that are a huge headache for both programming and for API/ABI compatibility. I personally like a KVP style API for something that's not performance sensitive because it makes the API infinitely flexible without having to worry about headers anymore.

Interesting. I had originally read the values as being strings as well which was not going to be a PITA, but a uintptr_t for the value will probably work fine. You could conceivably use nvlist for this as well if you wanted to use an existing API for managing the tag attributes. (That might be too verbose in terms of boilerplate.)

Pending our discussion on IRC, would you mind if I took over this review and extended it?

cem added a subscriber: cem.Aug 3 2018, 4:56 PM
cem accepted this revision.Aug 4 2018, 6:04 AM

I like the general approach. The generic code changes look good. I did not review the driver changes. I agree that a similar args-style method should be added to bus_dma_tag_create().

My main concerns are around naming and documenting things. If we just throw these new routines in bus_dma.9, it's going to be even more confusing than it already is. I think we need to split bus_dma into dynamic and static allocations, at a minimum, and probably keep this new stuff logically separate from the lower level static API.

Also, I think we have a good opportunity here to name the "lowaddr" and "highaddr" more descriptively. I don't have any great ideas in mind, but I'll throw out e.g. "lowexcludeaddr" to start.

sys/sys/bus_dma.h
368–369

These could be named more descriptively — they really describe an excluded region, right?

388

There's no reason this should not be an inline function instead.

cem added inline comments.Aug 9 2018, 7:51 PM
sys/sys/bus_dma.h
373–375

I guess this could be a static inline function too.

jhb added inline comments.Aug 9 2018, 7:59 PM
sys/sys/bus_dma.h
368–369

These match the args to bus_dma_tag_create. We could perhaps boil that ocean when revisiting bus_dma_tag_create, but I think it is worth it to be consistent for now.

373–375

Hmm, possibly. This is the style that make_dev_s() uses which is what I modeled this on. It is very important that it uses the caller's size and not a size compiled into the kernel (so if the compiler ever chose to not inline it by treating the 'inline' as advisory only and still generating a real function that it called it would be fatal)

cem added inline comments.Aug 9 2018, 8:08 PM
sys/sys/bus_dma.h
368–369

Yeah, I understood where they came from, but I don't think we care about matching names exactly with bus_dma_tag_create.

C consumers do not use parameter names in invocation, so it's not like any driver today initializes with anything other than the equivalent of a struct literal with an ordered list of member values.

We can change it later, but it's always easier to change before anything is written down, so to speak. Any name change to struct members won't be MFCable, for example, and that pretty much poisons ever changing it once the struct lands in a stable branch.

373–375

Yeah, I agree that inline is definitely not safe on its own. I think static inline is safe — I don't believe the compiler is allowed to treat the static as advisory.