Re: [PATCH V3 6/6] crypto/nx: Add P9 NX support for 842 compression engine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 28, 2017 at 7:25 PM, Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
> Hi Haren,
>
> Some comments inline ...
>
> Haren Myneni <haren@xxxxxxxxxxxxxxxxxx> writes:
>
>> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
>> index c0dd4c7e17d3..13089a0b9dfa 100644
>> --- a/drivers/crypto/nx/nx-842-powernv.c
>> +++ b/drivers/crypto/nx/nx-842-powernv.c
>> @@ -32,6 +33,9 @@ MODULE_ALIAS_CRYPTO("842-nx");
>>
>>  #define WORKMEM_ALIGN        (CRB_ALIGN)
>>  #define CSB_WAIT_MAX (5000) /* ms */
>> +#define VAS_RETRIES  (10)
>
> Where does that number come from?
>
> Do we have any idea what the trade off is between retrying vs just
> falling back to doing the request in software?
>
>> +/* # of requests allowed per RxFIFO at a time. 0 for unlimited */
>> +#define MAX_CREDITS_PER_RXFIFO       (1024)
>>
>>  struct nx842_workmem {
>>       /* Below fields must be properly aligned */
>> @@ -42,16 +46,27 @@ struct nx842_workmem {
>>
>>       ktime_t start;
>>
>> +     struct vas_window *txwin;       /* Used with VAS function */
>
> I don't understand how it makes sense to put txwin and start between the
> fields above, and the padding.

workmem is a scratch buffer and shouldn't be used for something
persistent like this.

>
> If the workmem pointer you receive is not aligned, then PTR_ALIGN() will
> advance it and mean you end up writing over start and txwin.
>
> That's probably not your bug, the code is already like that.

no, it's a bug in this patch, because workmem is scratch whose
contents are only valid for the duration of each operation (compress
or decompress).

>
>>       char padding[WORKMEM_ALIGN]; /* unused, to allow alignment */
>>  } __packed __aligned(WORKMEM_ALIGN);
>
>> @@ -576,6 +690,198 @@ static inline void nx842_add_coprocs_list(struct nx842_coproc *coproc,
>>       list_add(&coproc->list, &nx842_coprocs);
>>  }
>>
>> +/*
>> + * Identify chip ID for each CPU and save coprocesor adddress for the
>> + * corresponding NX engine in percpu coproc_inst.
>> + * coproc_inst is used in crypto_init to open send window on the NX instance
>> + * for the corresponding CPU / chip where the open request is executed.
>> + */
>> +static void nx842_set_per_cpu_coproc(struct nx842_coproc *coproc)
>> +{
>> +     unsigned int i, chip_id;
>> +
>> +     for_each_possible_cpu(i) {
>> +             chip_id = cpu_to_chip_id(i);
>> +
>> +             if (coproc->chip_id == chip_id)
>> +                     per_cpu(coproc_inst, i) = coproc;
>> +     }
>> +}
>> +
>> +
>> +static struct vas_window *nx842_alloc_txwin(struct nx842_coproc *coproc)
>> +{
>> +     struct vas_window *txwin = NULL;
>> +     struct vas_tx_win_attr txattr;
>> +
>> +     /*
>> +      * Kernel requests will be high priority. So open send
>> +      * windows only for high priority RxFIFO entries.
>> +      */
>> +     vas_init_tx_win_attr(&txattr, coproc->ct);
>> +     txattr.lpid = 0;        /* lpid is 0 for kernel requests */
>> +     txattr.pid = mfspr(SPRN_PID);
>
> Should we be using SPRN_PID here? That makes it appear as if it comes
> from the current user process, which seems fishy.
>
>> +     /*
>> +      * Open a VAS send window which is used to send request to NX.
>> +      */
>> +     txwin = vas_tx_win_open(coproc->vas.id, coproc->ct, &txattr);
>> +     if (IS_ERR(txwin)) {
>> +             pr_err("ibm,nx-842: Can not open TX window: %ld\n",
>> +                             PTR_ERR(txwin));
>> +             return NULL;
>> +     }
>> +
>> +     return txwin;
>> +}
>> +
>> +static int __init vas_cfg_coproc_info(struct device_node *dn, int chip_id,
>> +                                     int vasid)
>> +{
>> +     struct vas_window *rxwin = NULL;
>> +     struct vas_rx_win_attr rxattr;
>> +     struct nx842_coproc *coproc;
>> +     u32 lpid, pid, tid, fifo_size;
>> +     u64 rx_fifo;
>> +     const char *priority;
>> +     int ret;
>> +
>> +     ret = of_property_read_u64(dn, "rx-fifo-address", (void *)&rx_fifo);
>                                                           ^^^^^^^^
>                                                           Unnecessary cast.
>
>> +     if (ret) {
>> +             pr_err("Missing rx-fifo-address property\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32(dn, "rx-fifo-size", &fifo_size);
>> +     if (ret) {
>> +             pr_err("Missing rx-fifo-size property\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32(dn, "lpid", &lpid);
>> +     if (ret) {
>> +             pr_err("Missing lpid property\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32(dn, "pid", &pid);
>> +     if (ret) {
>> +             pr_err("Missing pid property\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32(dn, "tid", &tid);
>> +     if (ret) {
>> +             pr_err("Missing tid property\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_string(dn, "priority", &priority);
>> +     if (ret) {
>> +             pr_err("Missing priority property\n");
>> +             return ret;
>> +     }
>> +
>> +     coproc = kzalloc(sizeof(*coproc), GFP_KERNEL);
>> +     if (!coproc)
>> +             return -ENOMEM;
>> +
>> +     if (!strcmp(priority, "High"))
>> +             coproc->ct = VAS_COP_TYPE_842_HIPRI;
>> +     else if (!strcmp(priority, "Normal"))
>> +             coproc->ct = VAS_COP_TYPE_842;
>> +     else {
>> +             pr_err("Invalid RxFIFO priority value\n");
>> +             ret =  -EINVAL;
>> +             goto err_out;
>> +     }
>> +
>> +     vas_init_rx_win_attr(&rxattr, coproc->ct);
>> +     rxattr.rx_fifo = (void *)rx_fifo;
>> +     rxattr.rx_fifo_size = fifo_size;
>> +     rxattr.lnotify_lpid = lpid;
>> +     rxattr.lnotify_pid = pid;
>> +     rxattr.lnotify_tid = tid;
>> +     rxattr.wcreds_max = MAX_CREDITS_PER_RXFIFO;
>> +
>> +     /*
>> +      * Open a VAS receice window which is used to configure RxFIFO
>> +      * for NX.
>> +      */
>> +     rxwin = vas_rx_win_open(vasid, coproc->ct, &rxattr);
>> +     if (IS_ERR(rxwin)) {
>> +             ret = PTR_ERR(rxwin);
>> +             pr_err("setting RxFIFO with VAS failed: %d\n",
>> +                     ret);
>> +             goto err_out;
>> +     }
>> +
>> +     coproc->vas.rxwin = rxwin;
>> +     coproc->vas.id = vasid;
>> +     nx842_add_coprocs_list(coproc, chip_id);
>> +
>> +     /*
>> +      * Kernel requests use only high priority FIFOs. So save coproc
>> +      * info in percpu coproc_inst which will be used to open send
>> +      * windows for crypto open requests later.
>> +      */
>> +     if (coproc->ct == VAS_COP_TYPE_842_HIPRI)
>> +             nx842_set_per_cpu_coproc(coproc);
>> +
>> +     return 0;
>> +
>> +err_out:
>> +     kfree(coproc);
>> +     return ret;
>> +}
>> +
>> +
>> +static int __init nx842_powernv_probe_vas(struct device_node *pn)
>> +{
>> +     struct device_node *dn;
>> +     int chip_id, vasid, ret = 0;
>> +     int nx_fifo_found = 0;
>> +
>> +     chip_id = of_get_ibm_chip_id(pn);
>> +     if (chip_id < 0) {
>> +             pr_err("ibm,chip-id missing\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     dn = of_find_compatible_node(pn, NULL, "ibm,power9-vas-x");
>
> That's the wrong compatible, that will find the raw node, not the one
> that's intended for Linux use.
>
> It's also not really the NX drivers business to be looking for VAS
> nodes. You should just look for the P9 NX nodes, and if VAS wasn't
> configured for some reason then the VAS window open will fail and you
> detect it then.
>
>> +     if (!dn) {
>> +             pr_err("Missing VAS device node\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (of_property_read_u32(dn, "ibm,vas-id", &vasid)) {
>> +             pr_err("Missing ibm,vas-id device property\n");
>> +             of_node_put(dn);
>> +             return -EINVAL;
>> +     }
>> +
>> +     of_node_put(dn);
>
> So basically just drop all of that above.
>
>> diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
>> index d94e25df503b..da3cb8c35ec7 100644
>> --- a/drivers/crypto/nx/nx-842.c
>> +++ b/drivers/crypto/nx/nx-842.c
>> @@ -116,7 +116,7 @@ int nx842_crypto_init(struct crypto_tfm *tfm, struct nx842_driver *driver)
>>
>>       spin_lock_init(&ctx->lock);
>>       ctx->driver = driver;
>> -     ctx->wmem = kmalloc(driver->workmem_size, GFP_KERNEL);
>> +     ctx->wmem = kzalloc(driver->workmem_size, GFP_KERNEL);

don't put persistent fields in the workmem, and you don't need to zero it.

>>       ctx->sbounce = (u8 *)__get_free_pages(GFP_KERNEL, BOUNCE_BUFFER_ORDER);
>>       ctx->dbounce = (u8 *)__get_free_pages(GFP_KERNEL, BOUNCE_BUFFER_ORDER);
>>       if (!ctx->wmem || !ctx->sbounce || !ctx->dbounce) {
>
> That looks OK but should be split into a separate patch because it
> affects the Power8 code as well as the pseries code.
>
> cheers



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux