by Tian, Kevin

[permalink] [raw]

Subject: RE: [RFC 2/5] crypto: qat - add interface for live migration

> From: Xin Zeng <[email protected]>
> Sent: Friday, June 30, 2023 9:13 PM
> +
> +int qat_vfmig_suspend_device(struct pci_dev *pdev, u32 vf_nr)
> +{
> + struct adf_accel_dev *accel_dev =
> adf_devmgr_pci_to_accel_dev(pdev);
> +
> + if (!accel_dev) {
> + dev_err(&pdev->dev, "Failed to find accel_dev\n");
> + return -ENODEV;
> + }
> +
> + if (WARN_ON(!GET_VFMIG_OPS(accel_dev)->suspend_device))
> + return -EINVAL;

this and other warns should be done one-off at device registration point
instead of letting it triggerable by every user ioctl.

2023-08-11 09:17:54

by Wan, Siming

[permalink] [raw]

Subject: RE: [RFC 1/5] crypto: qat - add bank save/restore and RP drain

-----Original Message-----
From: Tian, Kevin <[email protected]>
Sent: Friday, August 4, 2023 3:51 PM
To: Zeng, Xin <[email protected]>; [email protected]; [email protected]
Cc: Cabiddu, Giovanni <[email protected]>; [email protected]; Wan, Siming <[email protected]>; Pankratov, Svyatoslav <[email protected]>; Zeng, Xin <[email protected]>
Subject: RE: [RFC 1/5] crypto: qat - add bank save/restore and RP drain

> From: Xin Zeng <[email protected]>
> Sent: Friday, June 30, 2023 9:13 PM
> ---
> .../intel/qat/qat_4xxx/adf_4xxx_hw_data.c | 5 +-
> .../intel/qat/qat_c3xxx/adf_c3xxx_hw_data.c | 2 +-
> .../qat/qat_c3xxxvf/adf_c3xxxvf_hw_data.c | 2 +-
> .../intel/qat/qat_c62x/adf_c62x_hw_data.c | 2 +-
> .../intel/qat/qat_c62xvf/adf_c62xvf_hw_data.c | 2 +-
> .../intel/qat/qat_common/adf_accel_devices.h | 60 ++-
> .../intel/qat/qat_common/adf_gen2_hw_data.c | 17 +-
> .../intel/qat/qat_common/adf_gen2_hw_data.h | 10 +-
> .../intel/qat/qat_common/adf_gen4_hw_data.c | 362 +++++++++++++++++-
> .../intel/qat/qat_common/adf_gen4_hw_data.h | 131 ++++++-
> .../intel/qat/qat_common/adf_transport.c | 11 +-
> .../crypto/intel/qat/qat_common/adf_vf_isr.c | 2 +-
> .../qat/qat_dh895xcc/adf_dh895xcc_hw_data.c | 2 +-
> .../qat_dh895xccvf/adf_dh895xccvf_hw_data.c | 2 +-
> 14 files changed, 584 insertions(+), 26 deletions(-)

this could be split into 3 patches.

one is moving from hw_data->csr_ops to hw_data->csr_info. apply to all qat drivers.

the 2nd is adding new csr_ops.

the last one then covers bank save/restore.
->Will split it, thanks.

> +
> +#define ADF_RP_INT_SRC_SEL_F_RISE_MASK BIT(2) #define
> +ADF_RP_INT_SRC_SEL_F_FALL_MASK GENMASK(2, 0) static int
> +gen4_bank_state_restore(void __iomem *csr, u32 bank_number,
> + struct bank_state *state, u32 num_rings,
> + int tx_rx_gap)
> +{

restore is the most tricky part. it's worth of some comments to help understand the flow, e.g. what is rx/tx layout, why there are multiple ring tails writes, etc.
->Will add some comments to explain.

> + u32 val, tmp_val, i;
> +
> + write_csr_ring_srv_arb_en(csr, bank_number, 0);
> +
> + for (i = 0; i < num_rings; i++)
> + write_csr_ring_base(csr, bank_number, i, state-
> >rings[i].base);
> +
> + for (i = 0; i < num_rings; i++)
> + write_csr_ring_config(csr, bank_number, i, state-
> >rings[i].config);
> +
> + for (i = 0; i < num_rings / 2; i++) {
> + int tx = i * (tx_rx_gap + 1);
> + int rx = tx + tx_rx_gap;
> + u32 tx_idx = tx / ADF_RINGS_PER_INT_SRCSEL;
> + u32 rx_idx = rx / ADF_RINGS_PER_INT_SRCSEL;
> +
> + write_csr_ring_head(csr, bank_number, tx, state-
> >rings[tx].head);
> +
> + write_csr_ring_tail(csr, bank_number, tx, state->rings[tx].tail);
> +
> + if (state->ringestat & (BIT(tx))) {
> + val = read_csr_int_srcsel(csr, bank_number, tx_idx);
> + val |= (ADF_RP_INT_SRC_SEL_F_RISE_MASK << (8 *
> tx));
> + write_csr_int_srcsel(csr, bank_number, tx_idx, val);
> + write_csr_ring_head(csr, bank_number, tx, state-
> >rings[tx].head);
> + }
> +
> + write_csr_ring_tail(csr, bank_number, rx, state-
> >rings[rx].tail);
> +
> + val = read_csr_int_srcsel(csr, bank_number, rx_idx);
> + val |= (ADF_RP_INT_SRC_SEL_F_RISE_MASK << (8 * rx));
> + write_csr_int_srcsel(csr, bank_number, rx_idx, val);
> +
> + write_csr_ring_head(csr, bank_number, rx, state-
> >rings[rx].head);
> +
> + val = read_csr_int_srcsel(csr, bank_number, rx_idx);
> + val |= (ADF_RP_INT_SRC_SEL_F_FALL_MASK << (8 * rx));
> + write_csr_int_srcsel(csr, bank_number, rx_idx, val);
> +
> + if (state->ringfstat & BIT(rx))
> + write_csr_ring_tail(csr, bank_number, rx, state-
> >rings[rx].tail);
> + }
> +
> + write_csr_int_flag_and_col(csr, bank_number, state-
> >iaintflagandcolen);
> + write_csr_int_en(csr, bank_number, state->iaintflagen);
> + write_csr_int_col_en(csr, bank_number, state->iaintcolen);
> + write_csr_int_srcsel(csr, bank_number, 0, state->iaintflagsrcsel0);
> + write_csr_exp_int_en(csr, bank_number, state->ringexpintenable);
> + write_csr_int_col_ctl(csr, bank_number, state->iaintcolctl);
> +
> + /* Check that all ring statuses are restored into a saved state. */
> + tmp_val = read_csr_stat(csr, bank_number);
> + val = state->ringstat0;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringstat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_e_stat(csr, bank_number);
> + val = state->ringestat;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringestat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_ne_stat(csr, bank_number);
> + val = state->ringnestat;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringnestat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_nf_stat(csr, bank_number);
> + val = state->ringnfstat;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringnfstat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_f_stat(csr, bank_number);
> + val = state->ringfstat;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringfstat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_c_stat(csr, bank_number);
> + val = state->ringcstat0;
> + if (tmp_val != val) {
> + pr_err("Fail to restore ringcstat register. Expected 0x%x, but
> actual is 0x%x\n",
> + tmp_val, val);
> + return -EINVAL;
> + }
> +
> + tmp_val = read_csr_exp_stat(csr, bank_number);
> + val = state->ringexpstat;
> + if (tmp_val && !val) {
> + pr_err("Bank was restored with exception: 0x%x\n", val);
> + return -EINVAL;
> + }

above checks could be wrapped in macros.
->If define macro as below, checkpatch.pl will appear warning-> WARNING: Macros with flow control statements should be avoided.
So do we still need to change?
#define CHECK_STAT(expect, actual, csr_name) \
do { \
u32 _expect = expect; \
u32 _actual = actual; \
char *_csr_name = csr_name; \
if (_expect != _actual) { \
pr_err("Fail to restore %s register. Expected 0x%x, but actual is 0x%x\n", \
_csr_name, _expect, _actual); \
return -EINVAL; \
} \
} while (0)