set_status in virtio_dispatch inconsistency for remote/slave role

[diegosueiro]

We currently have in remoteproc_virtio.c:

const struct virtio_dispatch remoteproc_virtio_dispatch_funcs = {
	.get_status =  rproc_virtio_get_status,
	.get_features = rproc_virtio_get_features,
	.read_config = rproc_virtio_read_config,
	.notify = rproc_virtio_virtqueue_notify,
	.negotiate_features = rproc_virtio_negotiate_features,
#ifndef VIRTIO_SLAVE_ONLY
	/*
	 * We suppose here that the vdev is in a shared memory so that can
	 * be access only by one core: the master. In this case salve core has
	 * only read access right.
	 */
	.set_status = rproc_virtio_set_status,
	.set_features = rproc_virtio_set_features,
	.write_config = rproc_virtio_write_config,
	.reset_device = rproc_virtio_reset_device,
#endif
};

If compiling for SLAVE only the .set_status callback is not defined, but in rpmsg_virtio.c we can end up calling it (via rpmsg_virtio_set_status function) in the rpmsg_virtio_wait_remote_ready function:

#ifndef VIRTIO_MASTER_ONLY
/**     
 * check if the remote is ready to start RPMsg communication
 */                  
static int rpmsg_virtio_wait_remote_ready(struct rpmsg_virtio_device *rvdev)
{
        uint8_t status;
        
        while (1) {
                status = rpmsg_virtio_get_status(rvdev);
                /* Busy wait until the remote is ready */
                if (status & VIRTIO_CONFIG_STATUS_NEEDS_RESET) {
                        rpmsg_virtio_set_status(rvdev, 0);
                        /* TODO notify remote processor */
                } else if (status & VIRTIO_CONFIG_STATUS_DRIVER_OK) {
                        return true;
                }
                /* TODO: clarify metal_cpu_yield usage*/
                metal_cpu_yield();
        }
        
        return false;
}
#endif /*!VIRTIO_MASTER_ONLY*/

[arnopo]

Hello Diego,

Thanks to point the issue. You are right this not properly handled.

Wendy,
seems that
if (status & VIRTIO_CONFIG_STATUS_NEEDS_RESET) {
rpmsg_virtio_set_status(rvdev, 0);
/* TODO notify remote processor */
}
Should be under #ifndef VIRTIO_SLAVE_ONLY, as only master should set the status bits...
Could you confirm ?
Anyway do you use (or know if someone use) this feature)? I'm not sure to understand how it should work...
As master sets the VIRTIO_CONFIG_STATUS_NEEDS_RESET status, how could we ensure that remote processor gets the status and release its resources in time, before master reset the link? There is no ack from the remote processor...
Regards
arnaud

[wjliang]

HI Arnaud, Diego

