| Message ID | 20231029155952.67686-6-yishaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce a vfio driver over virtio devices | expand |
On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas wrote: > Initialize the supported admin commands upon activating the admin queue. > > The supported commands are saved as part of the admin queue context, it > will be used by the next patches from this series. > > Note: > As we don't want to let upper layers to execute admin commands before > that this initialization step was completed, we set ref count to 1 only > post of that flow and use a non ref counted version command for this > internal flow. > > Signed-off-by: Yishai Hadas <yishaih@nvidia.com> > --- > drivers/virtio/virtio_pci_common.h | 1 + > drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- > 2 files changed, 77 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > index a21b9ba01a60..9e07e556a51a 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { > struct virtio_pci_vq_info info; > struct completion flush_done; > refcount_t refcount; > + u64 supported_cmds; > /* Name of the admin queue: avq.$index. */ > char name[10]; > u16 vq_index; > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index ccd7a4d9f57f..25e27aa79cab 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -19,6 +19,9 @@ > #define VIRTIO_RING_NO_LEGACY > #include "virtio_pci_common.h" > > +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > + struct virtio_admin_cmd *cmd); > + I don't much like forward declarations. Just order functions sensibly and they will not be needed. > static u64 vp_get_features(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) > WRITE_ONCE(admin_vq->abort, abort); > } > > +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) > +{ > + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); > + struct virtio_admin_cmd cmd = {}; > + struct scatterlist result_sg; > + struct scatterlist data_sg; > + __le64 *data; > + int ret; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + sg_init_one(&result_sg, data, sizeof(*data)); > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); > + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); > + cmd.result_sg = &result_sg; > + > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > + if (ret) > + goto end; > + > + sg_init_one(&data_sg, data, sizeof(*data)); > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); > + cmd.data_sg = &data_sg; > + cmd.result_sg = NULL; > + > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > + if (ret) > + goto end; > + > + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); > +end: > + kfree(data); > +} > + > static void vp_modern_avq_activate(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) > if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) > return; > > + virtio_pci_admin_init_cmd_list(vdev); > init_completion(&admin_vq->flush_done); > refcount_set(&admin_vq->refcount, 1); > vp_modern_avq_set_abort(admin_vq, false); > @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, > return true; > } > > +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > + struct scatterlist **sgs, > + unsigned int out_num, > + unsigned int in_num, > + void *data, > + gfp_t gfp) > +{ > + struct virtqueue *vq; > + int ret, len; > + > + vq = admin_vq->info.vq; > + > + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); > + if (ret < 0) > + return ret; > + > + if (unlikely(!virtqueue_kick(vq))) > + return -EIO; > + > + while (!virtqueue_get_buf(vq, &len) && > + !virtqueue_is_broken(vq)) > + cpu_relax(); > + > + if (virtqueue_is_broken(vq)) > + return -EIO; > + > + return 0; > +} > + This is tolerable I guess but it might pin the CPU for a long time. The difficulty is handling suprize removal well which we currently don't do with interrupts. I would say it's ok as is but add a TODO comments along the lines of /* TODO: use interrupts once these virtqueue_is_broken */ > static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > struct scatterlist **sgs, > unsigned int out_num, > @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > in_num++; > } > > - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || > + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) > + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > + out_num, in_num, > + sgs, GFP_KERNEL); > + else > + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > out_num, in_num, > sgs, GFP_KERNEL); > if (ret) { > -- > 2.27.0
On 29/10/2023 22:17, Michael S. Tsirkin wrote: > On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas wrote: >> Initialize the supported admin commands upon activating the admin queue. >> >> The supported commands are saved as part of the admin queue context, it >> will be used by the next patches from this series. >> >> Note: >> As we don't want to let upper layers to execute admin commands before >> that this initialization step was completed, we set ref count to 1 only >> post of that flow and use a non ref counted version command for this >> internal flow. >> >> Signed-off-by: Yishai Hadas <yishaih@nvidia.com> >> --- >> drivers/virtio/virtio_pci_common.h | 1 + >> drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- >> 2 files changed, 77 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h >> index a21b9ba01a60..9e07e556a51a 100644 >> --- a/drivers/virtio/virtio_pci_common.h >> +++ b/drivers/virtio/virtio_pci_common.h >> @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { >> struct virtio_pci_vq_info info; >> struct completion flush_done; >> refcount_t refcount; >> + u64 supported_cmds; >> /* Name of the admin queue: avq.$index. */ >> char name[10]; >> u16 vq_index; >> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c >> index ccd7a4d9f57f..25e27aa79cab 100644 >> --- a/drivers/virtio/virtio_pci_modern.c >> +++ b/drivers/virtio/virtio_pci_modern.c >> @@ -19,6 +19,9 @@ >> #define VIRTIO_RING_NO_LEGACY >> #include "virtio_pci_common.h" >> >> +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, >> + struct virtio_admin_cmd *cmd); >> + > I don't much like forward declarations. Just order functions sensibly > and they will not be needed. OK, will be part of V3. > >> static u64 vp_get_features(struct virtio_device *vdev) >> { >> struct virtio_pci_device *vp_dev = to_vp_device(vdev); >> @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) >> WRITE_ONCE(admin_vq->abort, abort); >> } >> >> +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) >> +{ >> + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); >> + struct virtio_admin_cmd cmd = {}; >> + struct scatterlist result_sg; >> + struct scatterlist data_sg; >> + __le64 *data; >> + int ret; >> + >> + data = kzalloc(sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return; >> + >> + sg_init_one(&result_sg, data, sizeof(*data)); >> + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); >> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); >> + cmd.result_sg = &result_sg; >> + >> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); >> + if (ret) >> + goto end; >> + >> + sg_init_one(&data_sg, data, sizeof(*data)); >> + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); >> + cmd.data_sg = &data_sg; >> + cmd.result_sg = NULL; >> + >> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); >> + if (ret) >> + goto end; >> + >> + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); >> +end: >> + kfree(data); >> +} >> + >> static void vp_modern_avq_activate(struct virtio_device *vdev) >> { >> struct virtio_pci_device *vp_dev = to_vp_device(vdev); >> @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) >> if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) >> return; >> >> + virtio_pci_admin_init_cmd_list(vdev); >> init_completion(&admin_vq->flush_done); >> refcount_set(&admin_vq->refcount, 1); >> vp_modern_avq_set_abort(admin_vq, false); >> @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, >> return true; >> } >> >> +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, >> + struct scatterlist **sgs, >> + unsigned int out_num, >> + unsigned int in_num, >> + void *data, >> + gfp_t gfp) >> +{ >> + struct virtqueue *vq; >> + int ret, len; >> + >> + vq = admin_vq->info.vq; >> + >> + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); >> + if (ret < 0) >> + return ret; >> + >> + if (unlikely(!virtqueue_kick(vq))) >> + return -EIO; >> + >> + while (!virtqueue_get_buf(vq, &len) && >> + !virtqueue_is_broken(vq)) >> + cpu_relax(); >> + >> + if (virtqueue_is_broken(vq)) >> + return -EIO; >> + >> + return 0; >> +} >> + > > This is tolerable I guess but it might pin the CPU for a long time. > The difficulty is handling suprize removal well which we currently > don't do with interrupts. I would say it's ok as is but add > a TODO comments along the lines of /* TODO: use interrupts once these virtqueue_is_broken */ I assume that you asked for adding the below comment before the while loop: /* TODO use interrupts to reduce cpu cycles in the future */ Right ? Yishai > >> static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, >> struct scatterlist **sgs, >> unsigned int out_num, >> @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, >> in_num++; >> } >> >> - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >> + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || >> + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) >> + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >> + out_num, in_num, >> + sgs, GFP_KERNEL); >> + else >> + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >> out_num, in_num, >> sgs, GFP_KERNEL); >> if (ret) { >> -- >> 2.27.0
On Mon, Oct 30, 2023 at 05:27:50PM +0200, Yishai Hadas wrote: > On 29/10/2023 22:17, Michael S. Tsirkin wrote: > > On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas wrote: > > > Initialize the supported admin commands upon activating the admin queue. > > > > > > The supported commands are saved as part of the admin queue context, it > > > will be used by the next patches from this series. > > > > > > Note: > > > As we don't want to let upper layers to execute admin commands before > > > that this initialization step was completed, we set ref count to 1 only > > > post of that flow and use a non ref counted version command for this > > > internal flow. > > > > > > Signed-off-by: Yishai Hadas <yishaih@nvidia.com> > > > --- > > > drivers/virtio/virtio_pci_common.h | 1 + > > > drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- > > > 2 files changed, 77 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > > > index a21b9ba01a60..9e07e556a51a 100644 > > > --- a/drivers/virtio/virtio_pci_common.h > > > +++ b/drivers/virtio/virtio_pci_common.h > > > @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { > > > struct virtio_pci_vq_info info; > > > struct completion flush_done; > > > refcount_t refcount; > > > + u64 supported_cmds; > > > /* Name of the admin queue: avq.$index. */ > > > char name[10]; > > > u16 vq_index; > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > index ccd7a4d9f57f..25e27aa79cab 100644 > > > --- a/drivers/virtio/virtio_pci_modern.c > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > @@ -19,6 +19,9 @@ > > > #define VIRTIO_RING_NO_LEGACY > > > #include "virtio_pci_common.h" > > > +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > > > + struct virtio_admin_cmd *cmd); > > > + > > I don't much like forward declarations. Just order functions sensibly > > and they will not be needed. > > OK, will be part of V3. > > > > > > static u64 vp_get_features(struct virtio_device *vdev) > > > { > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) > > > WRITE_ONCE(admin_vq->abort, abort); > > > } > > > +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) > > > +{ > > > + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); > > > + struct virtio_admin_cmd cmd = {}; > > > + struct scatterlist result_sg; > > > + struct scatterlist data_sg; > > > + __le64 *data; > > > + int ret; > > > + > > > + data = kzalloc(sizeof(*data), GFP_KERNEL); > > > + if (!data) > > > + return; > > > + > > > + sg_init_one(&result_sg, data, sizeof(*data)); > > > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); > > > + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); > > > + cmd.result_sg = &result_sg; > > > + > > > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > > > + if (ret) > > > + goto end; > > > + > > > + sg_init_one(&data_sg, data, sizeof(*data)); > > > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); > > > + cmd.data_sg = &data_sg; > > > + cmd.result_sg = NULL; > > > + > > > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > > > + if (ret) > > > + goto end; > > > + > > > + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); > > > +end: > > > + kfree(data); > > > +} > > > + > > > static void vp_modern_avq_activate(struct virtio_device *vdev) > > > { > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) > > > if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) > > > return; > > > + virtio_pci_admin_init_cmd_list(vdev); > > > init_completion(&admin_vq->flush_done); > > > refcount_set(&admin_vq->refcount, 1); > > > vp_modern_avq_set_abort(admin_vq, false); > > > @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, > > > return true; > > > } > > > +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > > > + struct scatterlist **sgs, > > > + unsigned int out_num, > > > + unsigned int in_num, > > > + void *data, > > > + gfp_t gfp) > > > +{ > > > + struct virtqueue *vq; > > > + int ret, len; > > > + > > > + vq = admin_vq->info.vq; > > > + > > > + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (unlikely(!virtqueue_kick(vq))) > > > + return -EIO; > > > + > > > + while (!virtqueue_get_buf(vq, &len) && > > > + !virtqueue_is_broken(vq)) > > > + cpu_relax(); > > > + > > > + if (virtqueue_is_broken(vq)) > > > + return -EIO; > > > + > > > + return 0; > > > +} > > > + > > > > This is tolerable I guess but it might pin the CPU for a long time. > > The difficulty is handling suprize removal well which we currently > > don't do with interrupts. I would say it's ok as is but add > > a TODO comments along the lines of /* TODO: use interrupts once these virtqueue_is_broken */ > > I assume that you asked for adding the below comment before the while loop: > /* TODO use interrupts to reduce cpu cycles in the future */ > > Right ? > > Yishai Well I wrote what I meant. in the future is redundant - everyone knows we can't change the past. > > > > > static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > > > struct scatterlist **sgs, > > > unsigned int out_num, > > > @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > > > in_num++; > > > } > > > - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || > > > + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) > > > + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > + out_num, in_num, > > > + sgs, GFP_KERNEL); > > > + else > > > + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > out_num, in_num, > > > sgs, GFP_KERNEL); > > > if (ret) { > > > -- > > > 2.27.0 >
On 30/10/2023 17:57, Michael S. Tsirkin wrote: > On Mon, Oct 30, 2023 at 05:27:50PM +0200, Yishai Hadas wrote: >> On 29/10/2023 22:17, Michael S. Tsirkin wrote: >>> On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas wrote: >>>> Initialize the supported admin commands upon activating the admin queue. >>>> >>>> The supported commands are saved as part of the admin queue context, it >>>> will be used by the next patches from this series. >>>> >>>> Note: >>>> As we don't want to let upper layers to execute admin commands before >>>> that this initialization step was completed, we set ref count to 1 only >>>> post of that flow and use a non ref counted version command for this >>>> internal flow. >>>> >>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com> >>>> --- >>>> drivers/virtio/virtio_pci_common.h | 1 + >>>> drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- >>>> 2 files changed, 77 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h >>>> index a21b9ba01a60..9e07e556a51a 100644 >>>> --- a/drivers/virtio/virtio_pci_common.h >>>> +++ b/drivers/virtio/virtio_pci_common.h >>>> @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { >>>> struct virtio_pci_vq_info info; >>>> struct completion flush_done; >>>> refcount_t refcount; >>>> + u64 supported_cmds; >>>> /* Name of the admin queue: avq.$index. */ >>>> char name[10]; >>>> u16 vq_index; >>>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c >>>> index ccd7a4d9f57f..25e27aa79cab 100644 >>>> --- a/drivers/virtio/virtio_pci_modern.c >>>> +++ b/drivers/virtio/virtio_pci_modern.c >>>> @@ -19,6 +19,9 @@ >>>> #define VIRTIO_RING_NO_LEGACY >>>> #include "virtio_pci_common.h" >>>> +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, >>>> + struct virtio_admin_cmd *cmd); >>>> + >>> I don't much like forward declarations. Just order functions sensibly >>> and they will not be needed. >> OK, will be part of V3. >> >>>> static u64 vp_get_features(struct virtio_device *vdev) >>>> { >>>> struct virtio_pci_device *vp_dev = to_vp_device(vdev); >>>> @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) >>>> WRITE_ONCE(admin_vq->abort, abort); >>>> } >>>> +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) >>>> +{ >>>> + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); >>>> + struct virtio_admin_cmd cmd = {}; >>>> + struct scatterlist result_sg; >>>> + struct scatterlist data_sg; >>>> + __le64 *data; >>>> + int ret; >>>> + >>>> + data = kzalloc(sizeof(*data), GFP_KERNEL); >>>> + if (!data) >>>> + return; >>>> + >>>> + sg_init_one(&result_sg, data, sizeof(*data)); >>>> + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); >>>> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); >>>> + cmd.result_sg = &result_sg; >>>> + >>>> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); >>>> + if (ret) >>>> + goto end; >>>> + >>>> + sg_init_one(&data_sg, data, sizeof(*data)); >>>> + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); >>>> + cmd.data_sg = &data_sg; >>>> + cmd.result_sg = NULL; >>>> + >>>> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); >>>> + if (ret) >>>> + goto end; >>>> + >>>> + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); >>>> +end: >>>> + kfree(data); >>>> +} >>>> + >>>> static void vp_modern_avq_activate(struct virtio_device *vdev) >>>> { >>>> struct virtio_pci_device *vp_dev = to_vp_device(vdev); >>>> @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) >>>> if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) >>>> return; >>>> + virtio_pci_admin_init_cmd_list(vdev); >>>> init_completion(&admin_vq->flush_done); >>>> refcount_set(&admin_vq->refcount, 1); >>>> vp_modern_avq_set_abort(admin_vq, false); >>>> @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, >>>> return true; >>>> } >>>> +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, >>>> + struct scatterlist **sgs, >>>> + unsigned int out_num, >>>> + unsigned int in_num, >>>> + void *data, >>>> + gfp_t gfp) >>>> +{ >>>> + struct virtqueue *vq; >>>> + int ret, len; >>>> + >>>> + vq = admin_vq->info.vq; >>>> + >>>> + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (unlikely(!virtqueue_kick(vq))) >>>> + return -EIO; >>>> + >>>> + while (!virtqueue_get_buf(vq, &len) && >>>> + !virtqueue_is_broken(vq)) >>>> + cpu_relax(); >>>> + >>>> + if (virtqueue_is_broken(vq)) >>>> + return -EIO; >>>> + >>>> + return 0; >>>> +} >>>> + >>> This is tolerable I guess but it might pin the CPU for a long time. >>> The difficulty is handling suprize removal well which we currently >>> don't do with interrupts. I would say it's ok as is but add >>> a TODO comments along the lines of /* TODO: use interrupts once these virtqueue_is_broken */ >> I assume that you asked for adding the below comment before the while loop: >> /* TODO use interrupts to reduce cpu cycles in the future */ >> >> Right ? >> >> Yishai > Well I wrote what I meant. in the future is redundant - everyone knows > we can't change the past. I agree, no need for 'in the future'. However, in your suggestion you mentioned "once these virtqueue_is_broken". What does that mean in current polling mode ? Yishai > >>>> static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, >>>> struct scatterlist **sgs, >>>> unsigned int out_num, >>>> @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, >>>> in_num++; >>>> } >>>> - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >>>> + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || >>>> + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) >>>> + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >>>> + out_num, in_num, >>>> + sgs, GFP_KERNEL); >>>> + else >>>> + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, >>>> out_num, in_num, >>>> sgs, GFP_KERNEL); >>>> if (ret) { >>>> -- >>>> 2.27.0
On Mon, Oct 30, 2023 at 06:06:45PM +0200, Yishai Hadas wrote: > On 30/10/2023 17:57, Michael S. Tsirkin wrote: > > On Mon, Oct 30, 2023 at 05:27:50PM +0200, Yishai Hadas wrote: > > > On 29/10/2023 22:17, Michael S. Tsirkin wrote: > > > > On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas wrote: > > > > > Initialize the supported admin commands upon activating the admin queue. > > > > > > > > > > The supported commands are saved as part of the admin queue context, it > > > > > will be used by the next patches from this series. > > > > > > > > > > Note: > > > > > As we don't want to let upper layers to execute admin commands before > > > > > that this initialization step was completed, we set ref count to 1 only > > > > > post of that flow and use a non ref counted version command for this > > > > > internal flow. > > > > > > > > > > Signed-off-by: Yishai Hadas <yishaih@nvidia.com> > > > > > --- > > > > > drivers/virtio/virtio_pci_common.h | 1 + > > > > > drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- > > > > > 2 files changed, 77 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > > > > > index a21b9ba01a60..9e07e556a51a 100644 > > > > > --- a/drivers/virtio/virtio_pci_common.h > > > > > +++ b/drivers/virtio/virtio_pci_common.h > > > > > @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { > > > > > struct virtio_pci_vq_info info; > > > > > struct completion flush_done; > > > > > refcount_t refcount; > > > > > + u64 supported_cmds; > > > > > /* Name of the admin queue: avq.$index. */ > > > > > char name[10]; > > > > > u16 vq_index; > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > > > index ccd7a4d9f57f..25e27aa79cab 100644 > > > > > --- a/drivers/virtio/virtio_pci_modern.c > > > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > > > @@ -19,6 +19,9 @@ > > > > > #define VIRTIO_RING_NO_LEGACY > > > > > #include "virtio_pci_common.h" > > > > > +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > > > > > + struct virtio_admin_cmd *cmd); > > > > > + > > > > I don't much like forward declarations. Just order functions sensibly > > > > and they will not be needed. > > > OK, will be part of V3. > > > > > > > > static u64 vp_get_features(struct virtio_device *vdev) > > > > > { > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > > > @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) > > > > > WRITE_ONCE(admin_vq->abort, abort); > > > > > } > > > > > +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) > > > > > +{ > > > > > + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); > > > > > + struct virtio_admin_cmd cmd = {}; > > > > > + struct scatterlist result_sg; > > > > > + struct scatterlist data_sg; > > > > > + __le64 *data; > > > > > + int ret; > > > > > + > > > > > + data = kzalloc(sizeof(*data), GFP_KERNEL); > > > > > + if (!data) > > > > > + return; > > > > > + > > > > > + sg_init_one(&result_sg, data, sizeof(*data)); > > > > > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); > > > > > + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); > > > > > + cmd.result_sg = &result_sg; > > > > > + > > > > > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > > > > > + if (ret) > > > > > + goto end; > > > > > + > > > > > + sg_init_one(&data_sg, data, sizeof(*data)); > > > > > + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); > > > > > + cmd.data_sg = &data_sg; > > > > > + cmd.result_sg = NULL; > > > > > + > > > > > + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); > > > > > + if (ret) > > > > > + goto end; > > > > > + > > > > > + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); > > > > > +end: > > > > > + kfree(data); > > > > > +} > > > > > + > > > > > static void vp_modern_avq_activate(struct virtio_device *vdev) > > > > > { > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > > > @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) > > > > > if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) > > > > > return; > > > > > + virtio_pci_admin_init_cmd_list(vdev); > > > > > init_completion(&admin_vq->flush_done); > > > > > refcount_set(&admin_vq->refcount, 1); > > > > > vp_modern_avq_set_abort(admin_vq, false); > > > > > @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, > > > > > return true; > > > > > } > > > > > +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > > > > > + struct scatterlist **sgs, > > > > > + unsigned int out_num, > > > > > + unsigned int in_num, > > > > > + void *data, > > > > > + gfp_t gfp) > > > > > +{ > > > > > + struct virtqueue *vq; > > > > > + int ret, len; > > > > > + > > > > > + vq = admin_vq->info.vq; > > > > > + > > > > > + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + if (unlikely(!virtqueue_kick(vq))) > > > > > + return -EIO; > > > > > + > > > > > + while (!virtqueue_get_buf(vq, &len) && > > > > > + !virtqueue_is_broken(vq)) > > > > > + cpu_relax(); > > > > > + > > > > > + if (virtqueue_is_broken(vq)) > > > > > + return -EIO; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > This is tolerable I guess but it might pin the CPU for a long time. > > > > The difficulty is handling suprize removal well which we currently > > > > don't do with interrupts. I would say it's ok as is but add > > > > a TODO comments along the lines of /* TODO: use interrupts once these virtqueue_is_broken */ > > > I assume that you asked for adding the below comment before the while loop: > > > /* TODO use interrupts to reduce cpu cycles in the future */ > > > > > > Right ? > > > > > > Yishai > > Well I wrote what I meant. in the future is redundant - everyone knows > > we can't change the past. > > I agree, no need for 'in the future'. > > However, in your suggestion you mentioned "once these virtqueue_is_broken". > > What does that mean in current polling mode ? > > Yishai Maye better: /* TODO: use vq callback once it supports virtqueue_is_broken */ > > > > > > > static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > > > > > struct scatterlist **sgs, > > > > > unsigned int out_num, > > > > > @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > > > > > in_num++; > > > > > } > > > > > - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > > > + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || > > > > > + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) > > > > > + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > > > + out_num, in_num, > > > > > + sgs, GFP_KERNEL); > > > > > + else > > > > > + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > > > > > out_num, in_num, > > > > > sgs, GFP_KERNEL); > > > > > if (ret) { > > > > > -- > > > > > 2.27.0 >
Hi Yishai, kernel test robot noticed the following build warnings: [auto build test WARNING on awilliam-vfio/for-linus] [also build test WARNING on linus/master v6.6] [cannot apply to awilliam-vfio/next mst-vhost/linux-next next-20231102] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Yishai-Hadas/virtio-Define-feature-bit-for-administration-virtqueue/20231030-000414 base: https://github.com/awilliam/linux-vfio.git for-linus patch link: https://lore.kernel.org/r/20231029155952.67686-6-yishaih%40nvidia.com patch subject: [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands config: i386-randconfig-061-20231102 (https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp@intel.com/config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311030838.GjyaBTjM-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_pci_modern.c:726:16: sparse: sparse: restricted __le16 degrades to integer vim +726 drivers/virtio/virtio_pci_modern.c 673 674 static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, 675 struct virtio_admin_cmd *cmd) 676 { 677 struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat; 678 struct virtio_pci_device *vp_dev = to_vp_device(vdev); 679 struct virtio_admin_cmd_status *va_status; 680 unsigned int out_num = 0, in_num = 0; 681 struct virtio_admin_cmd_hdr *va_hdr; 682 struct virtqueue *avq; 683 u16 status; 684 int ret; 685 686 avq = virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ) ? 687 vp_dev->admin_vq.info.vq : NULL; 688 if (!avq) 689 return -EOPNOTSUPP; 690 691 va_status = kzalloc(sizeof(*va_status), GFP_KERNEL); 692 if (!va_status) 693 return -ENOMEM; 694 695 va_hdr = kzalloc(sizeof(*va_hdr), GFP_KERNEL); 696 if (!va_hdr) { 697 ret = -ENOMEM; 698 goto err_alloc; 699 } 700 701 va_hdr->opcode = cmd->opcode; 702 va_hdr->group_type = cmd->group_type; 703 va_hdr->group_member_id = cmd->group_member_id; 704 705 /* Add header */ 706 sg_init_one(&hdr, va_hdr, sizeof(*va_hdr)); 707 sgs[out_num] = &hdr; 708 out_num++; 709 710 if (cmd->data_sg) { 711 sgs[out_num] = cmd->data_sg; 712 out_num++; 713 } 714 715 /* Add return status */ 716 sg_init_one(&stat, va_status, sizeof(*va_status)); 717 sgs[out_num + in_num] = &stat; 718 in_num++; 719 720 if (cmd->result_sg) { 721 sgs[out_num + in_num] = cmd->result_sg; 722 in_num++; 723 } 724 725 if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || > 726 cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) 727 ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, 728 out_num, in_num, 729 sgs, GFP_KERNEL); 730 else 731 ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, 732 out_num, in_num, 733 sgs, GFP_KERNEL); 734 if (ret) { 735 dev_err(&vdev->dev, 736 "Failed to execute command on admin vq: %d\n.", ret); 737 goto err_cmd_exec; 738 } 739 740 status = le16_to_cpu(va_status->status); 741 if (status != VIRTIO_ADMIN_STATUS_OK) { 742 dev_err(&vdev->dev, 743 "admin command error: status(%#x) qualifier(%#x)\n", 744 status, le16_to_cpu(va_status->status_qualifier)); 745 ret = -status; 746 } 747 748 err_cmd_exec: 749 kfree(va_hdr); 750 err_alloc: 751 kfree(va_status); 752 return ret; 753 } 754
On Fri, Nov 03, 2023 at 08:33:06AM +0800, kernel test robot wrote: > Hi Yishai, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on awilliam-vfio/for-linus] > [also build test WARNING on linus/master v6.6] > [cannot apply to awilliam-vfio/next mst-vhost/linux-next next-20231102] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Yishai-Hadas/virtio-Define-feature-bit-for-administration-virtqueue/20231030-000414 > base: https://github.com/awilliam/linux-vfio.git for-linus > patch link: https://lore.kernel.org/r/20231029155952.67686-6-yishaih%40nvidia.com > patch subject: [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands > config: i386-randconfig-061-20231102 (https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp@intel.com/config) > compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202311030838.GjyaBTjM-lkp@intel.com/ > > sparse warnings: (new ones prefixed by >>) > >> drivers/virtio/virtio_pci_modern.c:726:16: sparse: sparse: restricted __le16 degrades to integer > > vim +726 drivers/virtio/virtio_pci_modern.c > > 673 > 674 static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, > 675 struct virtio_admin_cmd *cmd) > 676 { > 677 struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat; > 678 struct virtio_pci_device *vp_dev = to_vp_device(vdev); > 679 struct virtio_admin_cmd_status *va_status; > 680 unsigned int out_num = 0, in_num = 0; > 681 struct virtio_admin_cmd_hdr *va_hdr; > 682 struct virtqueue *avq; > 683 u16 status; > 684 int ret; > 685 > 686 avq = virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ) ? > 687 vp_dev->admin_vq.info.vq : NULL; > 688 if (!avq) > 689 return -EOPNOTSUPP; > 690 > 691 va_status = kzalloc(sizeof(*va_status), GFP_KERNEL); > 692 if (!va_status) > 693 return -ENOMEM; > 694 > 695 va_hdr = kzalloc(sizeof(*va_hdr), GFP_KERNEL); > 696 if (!va_hdr) { > 697 ret = -ENOMEM; > 698 goto err_alloc; > 699 } > 700 > 701 va_hdr->opcode = cmd->opcode; > 702 va_hdr->group_type = cmd->group_type; > 703 va_hdr->group_member_id = cmd->group_member_id; > 704 > 705 /* Add header */ > 706 sg_init_one(&hdr, va_hdr, sizeof(*va_hdr)); > 707 sgs[out_num] = &hdr; > 708 out_num++; > 709 > 710 if (cmd->data_sg) { > 711 sgs[out_num] = cmd->data_sg; > 712 out_num++; > 713 } > 714 > 715 /* Add return status */ > 716 sg_init_one(&stat, va_status, sizeof(*va_status)); > 717 sgs[out_num + in_num] = &stat; > 718 in_num++; > 719 > 720 if (cmd->result_sg) { > 721 sgs[out_num + in_num] = cmd->result_sg; > 722 in_num++; > 723 } > 724 > 725 if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || > > 726 cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) yes, this is broken on BE. You need to convert enums to LE before you compare. > 727 ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > 728 out_num, in_num, > 729 sgs, GFP_KERNEL); > 730 else > 731 ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, > 732 out_num, in_num, > 733 sgs, GFP_KERNEL); > 734 if (ret) { > 735 dev_err(&vdev->dev, > 736 "Failed to execute command on admin vq: %d\n.", ret); > 737 goto err_cmd_exec; > 738 } > 739 > 740 status = le16_to_cpu(va_status->status); > 741 if (status != VIRTIO_ADMIN_STATUS_OK) { > 742 dev_err(&vdev->dev, > 743 "admin command error: status(%#x) qualifier(%#x)\n", > 744 status, le16_to_cpu(va_status->status_qualifier)); > 745 ret = -status; > 746 } > 747 > 748 err_cmd_exec: > 749 kfree(va_hdr); > 750 err_alloc: > 751 kfree(va_status); > 752 return ret; > 753 } > 754 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h index a21b9ba01a60..9e07e556a51a 100644 --- a/drivers/virtio/virtio_pci_common.h +++ b/drivers/virtio/virtio_pci_common.h @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq { struct virtio_pci_vq_info info; struct completion flush_done; refcount_t refcount; + u64 supported_cmds; /* Name of the admin queue: avq.$index. */ char name[10]; u16 vq_index; diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index ccd7a4d9f57f..25e27aa79cab 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -19,6 +19,9 @@ #define VIRTIO_RING_NO_LEGACY #include "virtio_pci_common.h" +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, + struct virtio_admin_cmd *cmd); + static u64 vp_get_features(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq, bool abort) WRITE_ONCE(admin_vq->abort, abort); } +static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev) +{ + struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev); + struct virtio_admin_cmd cmd = {}; + struct scatterlist result_sg; + struct scatterlist data_sg; + __le64 *data; + int ret; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return; + + sg_init_one(&result_sg, data, sizeof(*data)); + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY); + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV); + cmd.result_sg = &result_sg; + + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); + if (ret) + goto end; + + sg_init_one(&data_sg, data, sizeof(*data)); + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE); + cmd.data_sg = &data_sg; + cmd.result_sg = NULL; + + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd); + if (ret) + goto end; + + vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data); +end: + kfree(data); +} + static void vp_modern_avq_activate(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev) if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ)) return; + virtio_pci_admin_init_cmd_list(vdev); init_completion(&admin_vq->flush_done); refcount_set(&admin_vq->refcount, 1); vp_modern_avq_set_abort(admin_vq, false); @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev, return true; } +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, + struct scatterlist **sgs, + unsigned int out_num, + unsigned int in_num, + void *data, + gfp_t gfp) +{ + struct virtqueue *vq; + int ret, len; + + vq = admin_vq->info.vq; + + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp); + if (ret < 0) + return ret; + + if (unlikely(!virtqueue_kick(vq))) + return -EIO; + + while (!virtqueue_get_buf(vq, &len) && + !virtqueue_is_broken(vq)) + cpu_relax(); + + if (virtqueue_is_broken(vq)) + return -EIO; + + return 0; +} + static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, struct scatterlist **sgs, unsigned int out_num, @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device *vdev, in_num++; } - ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, + if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY || + cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE) + ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, + out_num, in_num, + sgs, GFP_KERNEL); + else + ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs, out_num, in_num, sgs, GFP_KERNEL); if (ret) {
Initialize the supported admin commands upon activating the admin queue. The supported commands are saved as part of the admin queue context, it will be used by the next patches from this series. Note: As we don't want to let upper layers to execute admin commands before that this initialization step was completed, we set ref count to 1 only post of that flow and use a non ref counted version command for this internal flow. Signed-off-by: Yishai Hadas <yishaih@nvidia.com> --- drivers/virtio/virtio_pci_common.h | 1 + drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 1 deletion(-)