cam: Move bus_dmamap_load_ccb into cam.c.

[jhb]

Summary

This routine is specific to CAM and no longer assumes any internal
bus_dma knowledge as it is simple wrapper around bus_dmamap_load_mem.

Fixes: 60381fd1ee86 memdesc: Retire MEMDESC_CCB.

[imp]

So MINIMAL + nvme + nvd? That works with this?

I'm not sure how that could work. I'm starting to go the other way an include memdesc_ccb() when #ifdef _KERNEL in cam.h as a static inline. Unlike what I said on the mailing list, I see it's only used in one place now that I grep for it.

[kib]

I suspect at least sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c needs to grow MODULE_DEPEND("cam"). Seems that other uses of the function are already fine.

[kib]

In D41058#934894, @imp wrote:

So MINIMAL + nvme + nvd? That works with this?

I'm not sure how that could work. I'm starting to go the other way an include memdesc_ccb() when #ifdef _KERNEL in cam.h as a static inline. Unlike what I said on the mailing list, I see it's only used in one place now that I grep for it.

nvme already depends on cam. Perhaps nvd should also have MODULE_DEPEND added.

[imp]

In D41058#934896, @kib wrote:

In D41058#934894, @imp wrote:

So MINIMAL + nvme + nvd? That works with this?

I'm not sure how that could work. I'm starting to go the other way an include memdesc_ccb() when #ifdef _KERNEL in cam.h as a static inline. Unlike what I said on the mailing list, I see it's only used in one place now that I grep for it.

nvme already depends on cam. Perhaps nvd should also have MODULE_DEPEND added.

No. That won't cause cam to br loaded and this change wouldn't leak.

[kib]

I checked that the move still allows for nvd.ko to load. Please commit to unbreak stripped kernels.

[imp]

In D41058#935001, @kib wrote:

I checked that the move still allows for nvd.ko to load. Please commit to unbreak stripped kernels.

I was talking about a MINIMAL kernel with nvme and nvd added to it, not loaded as modules.

[kib]

In D41058#935002, @imp wrote:

In D41058#935001, @kib wrote:

I checked that the move still allows for nvd.ko to load. Please commit to unbreak stripped kernels.

I was talking about a MINIMAL kernel with nvme and nvd added to it, not loaded as modules.

If you add nvme to static kernel config, you have to add CAM, since nvme proclaims dependency on it:

nvme.c:
DECLARE_MODULE(nvme, nvme_mod, SI_SUB_DRIVERS, SI_ORDER_FIRST);
MODULE_VERSION(nvme, 1);
MODULE_DEPEND(nvme, cam, 1, 1, 1);

[jhb]

In D41058#934895, @kib wrote:

I suspect at least sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c needs to grow MODULE_DEPEND("cam"). Seems that other uses of the function are already fine.

It probably needs that for other reasons as well (e.g. calls to other CAM functions like xpt_alloc_ccb_nowait and xpt_rescan)

[imp]

I'll have to test my example to make sure it links.

[imp]

And it's broken as predicted

[imp]

The broken test case:

include MINIMAL
device nvme
device nvd

[imp]

In D41058#935003, @kib wrote:

In D41058#935002, @imp wrote:

In D41058#935001, @kib wrote:

I checked that the move still allows for nvd.ko to load. Please commit to unbreak stripped kernels.

I was talking about a MINIMAL kernel with nvme and nvd added to it, not loaded as modules.

If you add nvme to static kernel config, you have to add CAM, since nvme proclaims dependency on it:

nvme.c:
DECLARE_MODULE(nvme, nvme_mod, SI_SUB_DRIVERS, SI_ORDER_FIRST);
MODULE_VERSION(nvme, 1);
MODULE_DEPEND(nvme, cam, 1, 1, 1);

No, you don't need to. This is only for the module, not the static kernel. nvme + nvd without cam works today before this change and the fix to keep it working is trivial.
People will complain to me when it breaks and won't care for this explanation.

[imp]

Two proposed fixes

https://reviews.freebsd.org/D41077 which makes loading the dma for a nvme request a function pointer
https://reviews.freebsd.org/D41078 which moves renames memdesc_ccb to cam_memdesc_ccb and moves it to cam_cch.h

[imp]

Also: config(8) and sys/modules have always had mismatches like this... and they both suck in different ways. I've wanted to fix that bigger issue for ages.

[kib]

In D41058#935310, @imp wrote:

Also: config(8) and sys/modules have always had mismatches like this... and they both suck in different ways. I've wanted to fix that bigger issue for ages.

Lets put the discrepancy between config and modules aside. I still do not understand the real requirements of the code. MODULE_DEPEND() statement, besides some bus magic, mostly about allowing depended module to bind to symbols from the dependency. Is nvme depends on CAM in this sense?

[imp]

In D41058#935413, @kib wrote:

In D41058#935310, @imp wrote:

Also: config(8) and sys/modules have always had mismatches like this... and they both suck in different ways. I've wanted to fix that bigger issue for ages.

Lets put the discrepancy between config and modules aside. I still do not understand the real requirements of the code. MODULE_DEPEND() statement, besides some bus magic, mostly about allowing depended module to bind to symbols from the dependency. Is nvme depends on CAM in this sense?

The NVMe driver proper does not depend on CAM. It was written so that any block provider could be layered on top of the core driver. nvd and nda are two such providers. The former has no dependencies on CAM. The latter provides a cam SIM front-end to the nvme core as well as the nda block driver. Many users use nvme w/o nda or cam at all because of this arrangement.

nvme_qpair.c grew unfortunate knowledge of ccb's after I did the initial nvme sim work. Although I committed it, chuck tuffli did the the work to add CAM_DATA_SG support to the pass driver for nvme, where we have to load a dmamap for that operation (so normal I/O doesn't use it, but this new, special I/O through pass does use it). This was fine while the bus_dmamap_load_ccb was inside of subr_busdma, but when it moved to cam, it was no longer fine, but the liability we're talking about now.

As for the MODULE_DEPEND on cam, that's because we made the simplifying assumption that our users would rather not have to manually load a separate cam sim to use nda with nvme (at least that's my memory, I could be mistaken since it's not a confident memory). So nvme.ko currently bundles together the core nvme driver along with the nvme_sim code to allow nda to attach to the drive. I added it to sort out the mess with loading nvme.ko and nvd.ko and the dependency here. The commit message doesn't explain why I thought it was needed, however. It's orthogonal to the issue at hand if an adjustment is needed.

[jhb]

In D41058#935594, @imp wrote:

In D41058#935413, @kib wrote:

In D41058#935310, @imp wrote:

Also: config(8) and sys/modules have always had mismatches like this... and they both suck in different ways. I've wanted to fix that bigger issue for ages.

Lets put the discrepancy between config and modules aside. I still do not understand the real requirements of the code. MODULE_DEPEND() statement, besides some bus magic, mostly about allowing depended module to bind to symbols from the dependency. Is nvme depends on CAM in this sense?

The NVMe driver proper does not depend on CAM. It was written so that any block provider could be layered on top of the core driver. nvd and nda are two such providers. The former has no dependencies on CAM. The latter provides a cam SIM front-end to the nvme core as well as the nda block driver. Many users use nvme w/o nda or cam at all because of this arrangement.

nvme_qpair.c grew unfortunate knowledge of ccb's after I did the initial nvme sim work. Although I committed it, chuck tuffli did the the work to add CAM_DATA_SG support to the pass driver for nvme, where we have to load a dmamap for that operation (so normal I/O doesn't use it, but this new, special I/O through pass does use it). This was fine while the bus_dmamap_load_ccb was inside of subr_busdma, but when it moved to cam, it was no longer fine, but the liability we're talking about now.

As for the MODULE_DEPEND on cam, that's because we made the simplifying assumption that our users would rather not have to manually load a separate cam sim to use nda with nvme (at least that's my memory, I could be mistaken since it's not a confident memory). So nvme.ko currently bundles together the core nvme driver along with the nvme_sim code to allow nda to attach to the drive. I added it to sort out the mess with loading nvme.ko and nvd.ko and the dependency here. The commit message doesn't explain why I thought it was needed, however. It's orthogonal to the issue at hand if an adjustment is needed.

You know, actually, the other option you have here (that I didn't think of after sending the mails I just sent) is you can make use of memdesc in nvme(4) the way I am now for the fabrics host. The generic fabrics transport layer I've designed uses 'memdesc' to describe buffers and in nvmf_sim.c it creates a memdesc from a CCB, and in nvmf_ns.c (for /dev/nvmeXnsY), it creates a memdesc from a bio. Then by the time it gets to the transport layer (TCP), it just has a command capsule with an attached memdesc. You could the same in nvme without requiring the function pointer, etc. if you made nvme_qpair.c use bus_dmamap_load_mem and made the different frontend's generate a memdesc from either the bio or CCB and pass the memdesc down in the nvme_command instead. This also allows you to handle nvme_command requests that use flat buffers, or you can use vm_fault_quick_hold_pages instead of the vmapbuf hack for passthrough commands. I suspect you would find this to be cleaner all around.

[chuck]

In D41058#935594, @imp wrote:

...
nvme_qpair.c grew unfortunate knowledge of ccb's after I did the initial nvme sim work. Although I committed it, chuck tuffli did the the work to add CAM_DATA_SG support to the pass driver for nvme, where we have to load a dmamap for that operation (so normal I/O doesn't use it, but this new, special I/O through pass does use it). This was fine while the bus_dmamap_load_ccb was inside of subr_busdma, but when it moved to cam, it was no longer fine, but the liability we're talking about now.

My only need is to pass-through NVMe commands to a CAM device from an application. If the current dmamap isn't a good option now, I'm happy to rework it.