Based on the virtio specification (http://docs.oasis-open.org/virtio/virtio/v1.0/virtio-v1.0.html), the virtio_config_status_needs_reset can be set by slave side to require master to reset the slave.

The code in rpmsg_virtio.c implementation to check this flag in slave was because there was a case that master restarts, but there is no way for master to notify slave, and thus, it reuse this status bit as a work around. When the slave sees this bit, it resets the status bit to 0 (to indicate the slave knows master restarted). The master monitors the status bit, it will not reinitialize the virtio rings until it sees slave clear the status. This workaround is not very proper. And what I have described about the master behaviour only exists in a customized Linux kernel. The code is "left over" from the effort when we did restructuring for the v2018.10 release.

I think we can:

• remove the "if (status & VIRTIO_CONFIG_STATUS_NEEDS_RESET) {" checking for virtio slave.
• Will need to raise this issue to the resource table to describe the issue, because today, both slave and master are using the same u8 for status, when master/slave set the status, it will overwrite the other's status bit. for the shared memory based virtio config space, we should have master_status and slave status separate. We have talked this before with Linux kernel maintainer, but no conclusion yet.
• Raise the virtio gap (for the virtio master/slave to notify the other end that it restarted) in virtio specification for a community acceptable solution.

Best Regards,
Wendy

[diegosueiro]

@wjliang,

Unfortunately, I'm not very familiar with the protocol specifications, so I do not know if I can comment on this.

I'd suggest for now to include the .set_status for SLAVE.

[arnopo]

Hi Wendy, Diego

On 11/21/18 7:28 PM, wjliang wrote: HI Arnaud, Diego Based on the virtio specification (http://docs.oasis-open.org/virtio/virtio/v1.0/virtio-v1.0.html), the virtio_config_status_needs_reset can be set by slave side to require master to reset the slave.

Please could you point out the chapter please? I can not see specific description about a master and a slave status management, only device and driver access description...

The code in rpmsg_virtio.c implementation to check this flag in slave was because there was a case that master restarts, but there is no way for master to notify slave, and thus, it reuse this status bit as a work around. When the slave sees this bit, it resets the status bit to 0 (to indicate the slave knows master restarted). The master monitors the status bit, it will not reinitialize the virtio rings until it sees slave clear the status. This workaround is not very proper. And what I have described about the master behaviour only exists in a customized Linux kernel. The code is "left over" from the effort when we did restructuring for the v2018.10 release. I think we can: * remove the "if (status & VIRTIO_CONFIG_STATUS_NEEDS_RESET) {" checking for virtio slave.

aligned with this solution for short term. proposed patch (not tested): @@ -221,14 +221,18 @@ static int rpmsg_virtio_wait_remote_ready(struct rpmsg_virtio_device *rvdev) uint8_t status; while (1) { status = rpmsg_virtio_get_status(rvdev); /* Busy wait until the remote is ready */ +#ifndef VIRTIO_SLAVE_ONLY if (status & VIRTIO_CONFIG_STATUS_NEEDS_RESET) { rpmsg_virtio_set_status(rvdev, 0); /* TODO notify remote processor */ - } else if (status & VIRTIO_CONFIG_STATUS_DRIVER_OK) { + continue; + } +#endif /*!VIRTIO_MASTER_ONLY*/ + if (status & VIRTIO_CONFIG_STATUS_DRIVER_OK) { return true; } /* TODO: clarify metal_cpu_yield usage*/ metal_cpu_yield(); }

* Will need to raise this issue to the resource table to describe the issue, because today, both slave and master are using the same u8 for status, when master/slave set the status, it will overwrite the other's status bit. for the shared memory based virtio config space, we should have master_status and slave status separate. We have talked this before with Linux kernel maintainer, but no conclusion yet.

For time being we have to ensure that only master update the status field. Reset request could be handled by remote proc based on mailbox or other signals, or by a specific rpmsg service. Then agree that a remote proc status field could be useful, to inform master about remote proc state. Regards Arnaud

…

* Raise the virtio gap (for the virtio master/slave to notify the other end that it restarted) in virtio specification for a community acceptable solution. Best Regards, Wendy — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYTZgwoXHPMT_bIMier7B0zXpW8aXC9eks5uxZtJgaJpZM4Ysg-m>.

[wjliang]

@wjliang,

Unfortunately, I'm not very familiar with the protocol specifications, so I do not know if I can comment on this.

I'd suggest for now to include the .set_status for SLAVE.

Hi Diego,
set_status can be available to SLAVE, but just in the remoteproc virtio driver, as it is based on resource table shared memory. The same byte used for both master and slave, I prefer not doing it in the upstream. if you want to use it, you can implement a different virtio driver to use a other virtio config space rather than the existing resource table. But if you just want a workaround, and you cannot see any issue by both sides using the same status byte in your case, you can enable .set_status for SLAVE in your branch.

Best Regards,
Wendy

[wjliang]

For time being we have to ensure that only master update the status field. Reset request could be handled by remote proc based on mailbox or other signals, or by a specific rpmsg service. Then agree that a remote proc status field could be useful, to inform master about remote proc state.

Reset request is only available for non-crash case, for crashing case, you cannot rely on the reset request.
That's why we previously have the virtio to send the notification.
Agree to allow remoteproc to have function to notify local about the remote status change, but maybe it should not rely on resource table, as resource table is optional.

[wjliang]

Please could you point out the chapter please? I can not see specific description about a master and a slave status management, only device and driver access description...

Chapter "2.1 Device Status Field"

[diegosueiro]

@wjliang,
Unfortunately, I'm not very familiar with the protocol specifications, so I do not know if I can comment on this.
I'd suggest for now to include the .set_status for SLAVE.

Hi Diego,
set_status can be available to SLAVE, but just in the remoteproc virtio driver, as it is based on resource table shared memory. The same byte used for both master and slave, I prefer not doing it in the upstream. if you want to use it, you can implement a different virtio driver to use a other virtio config space rather than the existing resource table. But if you just want a workaround, and you cannot see any issue by both sides using the same status byte in your case, you can enable .set_status for SLAVE in your branch.

But Wendy, currently even with or without the remoteproc framework if you compile the OpenAMP with -DVIRTIO_SLAVE_ONLY, the .set_status member on remoteproc_virtio_dispatch_funcs struct doesn't exist and if it gets called we will have problems.

What I don't understand is why the ".set_status" is removed from -DVIRTIO_SLAVE_ONLY.

[wjliang]

But Wendy, currently even with or without the remoteproc framework if you compile the OpenAMP with -DVIRTIO_SLAVE_ONLY, the .set_status member on remoteproc_virtio_dispatch_funcs struct doesn't exist and if it gets called we will have problems.

What I don't understand is why the ".set_status" is removed from -DVIRTIO_SLAVE_ONLY.

I think it is OK to have a dummy implementation, as it matches the "not VIRTIO_SLAVE_ONLY" case.
Here is just what I am thinking, as if we put dummy implementation to .set_status, it will do nothing, and the remote will not get any update. In this case, maybe let it fail can enforce a proper implementation instead of a dummy one when it is called in this use case.
I think we need to have more comments, to the function, and also in the VIRTIO_SLAVE_ONLY not defined case, it should check if the role is master before it set the status.