Overview:

[hareshx.sankar.raj_intel.com]

Summary

Intel(R) QuickAssist Technology (Intel(R) QAT) provides hardware
acceleration for offloading security, authentication and compression
services from the CPU, thus significantly increasing the performance
and efficiency of standard platform solutions.

This review introduces:

1. Fixing typos and formatting issues
2. Addition of disable safe dc mode for QAT SPR devices
3. Update 4xxx capabilities handling
4. Moving debugfs handling to separate file
5. Restrict sysctl access to privileged users
6. Reimplement cpaCyGetXXXInstances as a wrapper
7. Driver updates to improve code and fix bugs
8. Refactor error handling and addition of mutex locks
9. Update API files to use SPDX identifier

Some background on our QAT FreeBSD code base. This review includes typo/grammar/format changes (in addition to features/bug fixes) made by our Linux/Windows team. They release this code and also go through community reviews. Our QAT FreeBSD team shares this same code base and at a later time, release this code to FreeBSD. While there is some FreeBSD specific code, we try to keep this to a minimum (based on OS differences). As we lag the Linux/Windows releases, our release strategy is to try and influence changes made by those teams before we inherit the code. Otherwise our QAT FreeBSD code would diverge more and more over time and be very difficult for us to maintain. That said, we do immediately fix critical/security issues found.

[imp]

sys/modules/qat/qat/Makefile

27

Please use ">>" in case we ever add more of these.

[hareshx.sankar.raj_intel.com]

share/man/man4/qat.4

114

Thanks for your feedback. We will update this in the review.
Locally checked the change and it is reflecting as desired.

sys/dev/qat/qat_api/common/crypto/sym/include/lac_session.h

40 ↗

