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: <20240123162547.GA254773@kernel.org>
Date: Tue, 23 Jan 2024 16:25:47 +0000
From: Simon Horman <horms@...nel.org>
To: Alan Brady <alan.brady@...el.com>
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 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.

> +
> +	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);
> +}