-
Notifications
You must be signed in to change notification settings - Fork 288
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
remoteproc: do cache invalidation before reading rsc_table status #500
remoteproc: do cache invalidation before reading rsc_table status #500
Conversation
146f72b
to
ddf6fb1
Compare
ddf6fb1
to
a2b48ce
Compare
Changes since v1:
|
a2b48ce
to
125b19d
Compare
lib/remoteproc/remoteproc_virtio.c
Outdated
@@ -40,6 +41,11 @@ static unsigned char rproc_virtio_get_status(struct virtio_device *vdev) | |||
rpvdev = metal_container_of(vdev, struct remoteproc_virtio, vdev); | |||
vdev_rsc = rpvdev->vdev_rsc; | |||
io = rpvdev->vdev_rsc_io; | |||
|
|||
#ifdef VIRTIO_CACHED_RSC_TABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move #ifdef part to relevant header file ?
May be we can introduce function to check if cache is on or not and then based on that we can decide if we need to call metal_cache_invalidate or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, in OpenAMP there are 2 ways of doing cache invalidation:
- rpmsg: https://github.com/OpenAMP/open-amp/blob/main/lib/rpmsg/rpmsg_virtio.c#L138
- virtqueue: https://github.com/OpenAMP/open-amp/blob/main/lib/virtio/virtqueue.c#L32
I've chosen the first one.
You're suggesting the second one - just to put the defines in a header, not in .c file, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be we can introduce function to check if cache is on or not and then based on that we can decide if we need to call metal_cache_invalidate or not.
That's way too wasteful IMO, you are going to have a huge hit in performance if for every cache op you are going to check whether the cache is on or not.
To make this nicer we can do what we did here:
open-amp/lib/virtio/virtqueue.c
Lines 32 to 38 in 7f90610
#ifdef VIRTIO_CACHED_VRINGS | |
#define VRING_FLUSH(x) metal_cache_flush(&x, sizeof(x)) | |
#define VRING_INVALIDATE(x) metal_cache_invalidate(&x, sizeof(x)) | |
#else | |
#define VRING_FLUSH(x) do { } while (0) | |
#define VRING_INVALIDATE(x) do { } while (0) | |
#endif /* VIRTIO_CACHED_VRINGS */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlocaione yes, will do like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlocaione yes, will do like this.
bonus point if you can clean up these two ways of doing the same thing 😜 (in a different PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Yes second way works.
125b19d
to
364ebc5
Compare
Changes since v2:
LE: the checkpatch issue is a false-positive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but looking forward to the cleanup PR.
Please also update https://github.com/OpenAMP/open-amp/blob/main/README.md |
364ebc5
to
45c9ec4
Compare
Changes since v3:
|
#define RSC_TABLE_FLUSH(x) \ | ||
({void *_x = (x); \ | ||
metal_cache_flush(&_x, sizeof(_x)); }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is doing what you think it is doing? Looking at it it looks like you are flushing the pointer here, not the actual struct also with a wrong sizeof()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I've fixed it.
Also, to pass checkpatch I've added 2 arguments for RSC_TABLE_FLUSH and RSC_TABLE_INVALIDATE
cd05c20
to
3f1dd55
Compare
Changes since v4:
|
3f1dd55
to
4614bdf
Compare
Changes since v5:
|
lib/remoteproc/remoteproc_virtio.c
Outdated
|
||
RSC_TABLE_INVALIDATE(vdev_rsc, sizeof(struct fw_rsc_vdev)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine now, thanks.
Shouldn't we also RSC_TABLE_FLUSH
in rproc_virtio_set_status()
though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added RSC_TABLE_FLUSH
/RSC_TABLE_INVALIDATE
for status
, dfeatures
and gfeatures
.
I don't know if I should do the same for read_config
/write_config
.
What do you think?
For Zephyr, config
is not used: https://github.com/zephyrproject-rtos/zephyr/blob/main/lib/open-amp/resource_table.h#L50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arnopo I'm not familiar with remoteproc at all. What are the structs that are actually r/w from both sides that needs to be flushed / invalidated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need cache operation even in read config and write config as well.
One more question, should we perform only flush or invalidate and flush both during writing resource table? Or it is really platform dependent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Flush" and "invalidate" should be sufficient if the structures are aligned on a cache line. We can improve this in the future if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right @TanmayShah-xilinx,
The cache shoule also be managed in rproc_virtio_read_config
andrproc_virtio_read_config
4614bdf
to
ecd7ee3
Compare
ecd7ee3
to
9c5537d
Compare
Changes since v6:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this LGTM. I'm a bit concerned about the alignment of these structs. If the structs are not aligned to the cache line, the invalidation will corrupt neighboring structs in memory. It really is a silent killer.
See for example zephyrproject-rtos/zephyr#59456
lib/remoteproc/remoteproc_virtio.c
Outdated
|
||
RSC_TABLE_INVALIDATE(vdev_rsc, sizeof(struct fw_rsc_vdev)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you keep the newlines consistent among all the RSC_TABLE_*
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Do a cache invalidation before reading the resource table's info since this ca be in a cacheable region. Make this optional, based on VIRTIO_CACHED_RSC_TABLE. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
Add info for WITH_DCACHE_RSC_TABLE option. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
9c5537d
to
25ffebe
Compare
Changes since v7: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Do a cache invalidation before reading the resource table's status since this ca be in a cacheable region.