virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch #495

CV-Bowen · 2023-07-03T11:35:05Z

This is the first step to decoupling the virtio deivce and transport layer.

Related issue: #390 (comment)

@arnopo Could take a look?

arnopo

copy past issue in virtio.h + "deivce" to fix in commit message

lib/include/openamp/virtio.h

arnopo · 2023-07-03T17:03:40Z

copy past issue in virtio.h + "deivce" to fix in commit message

you still need to fix the commit message :

./scripts/checkpatch.pl --strict -g HEAD
ec793415d92ab97232bdd32aba5b2c9845c2b5f0:7: WARNING:TYPO_SPELLING: 'deivce' may be misspelled - perhaps 'device'?
#7: 
This is the first step to decoupling the virtio deivce and transport
                                                ^^^^^^

ec793415d92ab97232bdd32aba5b2c9845c2b5f0 total: 0 errors, 1 warnings, 0 checks, 155 lines checked

CV-Bowen · 2023-07-04T01:46:45Z

@arnopo Done.

uLipe

Just a small question.

uLipe · 2023-07-05T19:59:13Z

lib/include/openamp/virtio.h

+ */
+static inline void virtio_delete_virtqueues(struct virtio_device *vdev)
+{
+	return vdev->func->delete_virtqueues(vdev);


Question, is guaranteed the func will be always valid here? It would not be good to check for availabillity of this function before calling, also making this way will obligate the implementor of the dispatch functions to provide this function, is this intended?

Yes the point is valid. Vdev , 'vdev->func' and 'vdev->func->delete_virtqueues' should be checked.
@CV-Bowen: It is acceptable for me to do it in a second step as here the main objective is to create the API. It can also be done by updating the PR.

@arnopo @uLipe OK, I will update this PR, another question is should I add this check in all the new virtio_xxx() apis?

Done.

The @uLipe comment is valid for all the func. with a return on "-ENXIO" when requestion a return value.

@arnopo So I change all the virtio_xxx functions return type to int and call dispatch function in these functions ?
Like:

static inline int virtio_set_status(struct virtio_device *vdev, uint8_t status) { if (!vdev->func->set_status) return -ENXIO; vdev->func->set_status(vdev, status); return 0; }

@CV-Bowen , yes this is the intention, the dispatch functions have a return type, when it is present just return the result of the dispatch functions, taking the example above slightly modified:

static inline int virtio_set_status(struct virtio_device *vdev, uint8_t status) { if (!vdev->func->set_status) return -ENXIO; else return vdev->func->set_status(vdev, status); }

The idea is all the dispatch function templates have an return value.

@uLipe Thanks, I found the vdev->func->set_status return value change to int (void for now) in your example code, should I change all the dispatch functions return type to int? This is a significant change.

static inline int virtio_set_status(struct virtio_device *vdev, uint8_t status)
{
if (!vdev->func->set_status)
return -ENXIO;
else
return vdev->func->set_status(vdev, status);
}

arnopo · 2023-07-06T07:45:23Z

@danmilea
Could you have a look to this PR to confirm that it compatible ( or not) with your implementation?
I would like to have your feedback before merging it.

arnopo

few suggestions to increase the APi robustness, else LGTM

arnopo · 2023-07-11T09:49:58Z

lib/virtio/virtio.c

+	if (!vdev->func)
+		return -ENXIO;
+
+	if (vdev->func->create_virtqueues) {


to avoid regression on existing code
remove

if (!vdev->func) return -ENXIO;

instead use if (vdev-> func && vdev->func->create_virtqueues) {

Please could you also add

if (!vdev || !src) return -EINVAL;

at the beginning of the function?

arnopo · 2023-07-11T09:51:11Z

lib/include/openamp/virtio.h

+ */
+static inline int virtio_get_status(struct virtio_device *vdev, uint8_t *status)
+{
+	if (!status)


if (!vdev || !status)

lib/include/openamp/virtio.h

arnopo · 2023-07-11T09:54:26Z

lib/include/openamp/virtio.h

+static inline int virtio_get_features(struct virtio_device *vdev,
+				      uint32_t *features)
+{
+	if (!features)


if (!vdev || !features)

lib/include/openamp/virtio.h

arnopo · 2023-07-11T09:55:07Z

lib/include/openamp/virtio.h

+					    uint32_t features,
+					    uint32_t *final_features)
+{
+	if (!final_features)


if (!vdev || !final_features)

lib/include/openamp/virtio.h

This is the first step to decoupling the virtio device and transport layer. Signed-off-by: Bowen Wang <[email protected]>

arnopo

LGTM

danmilea

Good to merge.

arnopo · 2023-07-12T08:58:42Z

Merged, thanks for the contribution!

CV-Bowen mentioned this pull request Jul 3, 2023

remoteproc_virtio: optimize the remoteproc virtio transport layer #489

Merged

arnopo requested review from danmilea, edmooring, xiaoxiang781216, wmamills and tnmysh July 3, 2023 12:59

CV-Bowen force-pushed the virtio-decouple-1 branch from 7394743 to 2f4ef16 Compare July 3, 2023 15:21

arnopo mentioned this pull request Jul 3, 2023

Fix potential functions redefinition for IPC subsystem and example zephyrproject-rtos/zephyr#59964

Merged

arnopo requested changes Jul 3, 2023

View reviewed changes

lib/include/openamp/virtio.h Outdated Show resolved Hide resolved

CV-Bowen force-pushed the virtio-decouple-1 branch from 2f4ef16 to d49315b Compare July 3, 2023 16:53

xiaoxiang781216 approved these changes Jul 4, 2023

View reviewed changes

CV-Bowen force-pushed the virtio-decouple-1 branch from d49315b to 668df7e Compare July 4, 2023 01:46

arnopo approved these changes Jul 4, 2023

View reviewed changes

arnopo added this to the Release V2023.10 milestone Jul 4, 2023

uLipe reviewed Jul 5, 2023

View reviewed changes

CV-Bowen force-pushed the virtio-decouple-1 branch 3 times, most recently from 375fc2c to d70af8b Compare July 10, 2023 15:55

arnopo reviewed Jul 11, 2023

View reviewed changes

virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch

64ef3d9

This is the first step to decoupling the virtio device and transport layer. Signed-off-by: Bowen Wang <[email protected]>

CV-Bowen force-pushed the virtio-decouple-1 branch from d70af8b to 64ef3d9 Compare July 11, 2023 13:36

uLipe approved these changes Jul 11, 2023

View reviewed changes

arnopo self-requested a review July 11, 2023 14:28

arnopo approved these changes Jul 11, 2023

View reviewed changes

tnmysh approved these changes Jul 11, 2023

View reviewed changes

danmilea approved these changes Jul 12, 2023

View reviewed changes

arnopo merged commit 7f90610 into OpenAMP:main Jul 12, 2023
2 checks passed

arnopo mentioned this pull request Jul 12, 2023

what's plan to support the service/driver at the virtio level? #390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch #495

virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch #495

CV-Bowen commented Jul 3, 2023 •

edited

Loading

arnopo left a comment

arnopo commented Jul 3, 2023

CV-Bowen commented Jul 4, 2023

uLipe left a comment

uLipe Jul 5, 2023

arnopo Jul 6, 2023

CV-Bowen Jul 6, 2023

CV-Bowen Jul 9, 2023

arnopo Jul 10, 2023

CV-Bowen Jul 10, 2023 •

edited

Loading

uLipe Jul 10, 2023

CV-Bowen Jul 10, 2023

arnopo commented Jul 6, 2023

arnopo left a comment

arnopo Jul 11, 2023

CV-Bowen Jul 11, 2023

arnopo Jul 11, 2023

CV-Bowen Jul 11, 2023

arnopo Jul 11, 2023

CV-Bowen Jul 11, 2023

arnopo Jul 11, 2023

CV-Bowen Jul 11, 2023

arnopo left a comment

danmilea left a comment

arnopo commented Jul 12, 2023

virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch #495

virtio: add create_virtqueues and delete_virtqueues in virtio_dispatch #495

Conversation

CV-Bowen commented Jul 3, 2023 • edited Loading

arnopo left a comment

Choose a reason for hiding this comment

arnopo commented Jul 3, 2023

CV-Bowen commented Jul 4, 2023

uLipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CV-Bowen Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo commented Jul 6, 2023

arnopo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

danmilea left a comment

Choose a reason for hiding this comment

arnopo commented Jul 12, 2023

CV-Bowen commented Jul 3, 2023 •

edited

Loading

CV-Bowen Jul 10, 2023 •

edited

Loading