czw., 2 kwi 2020 o 14:36 Ferruh Yigit <ferruh.yi...@intel.com> napisał(a):
>
> On 4/1/2020 3:21 PM, Michal Krawczyk wrote:
> > The RSS hash key was present in the device, but it wasn't exposed to
> > the user. The other key still cannot be set, but now it can be accessed
> > if one needs to do that.
>
> What is 'the other key'?
>
I meant 'the custom key provided by the user'. I'll precise this in v3.
> >
> > By default, the random hash key is used and it is generated only once
> > when requested for the first time.
>
> It looks like this patch does
> 1- setting default rss hash key (generated randomly)
> 2- Separate 'ena_com_get_hash_function()' and 'ena_com_get_hash_key()'
> - I didn't get the motivation of this seperation, new function not called
> 3- Minor fixes, like 'key' check in 'ena_com_fill_hash_function()', in
> 'ENA_ADMIN_TOEPLITZ' case, ...
> 4- remove 'ena_com_ind_tbl_convert_from_device()'
>
> What to you think seperating above ones if they are logically seperate?
>
Sure, I'll split this into 3 commits - I'll skip the 2nd one as
initially we were planning to add full RSS configuration support in
this release, but postponed it as for now (it's a really big patch).
So this new function (ena_com_get_hash_key()) will be used there, but
maybe as it's not yet in, we can skip this.
> >
> > Signed-off-by: Michal Krawczyk <m...@semihalf.com>
> > Reviewed-by: Igor Chauskin <igo...@amazon.com>
> > Reviewed-by: Guy Tzalik <gtza...@amazon.com>
>
> <...>
>
> > @@ -1032,6 +1032,24 @@ static int ena_com_get_feature(struct ena_com_dev
> > *ena_dev,
> > feature_ver);
> > }
> >
> > +int ena_com_get_current_hash_function(struct ena_com_dev *ena_dev)
> > +{
> > + return ena_dev->rss.hash_func;
> > +}
>
>
> This function is not called, who is the intendent consumer of the function?
> Commit log mentions exposing to user, is the intention this function to be
> called by application, if so it shoudln't, applications doesn't call driver
> fucntions directly.
>
The intended customer was the PMD, which could further expose it to
the application via DPDK API - but we're missing this feature in this
release, so maybe it'll be better to add it later.
> <...>
>
> > @@ -1266,30 +1284,6 @@ static int ena_com_ind_tbl_convert_to_device(struct
> > ena_com_dev *ena_dev)
> > return 0;
> > }
> >
> > -static int ena_com_ind_tbl_convert_from_device(struct ena_com_dev *ena_dev)
> > -{
> > - u16 dev_idx_to_host_tbl[ENA_TOTAL_NUM_QUEUES] = { (u16)-1 };
> > - struct ena_rss *rss = &ena_dev->rss;
> > - u8 idx;
> > - u16 i;
> > -
> > - for (i = 0; i < ENA_TOTAL_NUM_QUEUES; i++)
> > - dev_idx_to_host_tbl[ena_dev->io_sq_queues[i].idx] = i;
> > -
> > - for (i = 0; i < 1 << rss->tbl_log_size; i++) {
> > - if (rss->rss_ind_tbl[i].cq_idx > ENA_TOTAL_NUM_QUEUES)
> > - return ENA_COM_INVAL;
> > - idx = (u8)rss->rss_ind_tbl[i].cq_idx;
> > -
> > - if (dev_idx_to_host_tbl[idx] > ENA_TOTAL_NUM_QUEUES)
> > - return ENA_COM_INVAL;
> > -
> > - rss->host_rss_ind_tbl[i] = dev_idx_to_host_tbl[idx];
> > - }
> > -
> > - return 0;
> > -}> -
>
> This function and where it is called seems removed, is this related to this
> patch, "setting default hash key"?
>
Yeah - it looks like it should be a part of the separate patch.
> <...>
>
> > case ENA_ADMIN_TOEPLITZ:
> > - if (key_len > sizeof(hash_key->key)) {
> > - ena_trc_err("key len (%hu) is bigger than the max
> > supported (%zu)\n",
> > - key_len, sizeof(hash_key->key));
> > - return ENA_COM_INVAL;
> > + if (key) {
> > + if (key_len != sizeof(hash_key->key)) {
> > + ena_trc_err("key len (%hu) doesn't equal the
> > supported size (%zu)\n",
> > + key_len, sizeof(hash_key->key));
> > + return ENA_COM_INVAL;
> > + }
> > + memcpy(hash_key->key, key, key_len);
> > + rss->hash_init_val = init_val;
> > + hash_key->keys_num = key_len >> 2;
>
> I am aware this is copy/paste, but here '>> 2' is "/ sizeof(unit2_t)", would
> it
> be better to use that way instead of hardcoded value?
>
Will change that in v3.
> <...>
>
> > @@ -256,6 +256,23 @@ static const struct eth_dev_ops ena_dev_ops = {
> > .reta_query = ena_rss_reta_query,
> > };
> >
> > +void ena_rss_key_fill(void *key, size_t size)
> > +{
> > + static bool key_generated;
> > + static uint8_t default_key[ENA_HASH_KEY_SIZE];
>
> If there are multiple 'ena' devices in the platform, using 'static' variables
> will cause each device use same rss key, I just want to double check this is
> the
> intention.
Yes, it's acceptable behavior. The most important thing is to have
those keys random per instance, to make the DDoS attacks harder.
