[PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

Reset of a virtio-mmio device is initiated by writing 0 to its Status
register. The reset operation itself may or may not be completed by the time
write instruction completes. For devices that are emulated in software, writes
to Status register are trapped and resumed only after the reset operation is
complete. Thus a driver can be assured of reset completion as soon as its write
completes. That may not be however true in other cases, such as when virtio-mmio
devices implemented directly in hardware. Driver's write to Status register in
such case could complete before the device reset operation is completed.

Update the specification to indicate when a driver may need to poll for reset
completion before going ahead with the remaining initialization steps.
    
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>

---
V2->V1:
- Use version 0x3 as an indication for drivers to poll for reset completion
  (rather than base it on a new feature bit)

Previous version can be found at:

https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html

 content.tex | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/content.tex b/content.tex
index 7cec1c3..b6b30a0 100644
--- a/content.tex
+++ b/content.tex
@@ -1730,9 +1730,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 +1916,7 @@ \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.
 
 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,12 +1947,15 @@ \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.
 
 The driver MUST ignore a device with \field{DeviceID} 0x0,
 but MUST NOT report any error.
 
+After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
+\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
+
 Before reading from \field{DeviceFeatures}, the driver MUST write a value to \field{DeviceFeaturesSel}.
 
 Before writing to the \field{DriverFeatures} register, the driver MUST write a value to the \field{DriverFeaturesSel} register.


---

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

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

On Fri, Aug 13, 2021 at 10:54 PM Srivatsa Vaddagiri
<quic_svaddagi@quicinc.com> wrote:
>
> Reset of a virtio-mmio device is initiated by writing 0 to its Status
> register. The reset operation itself may or may not be completed by the time
> write instruction completes. For devices that are emulated in software, writes
> to Status register are trapped and resumed only after the reset operation is
> complete. Thus a driver can be assured of reset completion as soon as its write
> completes. That may not be however true in other cases, such as when virtio-mmio
> devices implemented directly in hardware. Driver's write to Status register in
> such case could complete before the device reset operation is completed.
>
> Update the specification to indicate when a driver may need to poll for reset
> completion before going ahead with the remaining initialization steps.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
>
> ---
> V2->V1:
> - Use version 0x3 as an indication for drivers to poll for reset completion
>   (rather than base it on a new feature bit)
>
> Previous version can be found at:
>
> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
>
>  content.tex | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 7cec1c3..b6b30a0 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1730,9 +1730,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 +1916,7 @@ \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.
>
>  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,12 +1947,15 @@ \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.
>
>  The driver MUST ignore a device with \field{DeviceID} 0x0,
>  but MUST NOT report any error.
>
> +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
> +

Do we need to limit this behaviour to 0x3 only?

Thanks

>  Before reading from \field{DeviceFeatures}, the driver MUST write a value to \field{DeviceFeaturesSel}.
>
>  Before writing to the \field{DriverFeatures} register, the driver MUST write a value to the \field{DriverFeaturesSel} register.
>
>
> ---
>
> Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
> non-member to the virtio-dev mailing list for consideration and inclusion.
>

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

* Jason Wang <jasowang@redhat.com> [2021-08-16 10:10:56]:

> > +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> > +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
> > +
> 
> Do we need to limit this behaviour to 0x3 only?

I felt it will be simple for drivers to unconditionally poll for reset
completion (which is the right thing to do IMO), but can make it conditional to
version 3 only if you say so.

- vatsa

---

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

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 10:27:18AM +0530, Srivatsa Vaddagiri wrote:
> * Jason Wang <jasowang@redhat.com> [2021-08-16 10:10:56]:
> 
> > > +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> > > +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
> > > +
> > 
> > Do we need to limit this behaviour to 0x3 only?
> 
> I felt it will be simple for drivers to unconditionally poll for reset
> completion (which is the right thing to do IMO), but can make it conditional to
> version 3 only if you say so.
> 
> - vatsa

We can't make spec changes that declare all existing drivers
non-conformant.

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

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Michael S. Tsirkin]

On Fri, Aug 13, 2021 at 08:23:51PM +0530, Srivatsa Vaddagiri wrote:
> Reset of a virtio-mmio device is initiated by writing 0 to its Status
> register. The reset operation itself may or may not be completed by the time
> write instruction completes. For devices that are emulated in software, writes
> to Status register are trapped and resumed only after the reset operation is
> complete. Thus a driver can be assured of reset completion as soon as its write
> completes. That may not be however true in other cases, such as when virtio-mmio
> devices implemented directly in hardware. Driver's write to Status register in
> such case could complete before the device reset operation is completed.
> 
> Update the specification to indicate when a driver may need to poll for reset
> completion before going ahead with the remaining initialization steps.
>     
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
> 
> ---
> V2->V1:
> - Use version 0x3 as an indication for drivers to poll for reset completion
>   (rather than base it on a new feature bit)
> 
> Previous version can be found at:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> 
>  content.tex | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 7cec1c3..b6b30a0 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1730,9 +1730,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 +1916,7 @@ \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.
>  
>  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,12 +1947,15 @@ \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.
>  
>  The driver MUST ignore a device with \field{DeviceID} 0x0,
>  but MUST NOT report any error.
>  
> +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.

I know you copied this from pci but looking at PCI now, I think
this is not great there either.

1.  This is IMHO too opaque in that we did not say driver should read.

E.g. after writing 0 to Status and before reading any fields, the driver
MUST read Status and wait for a read etc.

also would not just a read be sufficient? Is there a case for device to return any value
other than 0 to signal "reset in progress"?

2. what if device encounters an error and sets
NEED_RESET meanwhile? waiting for a reset will then deadlock ...
I know, it's problematic like this in PCI too.

Further what will device return after reset was written but
before it completed? I think it is better to specify that
rather than rely on any non-0 value meaning "wait for reset".
For pci maybe make it a SHOULD ...


> +
>  Before reading from \field{DeviceFeatures}, the driver MUST write a value to \field{DeviceFeaturesSel}.
>  
>  Before writing to the \field{DriverFeatures} register, the driver MUST write a value to the \field{DriverFeaturesSel} register.
> 
> 
> ---
> 
> Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
> non-member to the virtio-dev mailing list for consideration and inclusion.

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

* Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:30:57]:

> > +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> > +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
> 
> I know you copied this from pci but looking at PCI now, I think
> this is not great there either.
> 
> 1.  This is IMHO too opaque in that we did not say driver should read.
> 
> E.g. after writing 0 to Status and before reading any fields, the driver
> MUST read Status and wait for a read etc.
> 
> also would not just a read be sufficient? Is there a case for device to return any value
> other than 0 to signal "reset in progress"?

Not sure I follow you here. We need some means for device to indicate its reset
operation is complete - non-zero value of Status would indicate reset is still
in progress while zero value would indicate reset is complete? 

> 2. what if device encounters an error and sets
> NEED_RESET meanwhile? waiting for a reset will then deadlock ...
> I know, it's problematic like this in PCI too.

Hmm a reset itself having failed - not sure if another reset will clear things
up better. I guess device could attempt another reset by itself, but may give up
after couple of attempts by indicating NEED_RESET, at which point not sure if
another driver initiated reset will make a difference. At the minimum, the
guidance for drivers here would be to block until reset is completed (indicated
by Status returning 0) or device encountering some failure (NEED_RESET)? When
NEED_RESET is seen, should the spec prescribe driver to attempt reset again?

> Further what will device return after reset was written but
> before it completed? I think it is better to specify that
> rather than rely on any non-0 value meaning "wait for reset".

Yes agree, we should have a specific status bit to indicate "reset in progress"

> For pci maybe make it a SHOULD ...

Do you mean pci driver SHOULD wait until this new status bit (RESET_IN_PROGRESS)
is cleared? 

- vatsa

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

[virtio-dev] Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Cornelia Huck]

On Fri, Aug 13 2021, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> wrote:

> Reset of a virtio-mmio device is initiated by writing 0 to its Status
> register. The reset operation itself may or may not be completed by the time
> write instruction completes. For devices that are emulated in software, writes
> to Status register are trapped and resumed only after the reset operation is
> complete. Thus a driver can be assured of reset completion as soon as its write
> completes. That may not be however true in other cases, such as when virtio-mmio
> devices implemented directly in hardware. Driver's write to Status register in
> such case could complete before the device reset operation is completed.
>
> Update the specification to indicate when a driver may need to poll for reset
> completion before going ahead with the remaining initialization steps.
>     
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
>
> ---
> V2->V1:
> - Use version 0x3 as an indication for drivers to poll for reset completion
>   (rather than base it on a new feature bit)
>
> Previous version can be found at:
>
> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
>
>  content.tex | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 7cec1c3..b6b30a0 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1730,9 +1730,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 +1916,7 @@ \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.

Maybe I'm overthining this, but might that suddenly render some existing
device non-compliant? IIUC, we never mandated reset to be completed when
the write was completed, only kind of implied it. An existing device
will have version 2, but it might not be quite finished with reset after
the status write yet.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

在 2021/8/17 下午11:26, Cornelia Huck 写道:
> On Fri, Aug 13 2021, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> wrote:
>
>> Reset of a virtio-mmio device is initiated by writing 0 to its Status
>> register. The reset operation itself may or may not be completed by the time
>> write instruction completes. For devices that are emulated in software, writes
>> to Status register are trapped and resumed only after the reset operation is
>> complete. Thus a driver can be assured of reset completion as soon as its write
>> completes. That may not be however true in other cases, such as when virtio-mmio
>> devices implemented directly in hardware. Driver's write to Status register in
>> such case could complete before the device reset operation is completed.
>>
>> Update the specification to indicate when a driver may need to poll for reset
>> completion before going ahead with the remaining initialization steps.
>>      
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
>>
>> ---
>> V2->V1:
>> - Use version 0x3 as an indication for drivers to poll for reset completion
>>    (rather than base it on a new feature bit)
>>
>> Previous version can be found at:
>>
>> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
>>
>>   content.tex | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 7cec1c3..b6b30a0 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -1730,9 +1730,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 +1916,7 @@ \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.
> Maybe I'm overthining this, but might that suddenly render some existing
> device non-compliant? IIUC, we never mandated reset to be completed when
> the write was completed, only kind of implied it. An existing device
> will have version 2, but it might not be quite finished with reset after
> the status write yet.


Won't this break the driver probe/registering?

int register_virtio_device(struct virtio_device *dev)

{

...

         /* We always start by resetting the device, in case a previous
          * driver messed it up.  This also tests that code path a 
little. */
         dev->config->reset(dev);

         /* Acknowledge that we've seen the device. */
         virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);

}

Thanks


>

[virtio-dev] Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Cornelia Huck]

On Wed, Aug 18 2021, Jason Wang <jasowang@redhat.com> wrote:

> 在 2021/8/17 下午11:26, Cornelia Huck 写道:
>> On Fri, Aug 13 2021, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> wrote:
>>
>>> Reset of a virtio-mmio device is initiated by writing 0 to its Status
>>> register. The reset operation itself may or may not be completed by the time
>>> write instruction completes. For devices that are emulated in software, writes
>>> to Status register are trapped and resumed only after the reset operation is
>>> complete. Thus a driver can be assured of reset completion as soon as its write
>>> completes. That may not be however true in other cases, such as when virtio-mmio
>>> devices implemented directly in hardware. Driver's write to Status register in
>>> such case could complete before the device reset operation is completed.
>>>
>>> Update the specification to indicate when a driver may need to poll for reset
>>> completion before going ahead with the remaining initialization steps.
>>>      
>>> Suggested-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
>>>
>>> ---
>>> V2->V1:
>>> - Use version 0x3 as an indication for drivers to poll for reset completion
>>>    (rather than base it on a new feature bit)
>>>
>>> Previous version can be found at:
>>>
>>> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
>>>
>>>   content.tex | 11 +++++++----
>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 7cec1c3..b6b30a0 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -1730,9 +1730,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 +1916,7 @@ \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.
>> Maybe I'm overthining this, but might that suddenly render some existing
>> device non-compliant? IIUC, we never mandated reset to be completed when
>> the write was completed, only kind of implied it. An existing device
>> will have version 2, but it might not be quite finished with reset after
>> the status write yet.
>
>
> Won't this break the driver probe/registering?

It might be racy, I guess; I'm not sure how other device or driver
implementations work.

>
> int register_virtio_device(struct virtio_device *dev)
>
> {
>
> ...
>
>          /* We always start by resetting the device, in case a previous
>           * driver messed it up.  This also tests that code path a 
> little. */
>          dev->config->reset(dev);
>
>          /* Acknowledge that we've seen the device. */
>          virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>
> }
>
> Thanks
>
>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

On Wed, Aug 18, 2021 at 2:34 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Wed, Aug 18 2021, Jason Wang <jasowang@redhat.com> wrote:
>
> > 在 2021/8/17 下午11:26, Cornelia Huck 写道:
> >> On Fri, Aug 13 2021, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> wrote:
> >>
> >>> Reset of a virtio-mmio device is initiated by writing 0 to its Status
> >>> register. The reset operation itself may or may not be completed by the time
> >>> write instruction completes. For devices that are emulated in software, writes
> >>> to Status register are trapped and resumed only after the reset operation is
> >>> complete. Thus a driver can be assured of reset completion as soon as its write
> >>> completes. That may not be however true in other cases, such as when virtio-mmio
> >>> devices implemented directly in hardware. Driver's write to Status register in
> >>> such case could complete before the device reset operation is completed.
> >>>
> >>> Update the specification to indicate when a driver may need to poll for reset
> >>> completion before going ahead with the remaining initialization steps.
> >>>
> >>> Suggested-by: Jason Wang <jasowang@redhat.com>
> >>> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
> >>>
> >>> ---
> >>> V2->V1:
> >>> - Use version 0x3 as an indication for drivers to poll for reset completion
> >>>    (rather than base it on a new feature bit)
> >>>
> >>> Previous version can be found at:
> >>>
> >>> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> >>>
> >>>   content.tex | 11 +++++++----
> >>>   1 file changed, 7 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 7cec1c3..b6b30a0 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -1730,9 +1730,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 +1916,7 @@ \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.
> >> Maybe I'm overthining this, but might that suddenly render some existing
> >> device non-compliant? IIUC, we never mandated reset to be completed when
> >> the write was completed, only kind of implied it. An existing device
> >> will have version 2, but it might not be quite finished with reset after
> >> the status write yet.
> >
> >
> > Won't this break the driver probe/registering?
>
> It might be racy, I guess; I'm not sure how other device or driver
> implementations work.

PCI forces a re-read like:

        /* 0 status means a reset. */
        vp_modern_set_status(mdev, 0);
        /* After writing 0 to device_status, the driver MUST wait for a read of
         * device_status to return 0 before reinitializing the device.
         * This will flush out the status write, and flush in device writes,
         * including MSI-X interrupts, if any.
         */
        while (vp_modern_get_status(mdev))
                msleep(1);
        /* Flush pending VQ/configuration callbacks. */
        vp_synchronize_vectors(vdev);

So it's fine.

Thanks

>
> >
> > int register_virtio_device(struct virtio_device *dev)
> >
> > {
> >
> > ...
> >
> >          /* We always start by resetting the device, in case a previous
> >           * driver messed it up.  This also tests that code path a
> > little. */
> >          dev->config->reset(dev);
> >
> >          /* Acknowledge that we've seen the device. */
> >          virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >
> > }
> >
> > Thanks
> >
> >
> >>
>

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

On Mon, Aug 16, 2021 at 1:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Aug 13, 2021 at 08:23:51PM +0530, Srivatsa Vaddagiri wrote:
> > Reset of a virtio-mmio device is initiated by writing 0 to its Status
> > register. The reset operation itself may or may not be completed by the time
> > write instruction completes. For devices that are emulated in software, writes
> > to Status register are trapped and resumed only after the reset operation is
> > complete. Thus a driver can be assured of reset completion as soon as its write
> > completes. That may not be however true in other cases, such as when virtio-mmio
> > devices implemented directly in hardware. Driver's write to Status register in
> > such case could complete before the device reset operation is completed.
> >
> > Update the specification to indicate when a driver may need to poll for reset
> > completion before going ahead with the remaining initialization steps.
> >
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
> >
> > ---
> > V2->V1:
> > - Use version 0x3 as an indication for drivers to poll for reset completion
> >   (rather than base it on a new feature bit)
> >
> > Previous version can be found at:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> >
> >  content.tex | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 7cec1c3..b6b30a0 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -1730,9 +1730,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 +1916,7 @@ \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.
> >
> >  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,12 +1947,15 @@ \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.
> >
> >  The driver MUST ignore a device with \field{DeviceID} 0x0,
> >  but MUST NOT report any error.
> >
> > +After writing 0 to \field{Status}, which triggers a reset of device, the driver MUST wait for a read of
> > +\field{Status} to return 0 before proceeding with the remaining steps of initializing the device.
>
> I know you copied this from pci but looking at PCI now, I think
> this is not great there either.
>
> 1.  This is IMHO too opaque in that we did not say driver should read.
>
> E.g. after writing 0 to Status and before reading any fields, the driver
> MUST read Status and wait for a read etc.
>
> also would not just a read be sufficient? Is there a case for device to return any value
> other than 0 to signal "reset in progress"?

I'm not sure what case it can help. What is doing here is no worse
than the way PCI deal with it.

