Re: [PATCH] remoteproc: remoteproc_virtio: add acknowledged vdev reset

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, Sep 08, 2026 at 02:21:53PM -0500, Shah, Tanmay wrote:
> Hello,
> 
> Thank you for the reviews.
> 
> On 9/8/2026 1:02 PM, Mathieu Poirier wrote:
> > Good day,
> > 
> > On Wed, Sep 02, 2026 at 02:44:54PM -0700, Tanmay Shah wrote:
> >> The existing remoteproc virtio reset path clears the vdev status locally
> >> without notifying the remote processor. As a result, the host cannot tell
> >> whether the remote side has observed the reset request or completed its
> >> cleanup.
> >>
> >> Add a new resource type, RSC_VDEV_V2, for virtio vdevs that support an
> >> acknowledged reset protocol. For these resources, encode a reset request
> >> in the virtio status byte, kick the remote processor using the vdev notify
> >> ID, and wait for the remote side to clear the status back to 0.
> >>
> >> Keep the existing RSC_VDEV behavior for backwards compatibility by
> >> clearing the status locally. Also reset remoteproc-created virtio
> >> devices before unregistering them, and expose RSC_VDEV_V2 reset state
> >> in debugfs.
> >>
> >> Assisted-by: Codex:GPT-5
> >> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c     |  3 +-
> >>  drivers/remoteproc/remoteproc_debugfs.c  | 29 +++++++++++++++-
> >>  drivers/remoteproc/remoteproc_internal.h | 21 +++++++++++
> >>  drivers/remoteproc/remoteproc_virtio.c   | 44 ++++++++++++++++++++++--
> >>  include/linux/rsc_table.h                |  5 ++-
> >>  5 files changed, 97 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index 1ed406714849..31d79684977c 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -471,6 +471,7 @@ void rproc_remove_rvdev(struct rproc_vdev *rvdev)
> >>  static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> >>  			     int offset, int avail)
> >>  {
> >> +	struct fw_rsc_hdr *hdr = ptr - sizeof(*hdr);
> > 
> > Spurious change.
> > 
> 
> Ack will remove it.
> 
> >>  	struct fw_rsc_vdev *rsc = ptr;
> >>  	struct device *dev = &rproc->dev;
> >>  	struct rproc_vdev *rvdev;
> >> @@ -485,7 +486,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	/* make sure reserved bytes are zeroes */
> > 
> > Same
> 
> Ack, will be removed.
> 
> > 
> >>  	if (rsc->reserved[0] || rsc->reserved[1]) {
> >>  		dev_err(dev, "vdev rsc has non zero reserved bytes\n");
> >>  		return -EINVAL;
> >> @@ -1009,6 +1009,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
> >>  	[RSC_DEVMEM] = rproc_handle_devmem,
> >>  	[RSC_TRACE] = rproc_handle_trace,
> >>  	[RSC_VDEV] = rproc_handle_vdev,
> >> +	[RSC_VDEV_V2] = rproc_handle_vdev,
> >>  };
> >>  
> >>  struct rproc_rsc_cb_data {
> >> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> >> index b86c1d09c70c..1fe99749f5b4 100644
> >> --- a/drivers/remoteproc/remoteproc_debugfs.c
> >> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> >> @@ -274,7 +274,7 @@ static const struct file_operations rproc_crash_ops = {
> >>  /* Expose resource table content via debugfs */
> >>  static int rproc_rsc_table_show(struct seq_file *seq, void *p)
> >>  {
> >> -	static const char * const types[] = {"carveout", "devmem", "trace", "vdev"};
> >> +	static const char * const types[] = {"carveout", "devmem", "trace", "vdev", "vdev_v2"};
> >>  	struct rproc *rproc = seq->private;
> >>  	struct resource_table *table = rproc->table_ptr;
> >>  	struct fw_rsc_carveout *c;
> >> @@ -336,6 +336,33 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
> >>  			seq_printf(seq, "  Reserved (should be zero) [%d][%d]\n\n",
> >>  				   v->reserved[0], v->reserved[1]);
> >>  
> >> +			for (j = 0; j < v->num_of_vrings; j++) {
> >> +				seq_printf(seq, "  Vring %d\n", j);
> >> +				seq_printf(seq, "    Device Address 0x%x\n", v->vring[j].da);
> >> +				seq_printf(seq, "    Alignment %d\n", v->vring[j].align);
> >> +				seq_printf(seq, "    Number of buffers %d\n", v->vring[j].num);
> >> +				seq_printf(seq, "    Notify ID %d\n", v->vring[j].notifyid);
> >> +				seq_printf(seq, "    Physical Address 0x%x\n\n",
> >> +					   v->vring[j].pa);
> >> +			}
> >> +			break;
> >> +		case RSC_VDEV_V2:
> >> +			v = rsc;
> >> +			seq_printf(seq, "Entry %d is of type %s\n", i, types[hdr->type]);
> >> +
> >> +			seq_printf(seq, "  ID %d\n", v->id);
> >> +			seq_printf(seq, "  Notify ID %d\n", v->notifyid);
> >> +			seq_printf(seq, "  Device features 0x%x\n", v->dfeatures);
> >> +			seq_printf(seq, "  Guest features 0x%x\n", v->gfeatures);
> >> +			seq_printf(seq, "  Config length 0x%x\n", v->config_len);
> >> +			seq_printf(seq, "  Status 0x%x\n", v->status);
> >> +			seq_printf(seq, "  Number of vrings %d\n", v->num_of_vrings);
> >> +			seq_printf(seq, "  Reset request pending %s\n",
> >> +				   rproc_rsc_vdev_reset_requested(v->status) ?
> >> +				   "yes" : "no");
> >> +			seq_printf(seq, "  Reserved (should be zero) [%d][%d]\n\n",
> >> +				   v->reserved[0], v->reserved[1]);
> >> +
> >>  			for (j = 0; j < v->num_of_vrings; j++) {
> >>  				seq_printf(seq, "  Vring %d\n", j);
> >>  				seq_printf(seq, "    Device Address 0x%x\n", v->vring[j].da);
> >> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> >> index 3a742ef6ef60..f07a96ff82a4 100644
> >> --- a/drivers/remoteproc/remoteproc_internal.h
> >> +++ b/drivers/remoteproc/remoteproc_internal.h
> >> @@ -14,6 +14,7 @@
> >>  
> >>  #include <linux/irqreturn.h>
> >>  #include <linux/firmware.h>
> >> +#include <linux/virtio_config.h>
> >>  #ifdef CONFIG_HAS_IOMEM
> >>  #include <linux/io.h>
> >>  #endif
> >> @@ -42,6 +43,26 @@ struct rproc_vdev_data {
> >>  	struct fw_rsc_vdev *rsc;
> >>  };
> >>  
> >> +/*
> >> + * RSC_VDEV_V2 requests an acknowledged reset by writing an otherwise
> >> + * impossible virtio status pattern: DRIVER and FAILED set while
> >> + * ACKNOWLEDGE is clear. Other status bits are left unchanged.
> >> + */
> >> +static inline u8 rproc_rsc_vdev_reset_status(u8 status)
> >> +{
> >> +	status |= VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_FAILED;
> >> +	status &= ~VIRTIO_CONFIG_S_ACKNOWLEDGE;
> >> +
> >> +	return status;
> >> +}
> >> +
> >> +static inline bool rproc_rsc_vdev_reset_requested(u8 status)
> >> +{
> >> +	return !(status & VIRTIO_CONFIG_S_ACKNOWLEDGE) &&
> >> +	       (status & VIRTIO_CONFIG_S_DRIVER) &&
> >> +	       (status & VIRTIO_CONFIG_S_FAILED);
> >> +}
> >> +
> >>  static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature)
> >>  {
> >>  	return test_bit(feature, rproc->features);
> >> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> >> index d5e9ff045a28..e682caa546b2 100644
> >> --- a/drivers/remoteproc/remoteproc_virtio.c
> >> +++ b/drivers/remoteproc/remoteproc_virtio.c
> >> @@ -13,6 +13,7 @@
> >>  #include <linux/dma-map-ops.h>
> >>  #include <linux/dma-mapping.h>
> >>  #include <linux/export.h>
> >> +#include <linux/iopoll.h>
> >>  #include <linux/of_reserved_mem.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/remoteproc.h>
> >> @@ -234,12 +235,48 @@ static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
> >>  static void rproc_virtio_reset(struct virtio_device *vdev)
> >>  {
> >>  	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> >> +	struct rproc *rproc = rvdev->rproc;
> >>  	struct fw_rsc_vdev *rsc;
> >> +	struct fw_rsc_hdr *hdr;
> >> +	int ret;
> >> +	u8 val;
> >> +
> >> +	/*
> >> +	 * During crash recovery, vdev can be stopped. But the driver can't reset
> >> +	 * the device, as device is already crashed. In this case, reset becomes
> >> +	 * no op.
> >> +	 */
> >> +	if (rproc->state == RPROC_CRASHED)
> >> +		return;
> >>  
> >>  	rsc = (void *)rvdev->rproc->table_ptr + rvdev->rsc_offset;
> >> +	hdr = (void *)rsc - sizeof(*hdr);
> >> +
> >> +	if (hdr->type == RSC_VDEV_V2) {
> >> +		/*
> >> +		 * RSC_VDEV_V2 encodes an acknowledged reset request in the
> >> +		 * status byte. The remote is expected to complete the reset
> >> +		 * and then clear status back to 0.
> >> +		 */
> >> +		rsc->status = rproc_rsc_vdev_reset_status(rsc->status);
> >> +
> >> +		/* after setting reset request, kick the device */
> >> +		rproc->ops->kick(rproc, rsc->notifyid);
> >>  
> >> -	rsc->status = 0;
> >> -	dev_dbg(&vdev->dev, "reset !\n");
> >> +		/*
> >> +		 * When device completes reset, it is expected to set status
> >> +		 * to 0.
> >> +		 */
> >> +		ret = readb_poll_timeout(&rsc->status, val, val == 0,
> >> +					 1000,      /* 1ms between reads */
> >> +					 3000000);  /* 3s total timeout */
> >> +		if (ret)
> >> +			dev_warn(&vdev->dev, "vdev reset timed out\n");
> > 
> > The problem here is that we are introducing behavior that is not compliant with
> > the virtio specifications.  One way to acheive the same behavior could be for
> > the remote processor to check rsc->status before sending a interrupt of using
> > the virtqueues.
> > 
> 
> That is what remote is supposed to do. But what if remote do not
> respond? If remote is deadlocked for some reason, then the Linux will
> hang at this point too. That is why we need some kind of timeout.

If the remote is dead then a watchdog timer should fire at some point.
Moreover, that situation won't be different from other circumstances where a
remote processor locks up.

Looking at your patch, sending a kick() won't do anything for a dead remote
processor.  If the remote processor is alive, it should monitor rsc->status and
take action when it is set to '0' by the host.  If it is locked-up, the normal
lockup procedure should apply.

I'm not sure what problem this patch is trying to address.

> 
> I think timeout mechanism is better for AMP systems over waiting forever
> for remote to clear the status.
> 
> Thanks,
> Tanmay
> 
> 
> >> +	} else {
> >> +		/* back compatible for RSC_VDEV type of rsc vdev */
> >> +		rsc->status = 0;
> >> +	}
> >> +	dev_info(&vdev->dev, "reset !\n");
> >>  }
> >>  
> >>  /* provide the vdev features as retrieved from the firmware */
> >> @@ -469,6 +506,9 @@ static int rproc_remove_virtio_dev(struct device *dev, void *data)
> >>  {
> >>  	struct virtio_device *vdev = dev_to_virtio(dev);
> >>  
> >> +	/* reset virtio device before unregister */
> >> +	virtio_reset_device(vdev);
> >> +
> > 
> > Regardless of this feature, I think it is wise to reset the device before
> > unregistering with the virtio subsystem.
> > 
> 
> Agreed. I intend to keep this.
> 
> > Thanks,
> > Mathieu 
> > 
> >>  	unregister_virtio_device(vdev);
> >>  	return 0;
> >>  }
> >> diff --git a/include/linux/rsc_table.h b/include/linux/rsc_table.h
> >> index 71b60125310e..2398a6d7033e 100644
> >> --- a/include/linux/rsc_table.h
> >> +++ b/include/linux/rsc_table.h
> >> @@ -66,6 +66,8 @@ struct fw_rsc_hdr {
> >>   *		    the remote processor will be writing logs.
> >>   * @RSC_VDEV:       declare support for a virtio device, and serve as its
> >>   *		    virtio header.
> >> + * @RSC_VDEV_V2:    declare support for a virtio device whose reset request is
> >> + *		    encoded in the virtio status byte.
> >>   * @RSC_LAST:       just keep this one at the end of standard resources
> >>   * @RSC_VENDOR_START:	start of the vendor specific resource types range
> >>   * @RSC_VENDOR_END:	end of the vendor specific resource types range
> >> @@ -83,7 +85,8 @@ enum fw_resource_type {
> >>  	RSC_DEVMEM		= 1,
> >>  	RSC_TRACE		= 2,
> >>  	RSC_VDEV		= 3,
> >> -	RSC_LAST		= 4,
> >> +	RSC_VDEV_V2		= 4,
> >> +	RSC_LAST		= 5,
> >>  	RSC_VENDOR_START	= 128,
> >>  	RSC_VENDOR_END		= 512,
> >>  };
> >>
> >> base-commit: d4d61a4b0a52e8f3cdb3e1578602850a3452ec3e
> >> -- 
> >> 2.43.0
> >>
>