Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtio.h: add some user-friendly apis #622

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CV-Bowen
Copy link
Contributor

@CV-Bowen CV-Bowen commented Oct 8, 2024

  • virtio.h: add apis virtio_read/write_config_member
Used to read/write the virtio deivces' configuration space member
  • virtio.h: add new api virtio_has_feature()
virtio_has_feature() can be easily used to heck if the virtio device
support a specific feature.
  • virtio.h: add new feature bit VIRTIO_F_ANY_LAYOUT
Follow the virtio spec, this feature bit indicates that the device
accepts arbitrary descriptor layouts.

Follow the virtio spec, this feature bit indicates that the device
accepts arbitrary descriptor layouts.

Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
@CV-Bowen CV-Bowen force-pushed the virtio-config branch 2 times, most recently from 0d9c1ce to c1646fb Compare October 8, 2024 13:41
@CV-Bowen CV-Bowen force-pushed the virtio-config branch 3 times, most recently from c2047af to 217ee1d Compare October 10, 2024 02:55
virtio_has_feature() can be easily used to heck if the virtio device
support a specific feature.

Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
@CV-Bowen
Copy link
Contributor Author

@arnopo Hi, I meet a CI issue:

#define virtio_read_config_member(vdev, structname, member, _ptr) \
	virtio_read_config((vdev), metal_offset_of(structname, member), \
			   _ptr, sizeof(*(_ptr)))

This code reports MACRO_ARG_REUSE: Macro argument reuse '_ptr' - possible side-effects?
So can we use typeof to avoid this issue?

return false;

return (vdev->features & (1UL << feature_bit)) != 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function return valid value only if virtio_negotiate_features has been called first . That also means that it does not work for virtio device role.

#define virtio_write_config_member(vdev, structname, member, _ptr) \
virtio_write_config((vdev), metal_offset_of(structname, member), \
_ptr, sizeof(*(_ptr)))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring a local variable in the macro should solve the build issue

#define virtio_write_config_member(vdev, structname, member, _ptr)  do { \
	int ptr = (_ptr); \
	virtio_write_config((vdev), metal_offset_of(structname, member), \
			    ptr, sizeof(*(ptr)));
	} while (0)

Copy link
Contributor Author

@CV-Bowen CV-Bowen Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I need use the sizeof(member), i find a new way, but have same issue :(.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I need use the sizeof(member), i find a new way, but have same issue :(.

Perhaps this could work:

#define virtio_write_config_member(vdev, structname, member, ptr) do { \
    __typeof__(structname) __tmp_structname = (structname); \
    __typeof__(member) __tmp_member = (member); \
    virtio_write_config(vdev, metal_offset_of(__tmp_structname, __tmp_member), \
                        ptr, sizeof(((__tmp_structname *)0)->__tmp_member)); \
} while (0)

but is it work for every compiler...?

@glneo concerns seems valid. This macro introduces an additional API, but it may not offer significant benefits to the library.

/*
* Read the virtio device configuration member.
*/
#define virtio_read_config_member(vdev, structname, member, _ptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a user of this new API we could take a look at? Not sure I get the use right now..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NuttX is a RTOS that we used, and we have implemented many virtio drivers in it, virtio-blk is one for them and use this API:
https://github.com/apache/nuttx/blob/master/drivers/virtio/virtio-blk.c#L584
Now these APIs are placed in NuttX but we think them should be moved to OpenAMP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. I'm trying to make sure we do not needlessly grow the OpenAMP API.

virtio_has_feature() seems like a reasonable addition to me as it abstracts access to the contents of the vdev struct (opaque structures with accessors is good design).

'virtio_{read,write}_config_member()' on the other hand looks like a thin wrapper around metal_offset_of(). Accessing structures custom to the transport layer through the virtio layer API feels like a layering issue to begin with, so I want to make sure we really want to keep building more API's on top of that..

Used to read/write the virtio deivces' configuration space member

Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants