Re: [PATCH V2] vdpa: allow provisioning device features
On Wed, Nov 23, 2022 at 3:53 AM David Ahern wrote:
>
> On 11/16/22 8:33 PM, Jason Wang wrote:
> > diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> > index 94e4dad1..7c961991 100644
> > --- a/vdpa/include/uapi/linux/vdpa.h
> > +++ b/vdpa/include/uapi/linux/vdpa.h
> > @@ -51,6 +51,7 @@ enum vdpa_attr {
> > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */
> > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
> > VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,/* u64 */
> > + VDPA_ATTR_DEV_FEATURES, /* u64 */
> >
> > /* new attributes must be added above here */
> > VDPA_ATTR_MAX,
>
> this header file already has:
> ...
> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */
> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,/* u64 */
>
> VDPA_ATTR_DEV_FEATURES, /* u64 */
>
> /* virtio features that are supported by the vDPA device */
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
>
> /* new attributes must be added above here */
> VDPA_ATTR_MAX,
>
> in which case your diff is not needed. More importantly it raises
> questions about the status of the uapi file (is it correct as is or is
> an update needed) and which tree you are creating patches against?
I'm using git://git.kernel.org/pub/scm/network/iproute2/iproute2 main.
But I don't pull the new codes before sending the patches. Will fix
this.
>
> > @@ -615,8 +640,9 @@ static int cmd_mgmtdev(struct vdpa *vdpa, int argc,
> > char **argv)
> > static void cmd_dev_help(void)
> > {
> > fprintf(stderr, "Usage: vdpa dev show [ DEV ]\n");
> > - fprintf(stderr, " vdpa dev add name NAME mgmtdev MANAGEMENTDEV
> > [ mac MACADDR ] [ mtu MTU ]\n");
> > - fprintf(stderr, "
> > [ max_vqp MAX_VQ_PAIRS ]\n");
> > + fprintf(stderr, " vdpa dev add name NAME mgmtdevMANAGEMENTDEV [
> > device_features DEVICE_FEATURES]\n");
>
> lost the space between mgmtdev and MANAGEMENTDEV
Will fix it.
Thanks
>
>
> > + fprintf(stderr, " [
> > mac MACADDR ] [ mtu MTU ]\n");
> > + fprintf(stderr, " [
> > max_vqp MAX_VQ_PAIRS ]\n");
> > fprintf(stderr, " vdpa dev del DEV\n");
> > fprintf(stderr, "Usage: vdpa dev config COMMAND [ OPTIONS ]\n");
> > fprintf(stderr, "Usage: vdpa dev vstats COMMAND\n");
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/7] vdpa/mlx5: Avoid using reslock in event_handler
On Mon, Nov 14, 2022 at 4:58 PM Eli Cohen wrote:
>
> > From: Jason Wang
> > Sent: Monday, 14 November 2022 9:53
> > To: Eli Cohen
> > Cc: m...@redhat.com; linux-ker...@vger.kernel.org;
> > virtualization@lists.linux-
> > foundation.org; si-wei@oracle.com; epere...@redhat.com;
> > l...@redhat.com
> > Subject: Re: [PATCH 6/7] vdpa/mlx5: Avoid using reslock in event_handler
> >
> > On Sun, Nov 13, 2022 at 9:45 PM Eli Cohen wrote:
> > >
> > > event_handler runs under atomic context and may not acquire reslock. We
> > > can still guarantee that the handler won't be called after suspend by
> > > clearing nb_registered, unregistering the handler and flushing the
> > > workqueue.
> > >
> > > Signed-off-by: Eli Cohen
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 14 +++---
> > > 1 file changed, 3 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 6e6490c85be2..bebfba530247 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -2872,8 +2872,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device
> > *vdev)
> > > int i;
> > >
> > > down_write(>reslock);
> > > - mlx5_notifier_unregister(mvdev->mdev, >nb);
> > > ndev->nb_registered = false;
> > > + mlx5_notifier_unregister(mvdev->mdev, >nb);
> >
> > I wonder why this can help anything.
> I think you were concerned that async events will come when the device was
> suspended. Since we can't take reslock, I think this guarantees that we won't
> get any events after suspension.
>
Ok, I see.
> > And if it does, we have simliar
> > logic in mlx5_vdpa_dev_del() do we need to fix that as well?
> >
> We have the same construct there only that I set nb_registered = false after
> unregistering the notifier. So I probably need to move it before
> mlx5_notifier_unregister().
Right.
Thanks
>
> > Thanks
> >
> > > flush_workqueue(ndev->mvdev.wq);
> > > for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > mvq = >vqs[i];
> > > @@ -3051,7 +3051,7 @@ static void update_carrier(struct work_struct
> > *work)
> > > else
> > > ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
> > ~VIRTIO_NET_S_LINK_UP);
> > >
> > > - if (ndev->config_cb.callback)
> > > + if (ndev->nb_registered && ndev->config_cb.callback)
> > > ndev->config_cb.callback(ndev->config_cb.private);
> > >
> > > kfree(wqent);
> > > @@ -3068,21 +3068,13 @@ static int event_handler(struct notifier_block
> > *nb, unsigned long event, void *p
> > > switch (eqe->sub_type) {
> > > case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> > > case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> > > - down_read(>reslock);
> > > - if (!ndev->nb_registered) {
> > > - up_read(>reslock);
> > > - return NOTIFY_DONE;
> > > - }
> > > wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> > > - if (!wqent) {
> > > - up_read(>reslock);
> > > + if (!wqent)
> > > return NOTIFY_DONE;
> > > - }
> > >
> > > wqent->mvdev = >mvdev;
> > > INIT_WORK(>work, update_carrier);
> > > queue_work(ndev->mvdev.wq, >work);
> > > - up_read(>reslock);
> > > ret = NOTIFY_OK;
> > > break;
> > > default:
> > > --
> > > 2.38.1
> > >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vdpa: allow provisioning device features
On Wed, Nov 23, 2022 at 6:29 AM Si-Wei Liu wrote:
>
>
>
> On 11/16/2022 7:33 PM, Jason Wang wrote:
> > This patch allows device features to be provisioned via vdpa. This
> > will be useful for preserving migration compatibility between source
> > and destination:
> >
> > # vdpa dev add name dev1 mgmtdev pci/:02:00.0 device_features
> > 0x30002
> Miss the actual "vdpa dev config show" command below
Right, let me fix that.
> > # dev1: mac 52:54:00:12:34:56 link up link_announce false mtu 65535
> >negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
> >
> > Signed-off-by: Jason Wang
> > ---
> > Changes since v1:
> > - Use uint64_t instead of __u64 for device_features
> > - Fix typos and tweak the manpage
> > - Add device_features to the help text
> > ---
> > man/man8/vdpa-dev.8| 15 +++
> > vdpa/include/uapi/linux/vdpa.h | 1 +
> > vdpa/vdpa.c| 32 +---
> > 3 files changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8
> > index 9faf3838..43e5bf48 100644
> > --- a/man/man8/vdpa-dev.8
> > +++ b/man/man8/vdpa-dev.8
> > @@ -31,6 +31,7 @@ vdpa-dev \- vdpa device configuration
> > .I NAME
> > .B mgmtdev
> > .I MGMTDEV
> > +.RI "[ device_features " DEVICE_FEATURES " ]"
> > .RI "[ mac " MACADDR " ]"
> > .RI "[ mtu " MTU " ]"
> > .RI "[ max_vqp " MAX_VQ_PAIRS " ]"
> > @@ -74,6 +75,15 @@ Name of the new vdpa device to add.
> > Name of the management device to use for device addition.
> >
> > .PP
> > +.BI device_features " DEVICE_FEATURES"
> > +Specifies the virtio device features bit-mask that is provisioned for the
> > new vdpa device.
> > +
> > +The bits can be found under include/uapi/linux/virtio*h.
> > +
> > +see macros such as VIRTIO_F_ and VIRTIO_XXX(e.g NET)_F_ for specific bit
> > values.
> > +
> > +This is optional.
> Document the behavior when this attribute is missing? For e.g. inherit
> device features from parent device.
This is the current behaviour but unless we've found a way to mandate
it, I'd like to not mention it. Maybe add a description to say the
user needs to check the features after the add if features are not
specified.
>
> And what is the expected behavior when feature bit mask is off but the
> corresponding config attr (for e.g. mac, mtu, and max_vqp) is set?
It depends totally on the parent. And this "issue" is not introduced
by this feature. Parents can decide to provision MQ by itself even if
max_vqp is not specified.
> I
> think the previous behavior without device_features is that any config
> attr implies the presence of the specific corresponding feature (_F_MAC,
> _F_MTU, and _F_MQ). Should device_features override the other config
> attribute, or such combination is considered invalid thus should fail?
It follows the current policy, e.g if the parent doesn't support
_F_MQ, we can neither provision _F_MQ nor max_vqp.
Thanks
>
> Thanks,
> -Siwei
>
> > +
> > .BI mac " MACADDR"
> > - specifies the mac address for the new vdpa device.
> > This is applicable only for the network type of vdpa device. This is
> > optional.
> > @@ -127,6 +137,11 @@ vdpa dev add name foo mgmtdev vdpa_sim_net
> > Add the vdpa device named foo on the management device vdpa_sim_net.
> > .RE
> > .PP
> > +vdpa dev add name foo mgmtdev vdpa_sim_net device_features 0x30002
> > +.RS 4
> > +Add the vdpa device named foo on the management device vdpa_sim_net with
> > device_features of 0x30002
> > +.RE
> > +.PP
> > vdpa dev add name foo mgmtdev vdpa_sim_net mac 00:11:22:33:44:55
> > .RS 4
> > Add the vdpa device named foo on the management device vdpa_sim_net with
> > mac address of 00:11:22:33:44:55.
> > diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> > index 94e4dad1..7c961991 100644
> > --- a/vdpa/include/uapi/linux/vdpa.h
> > +++ b/vdpa/include/uapi/linux/vdpa.h
> > @@ -51,6 +51,7 @@ enum vdpa_attr {
> > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */
> > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
> > VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,/* u64 */
> > + VDPA_ATTR_DEV_FEATURES, /* u64 */
> >
> > /* new attributes must be added above here */
> > VDPA_ATTR_MAX,
> > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> > index b73e40b4..d0ce5e22 100644
> > --- a/vdpa/vdpa.c
> > +++ b/vdpa/vdpa.c
> > @@ -27,6 +27,7 @@
> > #define VDPA_OPT_VDEV_MTU BIT(5)
> > #define VDPA_OPT_MAX_VQPBIT(6)
> > #define VDPA_OPT_QUEUE_INDEXBIT(7)
> > +#define VDPA_OPT_VDEV_FEATURES BIT(8)
> >
> > struct vdpa_opts {
> > uint64_t present; /* flags of present items */
> > @@ -38,6 +39,7 @@ struct vdpa_opts {
> > uint16_t mtu;
> > uint16_t max_vqp;
> > uint32_t queue_idx;
> > + uint64_t device_features;
> > };
> >
> > struct vdpa {
> > @@ -187,6 +189,17 @@ static int
Re: [PATCH v2 9/9] dt-bindings: drop redundant part of title (manual)
On Mon, Nov 21, 2022 at 12:06:15PM +0100, Krzysztof Kozlowski wrote: > Documentation/devicetree/bindings/input/fsl,scu-key.yaml| 2 +- > Documentation/devicetree/bindings/input/matrix-keymap.yaml | 2 +- Acked-by: Dmitry Torokhov -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 7/9] dt-bindings: drop redundant part of title (beginning)
On Mon, Nov 21, 2022 at 12:06:13PM +0100, Krzysztof Kozlowski wrote: > Documentation/devicetree/bindings/input/gpio-keys.yaml | 2 +- > Documentation/devicetree/bindings/input/microchip,cap11xx.yaml | 2 +- Acked-by: Dmitry Torokhov -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 6/9] dt-bindings: drop redundant part of title (end, part three)
On Mon, Nov 21, 2022 at 12:06:12PM +0100, Krzysztof Kozlowski wrote: > .../bindings/input/touchscreen/cypress,cy8ctma140.yaml | 2 +- > .../bindings/input/touchscreen/cypress,cy8ctma340.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 2 +- > Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/melfas,mms114.yaml| 2 +- > .../devicetree/bindings/input/touchscreen/mstar,msg2638.yaml| 2 +- > .../devicetree/bindings/input/touchscreen/ti,tsc2005.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/touchscreen.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/zinitix,bt400.yaml| 2 +- Acked-by: Dmitry Torokhov -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/9] dt-bindings: drop redundant part of title (end)
On Mon, Nov 21, 2022 at 12:06:10PM +0100, Krzysztof Kozlowski wrote: > .../devicetree/bindings/input/pine64,pinephone-keyboard.yaml| 2 +- > .../devicetree/bindings/input/touchscreen/chipone,icn8318.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/pixcir,pixcir_ts.yaml | 2 +- > .../devicetree/bindings/input/touchscreen/silead,gsl1680.yaml | 2 +- Acked-by: Dmitry Torokhov -- Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH] crypto: virtio - Use helper to set reqsize
> -Original Message-
> From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
> Sent: Tuesday, November 22, 2022 5:42 PM
> To: Gonglei (Arei) ;
> virtualization@lists.linux-foundation.org; Linux Crypto Mailing List
>
> Subject: [PATCH] crypto: virtio - Use helper to set reqsize
>
> The value of reqsize must only be changed through the helper.
>
> Signed-off-by: Herbert Xu
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 168195672e2e..b2979be613b8 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -479,6 +479,9 @@ static int virtio_crypto_rsa_init_tfm(struct
> crypto_akcipher *tfm)
> ctx->enginectx.op.prepare_request = NULL;
> ctx->enginectx.op.unprepare_request = NULL;
>
> + akcipher_set_reqsize(tfm,
> + sizeof(struct virtio_crypto_akcipher_request));
> +
> return 0;
> }
>
> @@ -505,7 +508,6 @@ static struct virtio_crypto_akcipher_algo
> virtio_crypto_akcipher_algs[] = {
> .max_size = virtio_crypto_rsa_max_size,
> .init = virtio_crypto_rsa_init_tfm,
> .exit = virtio_crypto_rsa_exit_tfm,
> - .reqsize = sizeof(struct
> virtio_crypto_akcipher_request),
> .base = {
> .cra_name = "rsa",
> .cra_driver_name = "virtio-crypto-rsa", @@
> -528,7
> +530,6 @@ static struct virtio_crypto_akcipher_algo
> virtio_crypto_akcipher_algs[] = {
> .max_size = virtio_crypto_rsa_max_size,
> .init = virtio_crypto_rsa_init_tfm,
> .exit = virtio_crypto_rsa_exit_tfm,
> - .reqsize = sizeof(struct
> virtio_crypto_akcipher_request),
> .base = {
> .cra_name = "pkcs1pad(rsa,sha1)",
> .cra_driver_name = "virtio-pkcs1-rsa-with-sha1",
> --
Acked-by: Gonglei
Regards,
-Gonglei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vdpa: allow provisioning device features
On 11/16/2022 7:33 PM, Jason Wang wrote:
This patch allows device features to be provisioned via vdpa. This
will be useful for preserving migration compatibility between source
and destination:
# vdpa dev add name dev1 mgmtdev pci/:02:00.0 device_features 0x30002
Miss the actual "vdpa dev config show" command below
# dev1: mac 52:54:00:12:34:56 link up link_announce false mtu 65535
negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
Signed-off-by: Jason Wang
---
Changes since v1:
- Use uint64_t instead of __u64 for device_features
- Fix typos and tweak the manpage
- Add device_features to the help text
---
man/man8/vdpa-dev.8| 15 +++
vdpa/include/uapi/linux/vdpa.h | 1 +
vdpa/vdpa.c| 32 +---
3 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8
index 9faf3838..43e5bf48 100644
--- a/man/man8/vdpa-dev.8
+++ b/man/man8/vdpa-dev.8
@@ -31,6 +31,7 @@ vdpa-dev \- vdpa device configuration
.I NAME
.B mgmtdev
.I MGMTDEV
+.RI "[ device_features " DEVICE_FEATURES " ]"
.RI "[ mac " MACADDR " ]"
.RI "[ mtu " MTU " ]"
.RI "[ max_vqp " MAX_VQ_PAIRS " ]"
@@ -74,6 +75,15 @@ Name of the new vdpa device to add.
Name of the management device to use for device addition.
.PP
+.BI device_features " DEVICE_FEATURES"
+Specifies the virtio device features bit-mask that is provisioned for the new
vdpa device.
+
+The bits can be found under include/uapi/linux/virtio*h.
+
+see macros such as VIRTIO_F_ and VIRTIO_XXX(e.g NET)_F_ for specific bit
values.
+
+This is optional.
Document the behavior when this attribute is missing? For e.g. inherit
device features from parent device.
And what is the expected behavior when feature bit mask is off but the
corresponding config attr (for e.g. mac, mtu, and max_vqp) is set? I
think the previous behavior without device_features is that any config
attr implies the presence of the specific corresponding feature (_F_MAC,
_F_MTU, and _F_MQ). Should device_features override the other config
attribute, or such combination is considered invalid thus should fail?
Thanks,
-Siwei
+
.BI mac " MACADDR"
- specifies the mac address for the new vdpa device.
This is applicable only for the network type of vdpa device. This is optional.
@@ -127,6 +137,11 @@ vdpa dev add name foo mgmtdev vdpa_sim_net
Add the vdpa device named foo on the management device vdpa_sim_net.
.RE
.PP
+vdpa dev add name foo mgmtdev vdpa_sim_net device_features 0x30002
+.RS 4
+Add the vdpa device named foo on the management device vdpa_sim_net with
device_features of 0x30002
+.RE
+.PP
vdpa dev add name foo mgmtdev vdpa_sim_net mac 00:11:22:33:44:55
.RS 4
Add the vdpa device named foo on the management device vdpa_sim_net with mac
address of 00:11:22:33:44:55.
diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
index 94e4dad1..7c961991 100644
--- a/vdpa/include/uapi/linux/vdpa.h
+++ b/vdpa/include/uapi/linux/vdpa.h
@@ -51,6 +51,7 @@ enum vdpa_attr {
VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */
VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,/* u64 */
+ VDPA_ATTR_DEV_FEATURES, /* u64 */
/* new attributes must be added above here */
VDPA_ATTR_MAX,
diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
index b73e40b4..d0ce5e22 100644
--- a/vdpa/vdpa.c
+++ b/vdpa/vdpa.c
@@ -27,6 +27,7 @@
#define VDPA_OPT_VDEV_MTU BIT(5)
#define VDPA_OPT_MAX_VQP BIT(6)
#define VDPA_OPT_QUEUE_INDEX BIT(7)
+#define VDPA_OPT_VDEV_FEATURES BIT(8)
struct vdpa_opts {
uint64_t present; /* flags of present items */
@@ -38,6 +39,7 @@ struct vdpa_opts {
uint16_t mtu;
uint16_t max_vqp;
uint32_t queue_idx;
+ uint64_t device_features;
};
struct vdpa {
@@ -187,6 +189,17 @@ static int vdpa_argv_u32(struct vdpa *vdpa, int argc, char
**argv,
return get_u32(result, *argv, 10);
}
+static int vdpa_argv_u64_hex(struct vdpa *vdpa, int argc, char **argv,
+uint64_t *result)
+{
+ if (argc <= 0 || !*argv) {
+ fprintf(stderr, "number expected\n");
+ return -EINVAL;
+ }
+
+ return get_u64(result, *argv, 16);
+}
+
struct vdpa_args_metadata {
uint64_t o_flag;
const char *err_msg;
@@ -244,6 +257,10 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct
vdpa *vdpa)
mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
opts->max_vqp);
if (opts->present & VDPA_OPT_QUEUE_INDEX)
mnl_attr_put_u32(nlh, VDPA_ATTR_DEV_QUEUE_INDEX,
opts->queue_idx);
+ if (opts->present & VDPA_OPT_VDEV_FEATURES) {
+ mnl_attr_put_u64(nlh, VDPA_ATTR_DEV_FEATURES,
+ opts->device_features);
+
Re: [PATCH v2] virtio_net: Fix probe failed when modprobe virtio_net
On Tue, Nov 22, 2022 at 11:00:46PM +0800, Li Zetao wrote:
> When doing the following test steps, an error was found:
> step 1: modprobe virtio_net succeeded
> # modprobe virtio_net<-- OK
>
> step 2: fault injection in register_netdevice()
> # modprobe -r virtio_net <-- OK
> # ...
> FAULT_INJECTION: forcing a failure.
> name failslab, interval 1, probability 0, space 0, times 0
> CPU: 0 PID: 3521 Comm: modprobe
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> Call Trace:
>
>...
>should_failslab+0xa/0x20
>...
>dev_set_name+0xc0/0x100
>netdev_register_kobject+0xc2/0x340
>register_netdevice+0xbb9/0x1320
>virtnet_probe+0x1d72/0x2658 [virtio_net]
>...
>
> virtio_net: probe of virtio0 failed with error -22
>
> step 3: modprobe virtio_net failed
> # modprobe virtio_net<-- failed
> virtio_net: probe of virtio0 failed with error -2
>
> The root cause of the problem is that the queues are not
> disable
if you need to resend it:
not disabled
but that's minor, ok to ignore
> on the error handling path when register_netdevice()
> fails in virtnet_probe(), resulting in an error "-ENOENT"
> returned in the next modprobe call in setup_vq().
>
> virtio_pci_modern_device uses virtqueues to send or
> receive message, and "queue_enable" records whether the
> queues are available. In vp_modern_find_vqs(), all queues
> will be selected and activated, but once queues are enabled
> there is no way to go back except reset.
>
> Fix it by reset virtio device on error handling path. This
> makes error handling follow the same order as normal device
> cleanup in virtnet_remove() which does: unregister, destroy
> failover, then reset. And that flow is better tested than
> error handling so we can be reasonably sure it works well.
>
> Fixes: 02465021 ("virtio_net: fix use after free on allocation failure")
> Signed-off-by: Li Zetao
> Acked-by: Michael S. Tsirkin
Thanks, LGTM, feel free to merge.
> ---
> v1 was posted at:
> https://lore.kernel.org/all/20221121132935.2032325-1-lizet...@huawei.com/
> v1 -> v2: modify commit log and fixes tag
>
> drivers/net/virtio_net.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7106932c6f88..86e52454b5b5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3949,12 +3949,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> return 0;
>
> free_unregister_netdev:
> - virtio_reset_device(vdev);
> -
> unregister_netdev(dev);
> free_failover:
> net_failover_destroy(vi->failover);
> free_vqs:
> + virtio_reset_device(vdev);
> cancel_delayed_work_sync(>refill);
> free_receive_page_frags(vi);
> virtnet_del_vqs(vi);
> --
> 2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers
On Mon, 2022-11-14 at 18:38 +0100, Cédric Le Goater wrote: > When a virtio console port is initialized, it is registered as an hvc > console using a virtual console number. If a KVM guest is started with > multiple virtio console devices, the same vtermno (or virtual console > number) can be used to allocate different hvc consoles, which leads to > various communication problems later on. > > This is also reported in debugfs : > > # grep vtermno /sys/kernel/debug/virtio-ports/* > /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1 > /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1 > /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2 > /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3 > > Replace the next_vtermno global with an ID allocator and start the > allocation at 1 as it is today. Also recycle IDs when a console port > is removed. When the original virtio_console module was written, it didn't have support for multiple ports to be used this way. So the oddity you're seeing is left there deliberately: VMMs should not be instantiating console ports this way. I don't know if we should take in this change, but can you walk through all combinations of new/old guest and new/old hypervisor and ensure nothing's going to break -- and confirm with the spec this is still OK to do? It may not be a goal to still ensure launches of a new guest on a very old (say) Centos5 guest still works -- but that was the point of maintaining backward compat... Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/44] cpuidle,dt: Push RCU-idle into driver
On Wed, 16 Nov 2022 at 16:29, Peter Zijlstra wrote:
>
>
> Sorry; things keep getting in the way of finishing this :/
>
> As such, I need a bit of time to get on-track again..
>
> On Tue, Oct 04, 2022 at 01:03:57PM +0200, Ulf Hansson wrote:
>
> > > --- a/drivers/acpi/processor_idle.c
> > > +++ b/drivers/acpi/processor_idle.c
> > > @@ -1200,6 +1200,8 @@ static int acpi_processor_setup_lpi_stat
> > > state->target_residency = lpi->min_residency;
> > > if (lpi->arch_flags)
> > > state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> > > + if (lpi->entry_method == ACPI_CSTATE_FFH)
> > > + state->flags |= CPUIDLE_FLAG_RCU_IDLE;
> >
> > I assume the state index here will never be 0?
> >
> > If not, it may lead to that acpi_processor_ffh_lpi_enter() may trigger
> > CPU_PM_CPU_IDLE_ENTER_PARAM() to call ct_cpuidle_enter|exit() for an
> > idle-state that doesn't have the CPUIDLE_FLAG_RCU_IDLE bit set.
>
> I'm not quite sure I see how. AFAICT this condition above implies
> acpi_processor_ffh_lpi_enter() gets called, no?
>
> Which in turn is an unconditional __CPU_PM_CPU_IDLE_ENTER() user, so
> even if idx==0, it ends up in ct_idle_{enter,exit}().
Seems like I was overlooking something here, you are right, this
shouldn't really be a problem.
>
> >
> > > state->enter = acpi_idle_lpi_enter;
> > > drv->safe_state_index = i;
> > > }
> > > --- a/drivers/cpuidle/cpuidle-arm.c
> > > +++ b/drivers/cpuidle/cpuidle-arm.c
> > > @@ -53,6 +53,7 @@ static struct cpuidle_driver arm_idle_dr
> > > * handler for idle state index 0.
> > > */
> > > .states[0] = {
> > > + .flags = CPUIDLE_FLAG_RCU_IDLE,
> >
> > Comparing arm64 and arm32 idle-states/idle-drivers, the $subject
> > series ends up setting the CPUIDLE_FLAG_RCU_IDLE for the ARM WFI idle
> > state (state zero), but only for the arm64 and psci cases (mostly
> > arm64). For arm32 we would need to update the ARM_CPUIDLE_WFI_STATE
> > too, as that is what most arm32 idle-drivers are using. My point is,
> > the code becomes a bit inconsistent.
>
> True.
>
> > Perhaps it's easier to avoid setting the CPUIDLE_FLAG_RCU_IDLE bit for
> > all of the ARM WFI idle states, for both arm64 and arm32?
>
> As per the below?
>
> >
> > > .enter = arm_enter_idle_state,
> > > .exit_latency = 1,
> > > .target_residency = 1,
>
> > > --- a/include/linux/cpuidle.h
> > > +++ b/include/linux/cpuidle.h
> > > @@ -282,14 +282,18 @@ extern s64 cpuidle_governor_latency_req(
> > > int __ret = 0; \
> > > \
> > > if (!idx) { \
> > > + ct_idle_enter();\
> >
> > According to my comment above, we should then drop these calls to
> > ct_idle_enter and ct_idle_exit() here. Right?
>
> Yes, if we ensure idx==0 never has RCU_IDLE set then these must be
> removed.
>
> > > cpu_do_idle(); \
> > > + ct_idle_exit(); \
> > > return idx; \
> > > } \
> > > \
> > > if (!is_retention) \
> > > __ret = cpu_pm_enter();\
> > > if (!__ret) { \
> > > + ct_idle_enter();\
> > > __ret = low_level_idle_enter(state);\
> > > + ct_idle_exit(); \
> > > if (!is_retention) \
> > > cpu_pm_exit(); \
> > > } \
> > >
>
> So the basic premise is that everything that needs RCU inside the idle
> callback must set CPUIDLE_FLAG_RCU_IDLE and by doing that promise to
> call ct_idle_{enter,exit}() themselves.
>
> Setting RCU_IDLE is required when there is RCU usage, however even if
> there is no RCU usage, setting RCU_IDLE is fine, as long as
> ct_idle_{enter,exit}() then get called.
Right, I was thinking that it could make sense to shrink the window
for users getting this wrong. In other words, we shouldn't set the
CPUIDLE_FLAG_RCU_IDLE unless we really need to.
And as I said, consistent behaviour is also nice to have.
>
>
> So does the below (delta) look better to you?
Yes, it
Re: [PATCH v3] virtio_console: Introduce an ID allocator for virtual console numbers
On 22/11/2022 14.46, Cédric Le Goater wrote:
When a virtio console port is initialized, it is registered as an hvc
console using a virtual console number. If a KVM guest is started with
multiple virtio console devices, the same vtermno (or virtual console
number) can be used to allocate different hvc consoles, which leads to
various communication problems later on.
This is also reported in debugfs :
# grep vtermno /sys/kernel/debug/virtio-ports/*
/sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
/sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
/sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
/sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3
Replace the next_vtermno global with an ID allocator and start the
allocation at 1 as it is today. Also recycle IDs when a console port
is removed.
Signed-off-by: Cédric Le Goater
---
Changes in v3:
- made use of ida_alloc_min()
- free ID in case of error
Changes in v2:
- introduced an ID allocator
drivers/char/virtio_console.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9fa3c76a267f..6a821118d553 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,22 +49,11 @@ struct ports_driver_data {
/* List of all the devices we're handling */
struct list_head portdevs;
- /*
-* This is used to keep track of the number of hvc consoles
-* spawned by this driver. This number is given as the first
-* argument to hvc_alloc(). To correctly map an initial
-* console spawned via hvc_instantiate to the console being
-* hooked up via hvc_alloc, we need to pass the same vtermno.
-*
-* We also just assume the first console being initialised was
-* the first one that got used as the initial console.
-*/
- unsigned int next_vtermno;
-
/* All the console devices handled by this driver */
struct list_head consoles;
};
-static struct ports_driver_data pdrvdata = { .next_vtermno = 1};
+
+static struct ports_driver_data pdrvdata;
static DEFINE_SPINLOCK(pdrvdata_lock);
static DECLARE_COMPLETION(early_console_added);
@@ -89,6 +79,8 @@ struct console {
u32 vtermno;
};
+static DEFINE_IDA(vtermno_ida);
+
struct port_buffer {
char *buf;
@@ -1244,18 +1236,21 @@ static int init_port_console(struct port *port)
* pointers. The final argument is the output buffer size: we
* can do any size, so we put PAGE_SIZE here.
*/
- port->cons.vtermno = pdrvdata.next_vtermno;
+ ret = ida_alloc_min(_ida, 1, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+ port->cons.vtermno = ret;
port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, _ops, PAGE_SIZE);
if (IS_ERR(port->cons.hvc)) {
ret = PTR_ERR(port->cons.hvc);
dev_err(port->dev,
"error %d allocating hvc for port\n", ret);
port->cons.hvc = NULL;
+ ida_free(_ida, port->cons.vtermno);
return ret;
}
spin_lock_irq(_lock);
- pdrvdata.next_vtermno++;
list_add_tail(>cons.list, );
spin_unlock_irq(_lock);
port->guest_connected = true;
@@ -1532,6 +1527,7 @@ static void unplug_port(struct port *port)
list_del(>cons.list);
spin_unlock_irq(_lock);
hvc_remove(port->cons.hvc);
+ ida_free(_ida, port->cons.vtermno);
}
remove_port_data(port);
Reviewed-by: Thomas Huth
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] drm/fb-helper: Don't use the preferred depth for the BPP default
On Wed, Nov 16, 2022 at 05:09:17PM +0100, Thomas Zimmermann wrote:
> If no preferred value for bits-per-pixel has been given, fall back
> to 32. Never use the preferred depth. The color depth is the number
> of color/alpha bits per pixel, while bpp is the overall number of
> bits in most cases.
>
> Most noteworthy, XRGB has a depth of 24 and a bpp value of 32.
> Using depth for bpp would make the value 24 as well and format
> selection in fbdev helpers fails. Unfortunately XRGB is the most
> common format and the old heuristic therefore fails for most of
> the drivers (unless they implement the 24-bit RGB888 format).
>
> Picking a bpp of 32 will lateron result in a default depth of 24
> and the format XRGB. As XRGB is the default format for most
> of the current and legacy graphics stack, all drivers must support
> it. So it is the safe choice.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fbdev_generic.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index ab86956692795..0a4c160e0e58a 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -431,7 +431,6 @@ static const struct drm_client_funcs
> drm_fbdev_client_funcs = {
> * drm_fbdev_generic_setup() - Setup generic fbdev emulation
> * @dev: DRM device
> * @preferred_bpp: Preferred bits per pixel for the device.
> - * @dev->mode_config.preferred_depth is used if this is zero.
> *
> * This function sets up generic fbdev emulation for drivers that supports
> * dumb buffers with a virtual address and that can be mmap'ed.
> @@ -475,12 +474,16 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
> }
>
> /*
> - * FIXME: This mixes up depth with bpp, which results in a glorious
> - * mess, resulting in some drivers picking wrong fbdev defaults and
> - * others wrong preferred_depth defaults.
> + * Pick a preferred bpp of 32 if no value has been given. This
> + * will select XRGB for the framebuffer formats. All drivers
> + * have to support XRGB for backwards compatibility with legacy
> + * userspace, so it's the safe choice here.
> + *
> + * TODO: Replace struct drm_mode_config.preferred_depth and this
> + * bpp value with a preferred format that is given as struct
> + * drm_format_info. Then derive all other values from the
> + * format.
I concur on this being the right design. What I've attempted years ago,
but never managed to finish, is sort the formats list on the primary plane
in preference order (since that seems useful for other reasons), and then
let everyone derive the preferred_whatever from the first format of the
first primary plane automatically.
But doing that is a pretty huge refactor, since you get to audit every
driver. So I kinda gave up on that. But I still thing something in that
direction would be a good design overall, since then userspace could also
use the same trick to infer format preferences ...
Anyway on the series, since it pushes in a direction I wanted to fix years
ago but gave up because too ambitious :-)
Acked-by: Daniel Vetter
>*/
> - if (!preferred_bpp)
> - preferred_bpp = dev->mode_config.preferred_depth;
> if (!preferred_bpp)
> preferred_bpp = 32;
> fb_helper->preferred_bpp = preferred_bpp;
> --
> 2.38.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers
On 14/11/2022 18.38, Cédric Le Goater wrote:
When a virtio console port is initialized, it is registered as an hvc
console using a virtual console number. If a KVM guest is started with
multiple virtio console devices, the same vtermno (or virtual console
number) can be used to allocate different hvc consoles, which leads to
various communication problems later on.
This is also reported in debugfs :
# grep vtermno /sys/kernel/debug/virtio-ports/*
/sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
/sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
/sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
/sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3
Replace the next_vtermno global with an ID allocator and start the
allocation at 1 as it is today. Also recycle IDs when a console port
is removed.
Sounds like a good idea!
@@ -1244,8 +1236,11 @@ static int init_port_console(struct port *port)
* pointers. The final argument is the output buffer size: we
* can do any size, so we put PAGE_SIZE here.
*/
- port->cons.vtermno = pdrvdata.next_vtermno;
+ ret = ida_alloc_range(_ida, 1, ~0, GFP_KERNEL);
Just cosmetics: I think you could use ida_alloc_min() instead.
+ if (ret < 0)
+ return ret;
+ port->cons.vtermno = ret;
port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, _ops, PAGE_SIZE);
if (IS_ERR(port->cons.hvc)) {
ret = PTR_ERR(port->cons.hvc);
What if this if (IS_ERR()) error happens? The code seems to return early in
this case - shouldn't the ID be freed again in this case?
Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] crypto: virtio - Use helper to set reqsize
The value of reqsize must only be changed through the helper.
Signed-off-by: Herbert Xu
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 168195672e2e..b2979be613b8 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -479,6 +479,9 @@ static int virtio_crypto_rsa_init_tfm(struct
crypto_akcipher *tfm)
ctx->enginectx.op.prepare_request = NULL;
ctx->enginectx.op.unprepare_request = NULL;
+ akcipher_set_reqsize(tfm,
+sizeof(struct virtio_crypto_akcipher_request));
+
return 0;
}
@@ -505,7 +508,6 @@ static struct virtio_crypto_akcipher_algo
virtio_crypto_akcipher_algs[] = {
.max_size = virtio_crypto_rsa_max_size,
.init = virtio_crypto_rsa_init_tfm,
.exit = virtio_crypto_rsa_exit_tfm,
- .reqsize = sizeof(struct
virtio_crypto_akcipher_request),
.base = {
.cra_name = "rsa",
.cra_driver_name = "virtio-crypto-rsa",
@@ -528,7 +530,6 @@ static struct virtio_crypto_akcipher_algo
virtio_crypto_akcipher_algs[] = {
.max_size = virtio_crypto_rsa_max_size,
.init = virtio_crypto_rsa_init_tfm,
.exit = virtio_crypto_rsa_exit_tfm,
- .reqsize = sizeof(struct
virtio_crypto_akcipher_request),
.base = {
.cra_name = "pkcs1pad(rsa,sha1)",
.cra_driver_name = "virtio-pkcs1-rsa-with-sha1",
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
