virtio-mmio: Specify wait needed in driver during reset

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

---
v3->v2:
- Introduce a new status bit, DEVICE_RESET_IN_PROGRESS
- Limit driver need to poll for reset completion only for MMIO devices having Version = 0x3

Previous version can be found at:

https://lists.oasis-open.org/archives/virtio-dev/202108/msg00065.html

 content.tex | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

content.tex +19 -4

index 7cec1c3..e10f7d3 100644
@@ -49,6 +49,8 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 
 \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
   an error from which it can't recover.
+
+\item[DEVICE_RESET_IN_PROGRESS (256)] Indicates that the device reset is in progress.
 \end{description}
 
 The \field{device status} field starts out as 0, and is reinitialized to 0 by

@@ -1730,9 +1732,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
   } 
   \hline
   \mmioreg{Version}{Device version number}{0x004}{R}{%
-    0x2.
+    0x3.
     \begin{note}
-      Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
+      Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1. Devices that do not require drivers to poll for reset completion can use 0x2. See \ref{devicenormative:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} for more details.
     \end{note}
   }
   \hline 

@@ -1916,7 +1918,15 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 
 The device MUST return 0x74726976 in \field{MagicValue}.
 
-The device MUST return value 0x2 in \field{Version}.
+The device MUST return either value 0x3 or 0x2 in \field{Version} based on its reset behavior. Drivers trigger reset
+of a device by writing 0 to \field{Status}. The reset operation itself may or may not be completed by the time write
+operation is complete. Devices whose reset operation completes synchronously with the write operation are allowed to
+return value of 0x2 for \field{Version}. Other devices, whose reset operation can be incomplete by the time write
+operation completes MUST return value 0x3 as an indication for drivers to poll for reset completion. Such devices
+MUST indicate progress of reset operation in \field{Status}, with value of DEVICE_RESET_IN_PROGRESS indicating reset
+operation is still in progress, a value of 0x0 indicating reset operation is complete and a value of
+DEVICE_NEEDS_RESET indicating device has experienced an error and that reset operation could not be completed
+successfully.
 
 The device MUST present each event by setting the corresponding bit in \field{InterruptStatus} from the
 moment it takes place, until the driver acknowledges the interrupt

@@ -1947,9 +1957,14 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 The driver MUST ignore a device with \field{MagicValue} which is not 0x74726976,
 although it MAY report an error.
 
-The driver MUST ignore a device with \field{Version} which is not 0x2,
+The driver MUST ignore a device with \field{Version} which is neither 0x2 nor 0x3,
 although it MAY report an error.
 
+When \field{Version} is 0x3, the driver, after initiating reset of device by
+writing 0 to \field{Status}, MUST wait for device reset operation to complete.
+Further, when \field{Version} is 0x3, the driver MUST not access any register other than
+reading \field{Status} while device reset is in progress.
+
 The driver MUST ignore a device with \field{DeviceID} 0x0,
 but MUST NOT report any error.
 

Signature · 3 lines

-- 
Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
non-member to the virtio-dev mailing list for consideration and inclusion.

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I am still of two minds on whether we
want such a drastic change as a version update for such a minor thing. Yes, we did it for PCI but then PCI did not break backwards compat like mmio did.

Let's see what needs to happen to make existing drivers work 1- reset starts the reset process
2- following writes into status are buffered by the device
until reset completes
3- read from features completes after reset is complete

Quoted 19 lines · Michael S. Tsirkin

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I am still of two minds on whether we
want such a drastic change as a version update for such a minor thing. Yes, we did it for PCI but then PCI did not break backwards compat like mmio did.

Quoted 5 lines

Let's see what needs to happen to make existing drivers work 1- reset starts the reset process
2- following writes into status are buffered by the device
until reset completes
3- read from features completes after reset is complete

Couple of scenarios which we discussed in this regard earlier:

1) What if device reset encounters a failure? What should it return for the 'features' read in that case?

2) For untrusted devices, like in our case [A], it would require hypervisor to stall vcpu until the untrusted backend responds to the features read request,

which could take a long time.

In worst case, the VM may get reset due to
watchdog firing. Requiring drivers to poll will avoid that situation and allow drivers to fail probe gracefully.

Ref A: https://lists.oasis-open.org/archives/virtio-dev/202108/msg00090.html

- vatsa

Signature · 7 lines

Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
non-member to the virtio-dev mailing list for consideration and inclusion.


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Quoted 27 lines · Srivatsa Vaddagiri

Quoted 24 lines · Michael S. Tsirkin

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I am still of two minds on whether we
want such a drastic change as a version update for such a minor thing. Yes, we did it for PCI but then PCI did not break backwards compat like mmio did.

Let's see what needs to happen to make existing drivers work 1- reset starts the reset process
2- following writes into status are buffered by the device
until reset completes
3- read from features completes after reset is complete

Quoted 4 lines

Couple of scenarios which we discussed in this regard earlier:

1) What if device reset encounters a failure? What should it return for the 'features' read in that case?

I'd say the main thing is to fail to set FEATURES_OK.

Quoted 2 lines

2) For untrusted devices, like in our case [A], it would require hypervisor to stall vcpu until the untrusted backend responds to the features read request,

Wait a second, this is fundamental to reads anyway. They can't bypass writes. E.g. this is the case when FEATURES_OK is written then read back.

Quoted 1 line

which could take a long time.

This last is a reasonable argument. so it's not about hardware it's for software where reset takes a long time and we do not
want to stall the VCPU. That's a reasonable requirement but pls include it in the text though.

And I wonder how you are handling
other cases where reads are ordered with writes.
Is there a reason to assume reset is special?

Quoted 1 line

Ref A: https://lists.oasis-open.org/archives/virtio-dev/202108/msg00090.html

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Srivatsa Vaddagiri

Quoted 24 lines · Michael S. Tsirkin

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I am still of two minds on whether we
want such a drastic change as a version update for such a minor thing. Yes, we did it for PCI but then PCI did not break backwards compat like mmio did.

Let's see what needs to happen to make existing drivers work 1- reset starts the reset process
2- following writes into status are buffered by the device
until reset completes
3- read from features completes after reset is complete

Quoted 6 lines

Quoted 4 lines

Couple of scenarios which we discussed in this regard earlier:

1) What if device reset encounters a failure? What should it return for the 'features' read in that case?

I'd say the main thing is to fail to set FEATURES_OK.

That would signal device does not support the features offered by driver, which seems like very hackish way to fail probe in this case (when reset had actually failed). Driver has no way to understand what happened - whether device did not like the features offered or reset failed.

Quoted 7 lines

Quoted 2 lines

2) For untrusted devices, like in our case [A], it would require hypervisor to stall vcpu until the untrusted backend responds to the features read request,

Wait a second, this is fundamental to reads anyway. They can't bypass writes. E.g. this is the case when FEATURES_OK is written then read back.

Quoted 6 lines

Quoted 1 line

which could take a long time.

This last is a reasonable argument. so it's not about hardware it's for software where reset takes a long time and we do not
want to stall the VCPU. That's a reasonable requirement but pls include it in the text though.

Sure will update commit text to indicate that better.

Quoted 2 lines

And I wonder how you are handling
other cases where reads are ordered with writes.

Quoted 1 line

Is there a reason to assume reset is special?

With what we have atm, which is working well for atleast block device, hypervisor is able to handle read/writes on its own without needing synchronous intervention from (untrusted) backend diver - except for reset that is.

Hypervisor is fed with virtio device configuration information even before VM starts and in fact in our current prototype, reads don't even trap into hypervisor. A config page is mapped as read-only into guest and so all configuration information like device id, version etc can be read w/o causing a trap. Writes are trapped and we have been able to have hypervisor handles all writes (except reset) w/o needing synchronous intervention from backend driver. For example, any write to driver features, QueueNumMax etc are cached within hypervisor which is later passed to backend driver lazily - via some hypercall interface - before it starts working on the first IO transaction.

