netdev - Re: [PATCH 1/6 iwl-next] idpf: implement virtchnl transaction manager

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <961b597c-a6df-4544-b7da-bbb9f52ea657@intel.com>
Date: Tue, 23 Jan 2024 08:52:12 -0800
From: "Brady, Alan" <alan.brady@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, "Igor
 Bagnucki" <igor.bagnucki@...el.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, Joshua Hay <joshua.a.hay@...el.com>
Subject: Re: [PATCH 1/6 iwl-next] idpf: implement virtchnl transaction manager

On 1/23/2024 8:25 AM, Simon Horman wrote:
> On Mon, Jan 22, 2024 at 01:11:20PM -0800, Alan Brady wrote:
>> This starts refactoring how virtchnl messages are handled by adding a
>> transaction manager (idpf_vc_xn_manager).
>>
>> There are two primary motivations here which are to enable handling of
>> multiple messages at once and to make it more robust in general. As it
>> is right now, the driver may only have one pending message at a time and
>> there's no guarantee that the response we receive was actually intended
>> for the message we sent prior.
>>
>> This works by utilizing a "cookie" field of the message descriptor. It
>> is arbitrary what data we put in the cookie and the response is required
>> to have the same cookie the original message was sent with. Then using a
>> "transaction" abstraction that uses the completion API to pair responses
>> to the message it belongs to.
>>
>> The cookie works such that the first half is the index to the
>> transaction in our array, and the second half is a "salt" that gets
>> incremented every message. This enables quick lookups into the array and
>> also ensuring we have the correct message. The salt is necessary because
>> after, for example, a message times out and we deem the response was
>> lost for some reason, we could theoretically reuse the same index but
>> using a different salt ensures that when we do actually get a response
>> it's not the old message that timed out previously finally coming in.
>> Since the number of transactions allocated is U8_MAX and the salt is 8
>> bits, we can never have a conflict because we can't roll over the salt
>> without using more transactions than we have available.
>>
>> This starts by only converting the VIRTCHNL2_OP_VERSION message to use
>> this new transaction API. Follow up patches will convert all virtchnl
>> messages to use the API.
>>
>> Reviewed-by: Igor Bagnucki <igor.bagnucki@...el.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> Signed-off-by: Alan Brady <alan.brady@...el.com>
>> Co-developed-by: Joshua Hay <joshua.a.hay@...el.com>
>> Signed-off-by: Joshua Hay <joshua.a.hay@...el.com>
> ...
>
>> +/**
>> + * idpf_vc_xn_init - Initialize virtchnl transaction object
>> + * @vcxn_mngr: pointer to vc transaction manager struct
>> + */
>> +static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr)
>> +{
>> +	int i;
>> +
>> +	spin_lock_init(&vcxn_mngr->xn_bm_lock);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(vcxn_mngr->ring); i++) {
>> +		struct idpf_vc_xn *xn = &vcxn_mngr->ring[i];
>> +
>> +		xn->state = IDPF_VC_XN_IDLE;
>> +		xn->idx = i;
>> +		idpf_vc_xn_release_bufs(xn);
>> +		init_completion(&xn->completed);
>> +	}
> Hi Alan and Joshua,
>
> I'm slightly surprised to see that
> it is safe to initialise xn_bm_lock above,
> but it needs to be taken below.


Thanks for the review. I agree it's misguided overkill, will remove in 
next revision.

-Alan


>> +
>> +	spin_lock_bh(&vcxn_mngr->xn_bm_lock);
>> +	bitmap_set(vcxn_mngr->free_xn_bm, 0, IDPF_VC_XN_RING_LEN);
>> +	spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
>> +}