>
> 2. what if device encounters an error and sets
> NEED_RESET meanwhile? waiting for a reset will then deadlock ...
> I know, it's problematic like this in PCI too.

Yes, it's kind of a recursion that is not something that can be
handled at the virtio level.

We probably need a transport reset (FLR) here. But it's another topic I think.

>
> Further what will device return after reset was written but
> before it completed? I think it is better to specify that
> rather than rely on any non-0 value meaning "wait for reset".
> For pci maybe make it a SHOULD ...
>

I agree.

Thanks

>
> > +
> >  Before reading from \field{DeviceFeatures}, the driver MUST write a value to \field{DeviceFeaturesSel}.
> >
> >  Before writing to the \field{DriverFeatures} register, the driver MUST write a value to the \field{DriverFeaturesSel} register.
> >
> >
> > ---
> >
> > Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
> > non-member to the virtio-dev mailing list for consideration and inclusion.
>

Re: [PATCH v2] virtio-mmio: Specify wait needed in driver during reset

[Michael S. Tsirkin]

On Fri, Aug 20, 2021 at 11:48:04AM +0800, Jason Wang wrote:
> On Wed, Aug 18, 2021 at 2:34 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Wed, Aug 18 2021, Jason Wang <jasowang@redhat.com> wrote:
> >
> > > 在 2021/8/17 下午11:26, Cornelia Huck 写道:
> > >> On Fri, Aug 13 2021, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> wrote:
> > >>
> > >>> Reset of a virtio-mmio device is initiated by writing 0 to its Status
> > >>> register. The reset operation itself may or may not be completed by the time
> > >>> write instruction completes. For devices that are emulated in software, writes
> > >>> to Status register are trapped and resumed only after the reset operation is
> > >>> complete. Thus a driver can be assured of reset completion as soon as its write
> > >>> completes. That may not be however true in other cases, such as when virtio-mmio
> > >>> devices implemented directly in hardware. Driver's write to Status register in
> > >>> such case could complete before the device reset operation is completed.
> > >>>
> > >>> Update the specification to indicate when a driver may need to poll for reset
> > >>> completion before going ahead with the remaining initialization steps.
> > >>>
> > >>> Suggested-by: Jason Wang <jasowang@redhat.com>
> > >>> Signed-off-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
> > >>>
> > >>> ---
> > >>> V2->V1:
> > >>> - Use version 0x3 as an indication for drivers to poll for reset completion
> > >>>    (rather than base it on a new feature bit)
> > >>>
> > >>> Previous version can be found at:
> > >>>
> > >>> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> > >>>
> > >>>   content.tex | 11 +++++++----
> > >>>   1 file changed, 7 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/content.tex b/content.tex
> > >>> index 7cec1c3..b6b30a0 100644
> > >>> --- a/content.tex
> > >>> +++ b/content.tex
> > >>> @@ -1730,9 +1730,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 +1916,7 @@ \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.
> > >> Maybe I'm overthining this, but might that suddenly render some existing
> > >> device non-compliant? IIUC, we never mandated reset to be completed when
> > >> the write was completed, only kind of implied it. An existing device
> > >> will have version 2, but it might not be quite finished with reset after
> > >> the status write yet.
> > >
> > >
> > > Won't this break the driver probe/registering?
> >
> > It might be racy, I guess; I'm not sure how other device or driver
> > implementations work.
> 
> PCI forces a re-read like:
> 
>         /* 0 status means a reset. */
>         vp_modern_set_status(mdev, 0);
>         /* After writing 0 to device_status, the driver MUST wait for a read of
>          * device_status to return 0 before reinitializing the device.
>          * This will flush out the status write, and flush in device writes,
>          * including MSI-X interrupts, if any.
>          */
>         while (vp_modern_get_status(mdev))
>                 msleep(1);
>         /* Flush pending VQ/configuration callbacks. */
>         vp_synchronize_vectors(vdev);

Something to fix as we deal with suprise removal, BTW.

> 
> So it's fine.
> 
> Thanks
> 
> >
> > >
> > > int register_virtio_device(struct virtio_device *dev)
> > >
> > > {
> > >
> > > ...
> > >
> > >          /* We always start by resetting the device, in case a previous
> > >           * driver messed it up.  This also tests that code path a
> > > little. */
> > >          dev->config->reset(dev);
> > >
> > >          /* Acknowledge that we've seen the device. */
> > >          virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > >
> > > }
> > >
> > > Thanks
> > >
> > >
> > >>
> >