(On Diff #155568)

Thanks for the suggestions on grammar and typos.
The typos fixes we made currently in this review are done to align with the Linux codebase.

The files are shared with Linux/Windows platforms as well and in case we correct here, we will have some additional reworks in the internal repos as well to align the files across platforms. We will plan to consider the suggestions made in a future review.

sys/dev/qat/qat_api/common/crypto/sym/key/lac_sym_key.c

2165 ↗

(On Diff #155568)

Thanks - we will update this in this review and respond here

sys/dev/qat/qat_api/common/include/lac_mem.h

354 ↗

(On Diff #155568)

Our internal verify job has a spellcheck and formatting tool that had identified this and it has been fixed when fixing multiple other typos

sys/dev/qat/qat_common/adf_cfg_device.c

686 ↗

(On Diff #155568)

Thanks - We will update this review correcting it

sys/dev/qat/qat_common/adf_freebsd_dbgfs.c

26 ↗

(On Diff #155568)

Thanks - this EXPORT_SYMBOL_GPL() is not required. We will update this review removing it.
The debugfs is a qat specific sysctl entry which will display few config parameters when requested.

sys/dev/qat/qat_hw/qat_4xxx/adf_drv.c

86

We wanted to invoke a custom handler adf_4xxx_sysctl_disable_safe_dc_mode when the sysctl attribute is set and hence SYSCTL_ADD_OID was preferred.

sys/dev/qat/qat_hw/qat_4xxxvf/adf_4xxxvf_hw_data.c

136 ↗

(On Diff #155568)

Yes the capabilities handling logic has been updated and refactored in the code.
We are aligning the logic with the Linux drivers.

sys/dev/qat/qat_hw/qat_4xxxvf/adf_drv.c

223 ↗

(On Diff #155568)

Thanks Mark - Agree to this.
We will fix in this review.

sys/dev/qat/qat_hw/qat_c4xxx/adf_c4xxx_ras.c

64 ↗

(On Diff #155568)

Thanks for highlighting - There needs a cleanup regarding this function which we will do in a future release. Our current scope did not cover the RAS part. The privilege check was added uniformly for all sysctl functions

sys/modules/qat/qat/Makefile

27

We currently do not have a plan to add more options.
The options and file are qat specific.
We need this file to be generated freshly. In case we want to add new option we will ensure we append to the file.

opt_qat.h:
:> ${.TARGET}
.if defined(QAT_DISABLE_SAFE_DC_MODE) && ${QAT_DISABLE_SAFE_DC_MODE} == "1"
@echo "#define QAT_DISABLE_SAFE_DC_MODE 1" > ${.TARGET}
.endif
.if defined(QAT_ENABLE_NEW_FEATURE) && ${QAT_ENABLE_NEW_FEATURE} == "1"
@echo "#define QAT_ENABLE_NEW_FEATURE 1" >> ${.TARGET}
.endif
.endif

[hareshx.sankar.raj_intel.com]

sys/dev/qat/qat_api/common/compression/dc_datapath.c

121 ↗

(On Diff #155568)

Thanks.
Yes - we identified this as part of coverity scan and fixed it.

sys/dev/qat/qat_api/common/crypto/sym/include/lac_session.h

49 ↗

(On Diff #155568)

Thanks for the suggestion. We will plan this for a future release.

61 ↗

(On Diff #155568)

Thanks for the suggestion. We will plan this for a future release.

sys/dev/qat/qat_api/common/crypto/sym/include/lac_sym.h

12 ↗

(On Diff #155568)

Thanks for the feedback. We understand there are inconsistencies here and in some other portions as well.
We will check and plan this for a future release.

18 ↗

(On Diff #155568)

Agree - we will plan to fix this and more typos in a future release.

43 ↗

(On Diff #155568)

Totally agree! We will check the inconsistencies and plan in a future release.

sys/dev/qat/qat_api/common/crypto/sym/include/lac_sym_alg_chain.h

2 ↗

(On Diff #155568)

This was missed in an earlier release and has been corrected now.

sys/dev/qat/qat_api/common/crypto/sym/include/lac_sym_key.h

17 ↗

(On Diff #155568)

Thanks for the suggestion.
As mentioned - the files are shared with other platforms as well. And we will plan internally and correct and update in a future release

sys/dev/qat/qat_api/common/crypto/sym/key/lac_sym_key.c

2165 ↗

(On Diff #155568)

We discussed this internally and it looks it is the same code in Linux as well.
We also note there can be more places which do this style. Hence, we will check for these occurrences and plan for a future release.

sys/dev/qat/qat_api/common/ctrl/sal_compression.c

1439 ↗

(On Diff #155568)

This was identified as part of our coverity analysis on the code. There can be instances where the code flow can have device as non-NULL - and device structure not properly filled with device name. This could lead to subsequent issues - and hence a device name check here was essential

sys/dev/qat/qat_api/common/ctrl/sal_get_instances.c

342 ↗

(On Diff #155568)

The cpaGetNumInstances API is the one that is exposed outside and recommended for usage with the applications to get the number of instances in general by requesting with the service type. The Lac_GetCyNumInstancesByType() function is only internal wrapper function for the crypto instances.

We believe this should not cause issues.

sys/dev/qat/qat_api/common/include/lac_mem.h

484 ↗

(On Diff #155568)

Thanks. Totally agree. The formatting needs correction. We will correct this in a future release.

sys/dev/qat/qat_api/include/cpa.h

1 ↗

(On Diff #155568)

Sure - we will add the SPDX identifier and remove the longer form of license and ensure consistency across files. We will update this review.

[edward.f.roache_intel.com]

sys/dev/qat/qat_api/common/crypto/sym/include/lac_session.h

40 ↗

(On Diff #155568)

Thanks Eric and all for the good feedback on these typos/formatting items. This review is just a first step in us improving these items (as we rebase internally to a later version of the code shared with other teams). Some of the suggestions may already be fixed internally and we will be picking up in future QAT FreeBSD releases. Others that are not, we will be tracking internally and try to influence change in the shared internal code. Bottom line is that I expect this is just the first of several iterations we will be making to improve the quality. Just some background to try and explain why we are not immediately making changes for some of the suggestions.

[hareshx.sankar.raj_intel.com]

• qat: add disable safe dc mode for QAT SPR devices
• qat: update 4xxx capabilities handling
• qat: move debugfs handling to separate file
• qat: restrict sysctl access to privileged users
• qat: reimplement cpaCyGetXXXInstances as a wrapper
• qat: driver updates to improve code and fix bugs
• qat: refactor error handling and add mutex locks
• qat: update API files to use SPDX identifier

[hareshx.sankar.raj_intel.com]

Thanks everyone for your feedback.
We have responded and updated the code addressing the review comments.
Request to kindly re-review and share your feedback and approval. Thanks.

share/man/man4/qat.4

114

Changes are updated as suggested in the latest update.

sys/dev/qat/qat_api/include/cpa.h

1 ↗

(On Diff #155568)

The latest version is updated to add SPDX identifier and removing longer form license thereby maintaining consistency

sys/dev/qat/qat_api/include/icp_buffer_desc.h

22 ↗

(On Diff #155568)

Agree - We will feedback this and check in a future release.

sys/dev/qat/qat_api/qat_kernel/src/lac_adf_interface_freebsd.c

291 ↗

(On Diff #155568)

We believe this was corrected to match the functionality - "and atleast one" of the capabilities was required.

sys/dev/qat/qat_common/adf_cfg_device.c

686 ↗

(On Diff #155568)

It has been corrected in the latest update.

sys/dev/qat/qat_common/adf_freebsd_dbgfs.c

26 ↗

(On Diff #155568)

EXPORT_SYMBOL_GPL is not required and hence removed in the latest version

sys/dev/qat/qat_hw/qat_4xxxvf/adf_drv.c

223 ↗

(On Diff #155568)

bus master disable is called in the detach flow as well in the latest revision.

sys/dev/qat/qat_hw/qat_c3xxx/adf_drv.c

207 ↗

(On Diff #155568)

We have updated the code now. The bus master disable is called in the failure scenarios of driver attach.

sys/modules/qat/qat/Makefile

27

We reviewed it again and saw it is okay to append instead of overwrite.
The code has been updated to use ">>" append

sys/modules/qat/qat_api/Makefile

84

Thanks. The code has been updated to use ">>" append

sys/modules/qat/qat_common/Makefile

39

Thanks. The code has been updated to use ">>" append

sys/modules/qat/qat_hw/Makefile

35

Thanks. The code has been updated to use ">>" append

[markj]

Please mail me git-formatted patches and I'll apply them.

[ziaee]

I'm so sorry sir, due to the 14.3 release I revised the manual as part of an overhaul of all crypto device driver manuals, please rebase the manual page. Or I can do it if you want. Please forgive the disruption.

[hareshx.sankar.raj_intel.com]

In D50379#1155539, @ziaee wrote:

I'm so sorry sir, due to the 14.3 release I revised the manual as part of an overhaul of all crypto device driver manuals, please rebase the manual page. Or I can do it if you want. Please forgive the disruption.

Thanks - we will rebase to latest main code to include the qat man page update and have this phabricator review updated as well.
We would submit the git-formatted patches after rebase.

[hareshx.sankar.raj_intel.com]

Rebased and brought in qat.4 man page updates.

[hareshx.sankar.raj_intel.com]

In D50379#1157030, @hareshx.sankar.raj_intel.com wrote:

Rebased and brought in qat.4 man page updates.

Hi All - We have rebased and updated the phabricator review to bring in the qat.4 man page updates by the community.
Request your review and approval - so that we can prepare the final git-formatted patches. Thanks.

[ziaee]

LGTM from manpages with the following syntax nits which produce compiler complaints. Thank you so much for updating this!

share/man/man4/qat.4

3

Mandoc does not allow leading zeros in dates.

118

The roff(7) language does not allow blank lines, this produces a warning in the linter. You can check for roff errors using the manual compiler's linter: mandoc -Tlint src/share/man/man4/qat.4.

[hareshx.sankar.raj_intel.com]

In D50379#1157095, @ziaee wrote:

LGTM from manpages with the following syntax nits which produce compiler complaints. Thank you so much for updating this!

Thanks for your feedback - We will ensure to follow the mandoc checks in our further submissions.
We understand this review has been accepted by Mark from reviewers list and also by Manpages group.
Can we proceed to submit the commits as git-formatted patches over mail? and take up the man page formatting nits in a future review?

[markj]

In D50379#1157103, @hareshx.sankar.raj_intel.com wrote:

In D50379#1157095, @ziaee wrote:

LGTM from manpages with the following syntax nits which produce compiler complaints. Thank you so much for updating this!

Thanks for your feedback - We will ensure to follow the mandoc checks in our further submissions.
We understand this review has been accepted by Mark from reviewers list and also by Manpages group.
Can we proceed to submit the commits as git-formatted patches over mail? and take up the man page formatting nits in a future review?

Yes, that's fine, and I can correct the nits before pushing.

[hareshx.sankar.raj_intel.com]

Address minor nit comments on manpage
Add back accidentally removed copyright commit

[hareshx.sankar.raj_intel.com]

We noticed that we accidentally missed the copyright year commit changes recently during our latest push. Hence the review is updated ensuring the latest changes are brought in.
Apologies for the inconvenience.
On the latest push - we have addressed the nit comments on manpage as well.
We will plan to do a final patch review and validation and submit git formatted patches tomorrow. Thanks.

[hareshx.sankar.raj_intel.com]

share/man/man4/qat.4

3

Thanks, We noticed this by running mandoc command run as suggested.

mandoc -Tlint share/man/man4/qat.4
mandoc: share/man/man4/qat.4:118:1: WARNING: blank line in fill mode, using .sp
mandoc: share/man/man4/qat.4:3:5: STYLE: normalizing date format to: Dd June 2, 2025

We have fixed it in the latest revision.

118

Blank line is removed. Thanks for the knowledge regarding mandoc.

We will ensure this to be run as part of any of our future qat man page updates as well.