Re: [PATCH v3 0/4] Introduce akcipher service for virtio-crypto
On Mon, Apr 04, 2022 at 05:39:24PM +0200, Cornelia Huck wrote: > On Mon, Mar 07 2022, "Michael S. Tsirkin" wrote: > > > On Mon, Mar 07, 2022 at 10:42:30AM +0800, zhenwei pi wrote: > >> Hi, Michael & Lei > >> > >> The full patchset has been reviewed by Gonglei, thanks to Gonglei. > >> Should I modify the virtio crypto specification(use "__le32 akcipher_algo;" > >> instead of "__le32 reserve;" only, see v1->v2 change), and start a new > >> issue > >> for a revoting procedure? > > > > You can but not it probably will be deferred to 1.3. OK with you? > > > >> Also cc Cornelia Huck. > > [Apologies, I'm horribly behind on my email backlog, and on virtio > things in general :(] > > The akcipher update had been deferred for 1.2, so I think it will be 1.3 > material. However, I just noticed while browsing the fine lwn.net merge > window summary that this seems to have been merged already. That > situation is less than ideal, although I don't expect any really bad > problems, given that there had not been any negative feedback for the > spec proposal that I remember. Let's open a 1.3 branch? What do you think? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio_ring: add unlikely annotation for free descs check
On Mon, Apr 04, 2022 at 11:11:16PM +0800, Xianting Tian wrote: > I can't find it in next branch, will you apply this patch? yes, thanks! ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/4] Introduce akcipher service for virtio-crypto
On Mon, Mar 07 2022, "Michael S. Tsirkin" wrote: > On Mon, Mar 07, 2022 at 10:42:30AM +0800, zhenwei pi wrote: >> Hi, Michael & Lei >> >> The full patchset has been reviewed by Gonglei, thanks to Gonglei. >> Should I modify the virtio crypto specification(use "__le32 akcipher_algo;" >> instead of "__le32 reserve;" only, see v1->v2 change), and start a new issue >> for a revoting procedure? > > You can but not it probably will be deferred to 1.3. OK with you? > >> Also cc Cornelia Huck. [Apologies, I'm horribly behind on my email backlog, and on virtio things in general :(] The akcipher update had been deferred for 1.2, so I think it will be 1.3 material. However, I just noticed while browsing the fine lwn.net merge window summary that this seems to have been merged already. That situation is less than ideal, although I don't expect any really bad problems, given that there had not been any negative feedback for the spec proposal that I remember. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
On Mon, Apr 04, 2022 at 11:07:36AM +, Eli Cohen wrote:
> > From: Michael S. Tsirkin
> > Sent: Monday, April 4, 2022 1:35 PM
> > To: Jason Wang
> > Cc: Eli Cohen ; hdan...@sina.com;
> > virtualization@lists.linux-foundation.org; linux-ker...@vger.kernel.org
> > Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> >
> > On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> > > From: Eli Cohen
> > >
> > > The current code evaluates RQT size based on the configured number of
> > > virtqueues. This can raise an issue in the following scenario:
> > >
> > > Assume MQ was negotiated.
> > > 1. mlx5_vdpa_set_map() gets called.
> > > 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
> > >than the configured max VQs.
> > > 3. A second set_map gets called, but now a smaller number of VQs is used
> > >to evaluate the size of the RQT.
> > > 4. handle_ctrl_mq() is called with a value larger than what the RQT can
> > >hold. This will emit errors and the driver state is compromised.
> > >
> > > To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> > > required number of entries in the RQT. This value is evaluated in
> > > mlx5_vdpa_set_driver_features() where we have the negotiated features
> > > all set up.
> > >
> > > In addtion
> >
> > addition?
>
> Do you need me to send another version?
It's a bit easier that way but I can handle it manually too.
> If so, let's wait for Jason's reply.
Right.
> >
> > > to that, we take into consideration the max capability of RQT
> > > entries early when the device is added so we don't need to take consider
> > > it when creating the RQT.
> > >
> > > Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> > > max_vas / 2 and make the code clearer.
> > >
> > > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > > Signed-off-by: Eli Cohen
> >
> > Jason I don't have your ack or S.O.B on this one.
> >
> >
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++
> > > 1 file changed, 21 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 53b8c1a68f90..61bec1ed0bc9 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> > > struct mlx5_flow_handle *rx_rule_mcast;
> > > bool setup;
> > > u32 cur_num_vqs;
> > > + u32 rqt_size;
> > > struct notifier_block nb;
> > > struct vdpa_callback config_cb;
> > > struct mlx5_vdpa_wq_ent cvq_ent;
> > > @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct
> > > mlx5_vdpa_dev *mvdev, u16 val)
> > > return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> > > }
> > >
> > > -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> > > -{
> > > - return max_vqs / 2;
> > > -}
> > > -
> > > static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> > > {
> > > if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > > return 2;
> > >
> > > - return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > + return mvdev->max_vqs;
> > > }
> > >
> > > static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> > > @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net
> > > *ndev, struct mlx5_vdpa_virtqueue *
> > > static int create_rqt(struct mlx5_vdpa_net *ndev)
> > > {
> > > __be32 *list;
> > > - int max_rqt;
> > > void *rqtc;
> > > int inlen;
> > > void *in;
> > > int i, j;
> > > int err;
> > > - int num;
> > > -
> > > - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > > - num = 1;
> > > - else
> > > - num = ndev->cur_num_vqs / 2;
> > >
> > > - max_rqt = min_t(int, roundup_pow_of_two(num),
> > > - 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > > - if (max_rqt < 1)
> > > - return -EOPNOTSUPP;
> > > -
> > > - inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt *
> > > MLX5_ST_SZ_BYTES(rq_num);
> > > + inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size *
> > > MLX5_ST_SZ_BYTES(rq_num);
> > > in = kzalloc(inlen, GFP_KERNEL);
> > > if (!in)
> > > return -ENOMEM;
> > > @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> > > rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> > >
> > > MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > > - MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> > > + MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> > > list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > > - for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > > - list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> > > + for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > > + list[i] = cpu_to_be32(ndev->vqs[j %
> > > ndev->cur_num_vqs].virtq_id);
> > >
> > > - MLX5_SET(rqtc, rqtc, rqt_actual_size,
Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> From: Eli Cohen
>
> The current code evaluates RQT size based on the configured number of
> virtqueues. This can raise an issue in the following scenario:
>
> Assume MQ was negotiated.
> 1. mlx5_vdpa_set_map() gets called.
> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
>than the configured max VQs.
> 3. A second set_map gets called, but now a smaller number of VQs is used
>to evaluate the size of the RQT.
> 4. handle_ctrl_mq() is called with a value larger than what the RQT can
>hold. This will emit errors and the driver state is compromised.
>
> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> required number of entries in the RQT. This value is evaluated in
> mlx5_vdpa_set_driver_features() where we have the negotiated features
> all set up.
>
> In addtion
addition?
> to that, we take into consideration the max capability of RQT
> entries early when the device is added so we don't need to take consider
> it when creating the RQT.
>
> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> max_vas / 2 and make the code clearer.
>
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Signed-off-by: Eli Cohen
Jason I don't have your ack or S.O.B on this one.
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++
> 1 file changed, 21 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 53b8c1a68f90..61bec1ed0bc9 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> struct mlx5_flow_handle *rx_rule_mcast;
> bool setup;
> u32 cur_num_vqs;
> + u32 rqt_size;
> struct notifier_block nb;
> struct vdpa_callback config_cb;
> struct mlx5_vdpa_wq_ent cvq_ent;
> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct
> mlx5_vdpa_dev *mvdev, u16 val)
> return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> }
>
> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> -{
> - return max_vqs / 2;
> -}
> -
> static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> {
> if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> return 2;
>
> - return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> + return mvdev->max_vqs;
> }
>
> static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev,
> struct mlx5_vdpa_virtqueue *
> static int create_rqt(struct mlx5_vdpa_net *ndev)
> {
> __be32 *list;
> - int max_rqt;
> void *rqtc;
> int inlen;
> void *in;
> int i, j;
> int err;
> - int num;
> -
> - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> - num = 1;
> - else
> - num = ndev->cur_num_vqs / 2;
>
> - max_rqt = min_t(int, roundup_pow_of_two(num),
> - 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> - if (max_rqt < 1)
> - return -EOPNOTSUPP;
> -
> - inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt *
> MLX5_ST_SZ_BYTES(rq_num);
> + inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size *
> MLX5_ST_SZ_BYTES(rq_num);
> in = kzalloc(inlen, GFP_KERNEL);
> if (!in)
> return -ENOMEM;
> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>
> MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> - MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> + MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> - for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> - list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> + for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> + list[i] = cpu_to_be32(ndev->vqs[j %
> ndev->cur_num_vqs].virtq_id);
>
> - MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> + MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> err = mlx5_vdpa_create_rqt(>mvdev, in, inlen, >res.rqtn);
> kfree(in);
> if (err)
> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> {
> __be32 *list;
> - int max_rqt;
> void *rqtc;
> int inlen;
> void *in;
> int i, j;
> int err;
>
> - max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> - 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> - if (max_rqt < 1)
> - return -EOPNOTSUPP;
> -
> - inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt *
> MLX5_ST_SZ_BYTES(rq_num);
> + inlen =
Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override
On Mon, Apr 4, 2022 at 12:34 PM Krzysztof Kozlowski
wrote:
> On 04/04/2022 11:16, Andy Shevchenko wrote:
> > On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski
> > wrote:
...
> >> +int driver_set_override(struct device *dev, const char **override,
> >> + const char *s, size_t len)
> >> +{
> >> + const char *new, *old;
> >> + char *cp;
> >
> >> + if (!override || !s)
> >> + return -EINVAL;
> >
> > Still not sure if we should distinguish (s == NULL && len == 0) from
> > (s != NULL && len == 0).
> > Supplying the latter seems confusing (yes, I see that in the old code).
> > Perhaps
> > !s test, in case you want to leave it, should be also commented.
>
> The old semantics were focused on sysfs usage, so clearing is by passing
> an empty string. In the case of sysfs empty string is actually "\n". I
> intend to keep the semantics also for the in-kernel usage and in such
> case empty string can be also "".
>
> If I understand your comment correctly, you propose to change it to NULL
> for in-kernel usage, but that would change the semantics.
Yes. It's also possible to have a wrapper for sysfs use.
> > Another approach is to split above to two checks and move !s after !len.
>
> I don't follow why... The !override and !s are invalid uses. !len is a
> valid user for internal callers, just like "\n" is for sysfs.
I understand but always supplying s maybe an overhead for in-kernel usages.
In any case, it's not critical right now, just a remark that it can be modified.
> >> + /*
> >> +* The stored value will be used in sysfs show callback
> >> (sysfs_emit()),
> >> +* which has a length limit of PAGE_SIZE and adds a trailing
> >> newline.
> >> +* Thus we can store one character less to avoid truncation during
> >> sysfs
> >> +* show.
> >> +*/
> >> + if (len >= (PAGE_SIZE - 1))
> >> + return -EINVAL;
> >
> > Perhaps explain the case in the comment here?
>
> You mean the case we discuss here (to clear override with "")? Sure.
Yep. Before the below check.
> >> + if (!len) {
> >> + device_lock(dev);
> >> + old = *override;
> >> + *override = NULL;
> >
> >> + device_unlock(dev);
> >> + goto out_free;
> >
> > You may deduplicate this one, by
> >
> >goto out_unlock_free;
> >
> > But I understand your intention to keep lock-unlock in one place, so
> > perhaps dropping that label would be even better in this case and
> > keeping it
>
> Yes, exactly.
>
> >kfree(old);
> >return 0;
> >
> > here instead of goto.
>
> Slightly more code, but indeed maybe easier to follow. I'll do like this.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override
On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski
wrote:
>
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
>
> However such assumption is not documented, there were in the past and
> there are already users setting it to a string literal. This leads to
> kfree() of static memory during device release (e.g. in error paths or
> during unbind):
>
> kernel BUG at ../mm/slub.c:3960!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kfree) from [] (platform_device_release+0x88/0xb4)
> (platform_device_release) from [] (device_release+0x2c/0x90)
> (device_release) from [] (kobject_put+0xec/0x20c)
> (kobject_put) from [] (exynos5_clk_probe+0x154/0x18c)
> (exynos5_clk_probe) from [] (platform_drv_probe+0x6c/0xa4)
> (platform_drv_probe) from [] (really_probe+0x280/0x414)
> (really_probe) from [] (driver_probe_device+0x78/0x1c4)
> (driver_probe_device) from [] (bus_for_each_drv+0x74/0xb8)
> (bus_for_each_drv) from [] (__device_attach+0xd4/0x16c)
> (__device_attach) from [] (bus_probe_device+0x88/0x90)
> (bus_probe_device) from [] (device_add+0x3dc/0x62c)
> (device_add) from [] (of_platform_device_create_pdata+0x94/0xbc)
> (of_platform_device_create_pdata) from []
> (of_platform_bus_create+0x1a8/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_bus_create+0x20c/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_populate+0x84/0x118)
> (of_platform_populate) from []
> (of_platform_default_populate_init+0xa0/0xb8)
> (of_platform_default_populate_init) from []
> (do_one_initcall+0x8c/0x404)
>
> Provide a helper which clearly documents the usage of driver_override.
> This will allow later to reuse the helper and reduce the amount of
> duplicated code.
>
> Convert the platform driver to use a new helper and make the
> driver_override field const char (it is not modified by the core).
...
> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;
> + if (!override || !s)
> + return -EINVAL;
Still not sure if we should distinguish (s == NULL && len == 0) from
(s != NULL && len == 0).
Supplying the latter seems confusing (yes, I see that in the old code). Perhaps
!s test, in case you want to leave it, should be also commented.
Another approach is to split above to two checks and move !s after !len.
> + /*
> +* The stored value will be used in sysfs show callback
> (sysfs_emit()),
> +* which has a length limit of PAGE_SIZE and adds a trailing newline.
> +* Thus we can store one character less to avoid truncation during
> sysfs
> +* show.
> +*/
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;
Perhaps explain the case in the comment here?
> + if (!len) {
> + device_lock(dev);
> + old = *override;
> + *override = NULL;
> + device_unlock(dev);
> + goto out_free;
You may deduplicate this one, by
goto out_unlock_free;
But I understand your intention to keep lock-unlock in one place, so
perhaps dropping that label would be even better in this case and
keeping it
kfree(old);
return 0;
here instead of goto.
> + }
> +
> + cp = strnchr(s, len, '\n');
> + if (cp)
> + len = cp - s;
> +
> + new = kstrndup(s, len, GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + device_lock(dev);
> + old = *override;
> + if (cp != s) {
> + *override = new;
> + } else {
> + kfree(new);
> + *override = NULL;
> + }
> + device_unlock(dev);
> +
> +out_free:
> + kfree(old);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] fuse: avoid unnecessary spinlock bump
On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote: > Move dmap free worker kicker inside the critical region, so that extra > spinlock lock/unlock could be avoided. > > Suggested-by: Liu Jiang > Signed-off-by: Jeffle Xu > --- > fs/fuse/dax.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