Reset seems to be the only write that needs synchronous intervention from backend, which we want to address by having guest loop on reset completion (rather than stall vcpu until backend finishes reset handling).

Alternately, I could go by your suggestion and stall the vcpu for a certain threshold period of time when reset is issued. If backend fails to ACK reset within that threshold period, unblock vcpu and fail further initialization by refusing to set FATURES_OK bit. Like I said before, that seems very hackish way to fail probe - with no clear indication to driver on what failed.

How about merging a change like this to Linux driver w/o making any spec changes for now?

drivers/virtio/virtio_mmio.c +5 -1

index 56128b9..9db81e6 100644
@@ -262,6 +262,9 @@ static void vm_reset(struct virtio_device *vdev)
 
        /* 0 status means a reset. */
        writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+
+       while (vm_get_status(vdev)) {
+               /* Bail after some timeout */
+               msleep(1);
+       }
 }
 
- vatsa

Quoted 3 lines

Quoted 3 lines

Ref A: https://lists.oasis-open.org/archives/virtio-dev/202108/msg00090.html

--
Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a non-member to the virtio-dev mailing list for consideration and inclusion.

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Quoted 32 lines · Michael S. Tsirkin

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I have been thinking about this some more.

\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}

The driver may want to initiate a device reset at various times; notably, it is required to do so during device initialization and device cleanup.

The mechanism used by the driver to initiate the reset is transport specific.

\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}

A device MUST reinitialize \field{device status} to 0 after receiving a reset.

A device MUST NOT send notifications or interact with the queues after indicating completion of the reset by reinitializing \field{device status} to 0, until the driver re-initializes the device.

Quoted 4 lines

\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}

The driver SHOULD consider a driver-initiated reset complete when it reads \field{device status} as 0.

I wonder if we need to switch to using MUST here.

Quoted 2 lines

So I guess we can just fix the mmio driver to read status and declare victory? Do we really need all the spec changes?

Probably, but one consideration of this change is to avoid the breaking of existing devices. Maybe it's over engineering.

Quoted 33 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 11 lines · Srivatsa Vaddagiri

Reset of a virtio-mmio device is initiated by writing 0 to its Status register. In case of some devices, the reset operation itself may not be completed by the time write instruction completes and hence such devices would require drivers to wait on reset operation to complete before they proceed with remaining steps of initialization.

Update the specification to indicate which devices would need driver to block on reset completion.

Suggested-by Jason Wang <[email protected]>

Signed-off-by Srivatsa Vaddagiri <[email protected]>

I have been thinking about this some more.

\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}

The driver may want to initiate a device reset at various times; notably, it is required to do so during device initialization and device cleanup.

The mechanism used by the driver to initiate the reset is transport specific.

\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}

A device MUST reinitialize \field{device status} to 0 after receiving a reset.

A device MUST NOT send notifications or interact with the queues after indicating completion of the reset by reinitializing \field{device status} to 0, until the driver re-initializes the device.

Quoted 6 lines

Quoted 4 lines

\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}

The driver SHOULD consider a driver-initiated reset complete when it reads \field{device status} as 0.

I wonder if we need to switch to using MUST here.

I'm not quite sure what advantage s/SHOULD/MUST/ would give us here.

I don't think we want to force the driver to read back the device status to determine success if the transport already has a different way to signal completion. (I probably should undust my ccw reset clarification patch...)

Quoted 6 lines

Quoted 3 lines

So I guess we can just fix the mmio driver to read status and declare victory? Do we really need all the spec changes?

Probably, but one consideration of this change is to avoid the breaking of existing devices. Maybe it's over engineering.

How big is the chance that we use an unfixed driver with a device that would get it into trouble?

The proposed change is pretty complex, I'd
rather avoid it if possible.