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

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

[Srivatsa Vaddagiri]

[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]

Changes since v0:

Introduce a new feature bit, VIRTIO_F_MMIO_RESET_WAIT, which controls whether a driver polls
for reset completion or not.

===

Reset of a virtio-mmio device is accomplished by writing 0 to its Status register.
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 the case for virtio-mmio devices implemented in hardware
directly. A write could complete before the reset operation inside device is
completed. Introduce a new feature, VIRTIO_F_MMIO_RESET_WAIT, that such devices
will offer as means to indicate to driver that they need to poll for reset
completion, indicated by a read of Status register returning 0.

Signed-off-by: Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com>

diff --git a/content.tex b/content.tex
index 5c70a3c..1990a5c 100644
--- a/content.tex
+++ b/content.tex
@@ -1924,7 +1924,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 do not represent events which took place MUST be zero.

 Upon reset, the device MUST clear all bits in \field{InterruptStatus} and ready bits in the
-\field{QueueReady} register for all queues in the device.
+\field{QueueReady} register for all queues in the device. The device MUST also offer
+VIRTIO_F_MMIO_RESET_WAIT if it requires driver to poll for reset completion
+indicated by a read of \field{Status} returning 0. Such a device MUST also fail to accept
+FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.

 The device MUST change value returned in \field{ConfigGeneration} if there is any risk of a
 driver seeing an inconsistent configuration state.
@@ -1944,6 +1947,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 8 bit wide fields, 16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide and
 aligned accesses for 32 and 64 bit wide fields.

+The driver MUST accept VIRTIO_F_MMIO_RESET_WAIT if offered by device. During
+reset of such a device that offers VIRTIO_F_MMIO_RESET_WAIT, driver MUST poll for
+reset completion indicated by a read of \field{Status} returning 0.
+
 The driver MUST ignore a device with \field{MagicValue} which is not 0x74726976,
 although it MAY report an error.

@@ -6672,6 +6679,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   transport specific.
   For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.

+  \item[VIRTIO_F_MMIO_RESET_WAIT(40)] This feature could be offered by a device
+  using MMIO transport and indicates that the driver needs to poll for reset
+  completion indicated by a read of \field{Status} returning 0.
+
 \end{description}

 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
@@ -6708,6 +6719,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}

 A driver SHOULD accept VIRTIO_F_NOTIF_CONFIG_DATA if it is offered.

+A driver SHOULD accept VIRTIO_F_MMIO_RESET_WAIT if it is offered.
+
 \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

 A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further


---

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

[-- Attachment #2: Type: text/html, Size: 5811 bytes --]

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

[Cornelia Huck]

On Fri, Jul 23 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:

> Changes since v0:
>
> Introduce a new feature bit, VIRTIO_F_MMIO_RESET_WAIT, which controls whether a driver polls
> for reset completion or not.

Nit: changelog should go into a "---" section, next to the diffstat (git
format-patch should give you the correct base to edit.)

>
> ===
>
> Reset of a virtio-mmio device is accomplished by writing 0 to its Status register.
> 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 the case for virtio-mmio devices implemented in hardware
> directly. A write could complete before the reset operation inside device is
> completed. Introduce a new feature, VIRTIO_F_MMIO_RESET_WAIT, that such devices
> will offer as means to indicate to driver that they need to poll for reset
> completion, indicated by a read of Status register returning 0.
>
> Signed-off-by: Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com>
>
> diff --git a/content.tex b/content.tex
> index 5c70a3c..1990a5c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1924,7 +1924,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  do not represent events which took place MUST be zero.
>
>  Upon reset, the device MUST clear all bits in \field{InterruptStatus} and ready bits in the
> -\field{QueueReady} register for all queues in the device.
> +\field{QueueReady} register for all queues in the device. The device MUST also offer
> +VIRTIO_F_MMIO_RESET_WAIT if it requires driver to poll for reset completion

s/driver/the driver/

> +indicated by a read of \field{Status} returning 0. Such a device MUST also fail to accept
> +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.

So, this basically means that an older driver that does not know the new
feature bit will not work with devices offering this bit, even if it did
poll? This seems acceptable, I just wanted to spell it out.

>
>  The device MUST change value returned in \field{ConfigGeneration} if there is any risk of a
>  driver seeing an inconsistent configuration state.
> @@ -1944,6 +1947,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  8 bit wide fields, 16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide and
>  aligned accesses for 32 and 64 bit wide fields.
>
> +The driver MUST accept VIRTIO_F_MMIO_RESET_WAIT if offered by device. During

s/device/the device/

> +reset of such a device that offers VIRTIO_F_MMIO_RESET_WAIT, driver MUST poll for

s/driver/the driver/

But maybe use "if VIRTIO_F_MMIO_RESET_WAIT has been negotiated" instead?
This is implied by the statements above, but the deciding factor here is
that the feature has been negotiated, and not that it has been offered.


> +reset completion indicated by a read of \field{Status} returning 0.
> +
>  The driver MUST ignore a device with \field{MagicValue} which is not 0x74726976,
>  although it MAY report an error.
>
> @@ -6672,6 +6679,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    transport specific.
>    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>
> +  \item[VIRTIO_F_MMIO_RESET_WAIT(40)] This feature could be offered by a device
> +  using MMIO transport and indicates that the driver needs to poll for reset

"This feature is specific to the MMIO transport and indicates that the
device requires the driver to poll..."

?

> +  completion indicated by a read of \field{Status} returning 0.
> +
>  \end{description}
>
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> @@ -6708,6 +6719,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
>  A driver SHOULD accept VIRTIO_F_NOTIF_CONFIG_DATA if it is offered.
>
> +A driver SHOULD accept VIRTIO_F_MMIO_RESET_WAIT if it is offered.
> +

But this was specified as MUST above, wasn't it?

>  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>
>  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further


---------------------------------------------------------------------
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 v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

> > Changes since v0:
> >
> > Introduce a new feature bit, VIRTIO_F_MMIO_RESET_WAIT, which controls whether
> > a driver polls
> > for reset completion or not.
>
> Nit: changelog should go into a "---" section, next to the diffstat (git
> format-patch should give you the correct base to edit.)

Thanks for your review. Will take care of that in next version.

> > +indicated by a read of \field{Status} returning 0. Such a device MUST also
> > fail to accept
> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
>
> So, this basically means that an older driver that does not know the new
> feature bit will not work with devices offering this bit, even if it did
> poll? This seems acceptable, I just wanted to spell it out.

Yes I believe that's the desired result. Device has no way to know if driver
will poll for reset completion or not unless it accepts the feature.
Are you suggesting we explicitly put in some text in spec to that effect?

> > +reset of such a device that offers VIRTIO_F_MMIO_RESET_WAIT, driver MUST poll
> > for
>
> s/driver/the driver/
>
> But maybe use "if VIRTIO_F_MMIO_RESET_WAIT has been negotiated" instead?
> This is implied by the statements above, but the deciding factor here is
> that the feature has been negotiated, and not that it has been offered.

Sure sounds good.

> > +reset completion indicated by a read of \field{Status} returning 0.
> > +
> >  The driver MUST ignore a device with \field{MagicValue} which is not
> >  0x74726976,
> >  although it MAY report an error.
> >
> > @@ -6672,6 +6679,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> > Feature Bits}
> >    transport specific.
> >    For more details about driver notifications over PCI see \ref{sec:Virtio
> >    Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And
> >    Device Operation / Available Buffer Notifications}.
> >
> > +  \item[VIRTIO_F_MMIO_RESET_WAIT(40)] This feature could be offered by a
> > device
> > +  using MMIO transport and indicates that the driver needs to poll for reset
>
> "This feature is specific to the MMIO transport and indicates that the
> device requires the driver to poll..."
>
> ?

Ok

> > +  completion indicated by a read of \field{Status} returning 0.
> > +
> >  \end{description}
> >
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > @@ -6708,6 +6719,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> > Feature Bits}
> >
> >  A driver SHOULD accept VIRTIO_F_NOTIF_CONFIG_DATA if it is offered.
> >
> > +A driver SHOULD accept VIRTIO_F_MMIO_RESET_WAIT if it is offered.
> > +
>
> But this was specified as MUST above, wasn't it?

I guess I followed the convention of using SHOULD for other bits such as
VIRTIO_F_NOTIF_CONFIG_DATA in that section, but yes MUST sounds better.

I will post the next version shortly with recommended changes.

- vatsa

---

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

[-- Attachment #2: Type: text/html, Size: 5241 bytes --]

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

[Cornelia Huck]

On Mon, Jul 26 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:

>> > +indicated by a read of \field{Status} returning 0. Such a device MUST also
>> > fail to accept
>> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
>>
>> So, this basically means that an older driver that does not know the new
>> feature bit will not work with devices offering this bit, even if it did
>> poll? This seems acceptable, I just wanted to spell it out.
>
> Yes I believe that's the desired result. Device has no way to know if driver
> will poll for reset completion or not unless it accepts the feature.
> Are you suggesting we explicitly put in some text in spec to that
> effect?

Not sure whether it is needed (do others have an opinion?), but probably
not.


---------------------------------------------------------------------
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 v1] virtio-mmio: Specify wait needed in driver during reset

[Michael S. Tsirkin]

On Mon, Jul 26, 2021 at 01:36:56PM +0200, Cornelia Huck wrote:
> On Mon, Jul 26 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
> 
> >> > +indicated by a read of \field{Status} returning 0. Such a device MUST also
> >> > fail to accept
> >> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
> >>
> >> So, this basically means that an older driver that does not know the new
> >> feature bit will not work with devices offering this bit, even if it did
> >> poll? This seems acceptable, I just wanted to spell it out.
> >
> > Yes I believe that's the desired result. Device has no way to know if driver
> > will poll for reset completion or not unless it accepts the feature.
> > Are you suggesting we explicitly put in some text in spec to that
> > effect?
> 
> Not sure whether it is needed (do others have an opinion?), but probably
> not.

What about resets before FEATURES_OK? How are these handled?

-- 
MST

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

[Cornelia Huck]

On Mon, Jul 26 2021, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 26, 2021 at 01:36:56PM +0200, Cornelia Huck wrote:
>> On Mon, Jul 26 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
>> 
>> >> > +indicated by a read of \field{Status} returning 0. Such a device MUST also
>> >> > fail to accept
>> >> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
>> >>
>> >> So, this basically means that an older driver that does not know the new
>> >> feature bit will not work with devices offering this bit, even if it did
>> >> poll? This seems acceptable, I just wanted to spell it out.
>> >
>> > Yes I believe that's the desired result. Device has no way to know if driver
>> > will poll for reset completion or not unless it accepts the feature.
>> > Are you suggesting we explicitly put in some text in spec to that
>> > effect?
>> 
>> Not sure whether it is needed (do others have an opinion?), but probably
>> not.
>
> What about resets before FEATURES_OK? How are these handled?

I'd say that neither driver nor device can assume anything. I.e. if the
driver writes 0 to status, it won't know whether it should poll or not
(polling would be the safe bet), and the device simply cannot know
whether the driver will wait with the next operation or continue
immediately.

Do we want to spell that out?


---------------------------------------------------------------------
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 v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

> On Mon, Jul 26, 2021 at 01:36:56PM +0200, Cornelia Huck wrote:
> > On Mon, Jul 26 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
> >
> > >> > +indicated by a read of \field{Status} returning 0. Such a device MUST also
> > >> > fail to accept
> > >> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
> > >>
> > >> So, this basically means that an older driver that does not know the new
> > >> feature bit will not work with devices offering this bit, even if it did
> > >> poll? This seems acceptable, I just wanted to spell it out.
> > >
> > > Yes I believe that's the desired result. Device has no way to know if driver
> > > will poll for reset completion or not unless it accepts the feature.
> > > Are you suggesting we explicitly put in some text in spec to that
> > > effect?
> >
> > Not sure whether it is needed (do others have an opinion?), but probably
> > not.
>
> What about resets before FEATURES_OK? How are these handled?

From device perspective, it's reset logic will always be same, independent of
when reset was performed by driver (before or after feature negotiation). A
driver that does not wait for reset completion will see undefined behavior after
reset until it discovers that feature negotiation has failed?

- vatsa

---

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



[-- Attachment #2: Type: text/html, Size: 3284 bytes --]

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

[Michael S. Tsirkin]

On Mon, Jul 26, 2021 at 02:17:08PM +0000, Srivatsa Vaddagiri wrote:
> > On Mon, Jul 26, 2021 at 01:36:56PM +0200, Cornelia Huck wrote:
> > > On Mon, Jul 26 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
> > >
> > > >> > +indicated by a read of \field{Status} returning 0. Such a device MUST
> also
> > > >> > fail to accept
> > > >> > +FEATURES_OK bit if driver does not negotiate
> VIRTIO_F_MMIO_RESET_WAIT.
> > > >>
> > > >> So, this basically means that an older driver that does not know the new
> > > >> feature bit will not work with devices offering this bit, even if it did
> > > >> poll? This seems acceptable, I just wanted to spell it out.
> > > >
> > > > Yes I believe that's the desired result. Device has no way to know if
> driver
> > > > will poll for reset completion or not unless it accepts the feature.
> > > > Are you suggesting we explicitly put in some text in spec to that
> > > > effect?
> > >
> > > Not sure whether it is needed (do others have an opinion?), but probably
> > > not.
> >
> > What about resets before FEATURES_OK? How are these handled?
> 
> From device perspective, it's reset logic will always be same, independent of
> when reset was performed by driver (before or after feature negotiation). A
> driver that does not wait for reset completion will see undefined behavior
> after
> reset until it discovers that feature negotiation has failed?
> 
> - vatsa

Hmm. that doesn't sound too good. Makes using
feature negotiation for this kind of useless ...
Device can actually detect a read from status, right?
Maybe if it sees status was not read it can just
stay in reset state and not exit it?


> ---
> 
> 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 v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

> > > What about resets before FEATURES_OK? How are these handled?
> >
> > From device perspective, it's reset logic will always be same, independent of
> > when reset was performed by driver (before or after feature negotiation). A
> > driver that does not wait for reset completion will see undefined behavior
> > after
> > reset until it discovers that feature negotiation has failed?
>
> Hmm. that doesn't sound too good. Makes using
> feature negotiation for this kind of useless ...
> Device can actually detect a read from status, right?
> Maybe if it sees status was not read it can just
> stay in reset state and not exit it?

Thinking on those lines, I am wondering if we can avoid the feature bit
all together based on that logic.

1) Driver writes 0 to status register - device initiates reset sequence
2) Driver reads status - return 0x40 (VIRTIO_CONFIG_S_NEEDS_RESET)
   if reset sequence is in progress, otherwise return 0
3) Driver writes status (ACKNOWLEDGE Bit) - if this happened before reset
   sequence completed and before we returned 0 for status read, then reject
   further initialization by setting VIRTIO_CONFIG_S_NEEDS_RESET (until next
   reset that is)

This should let device detect if we have a driver polling on reset completion or
not?

---

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

[-- Attachment #2: Type: text/html, Size: 2288 bytes --]

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

[Cornelia Huck]

On Tue, Jul 27 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:

>> > > What about resets before FEATURES_OK? How are these handled?
>> >
>> > From device perspective, it's reset logic will always be same, independent of
>> > when reset was performed by driver (before or after feature negotiation). A
>> > driver that does not wait for reset completion will see undefined behavior
>> > after
>> > reset until it discovers that feature negotiation has failed?
>>
>> Hmm. that doesn't sound too good. Makes using
>> feature negotiation for this kind of useless ...
>> Device can actually detect a read from status, right?
>> Maybe if it sees status was not read it can just
>> stay in reset state and not exit it?
>
> Thinking on those lines, I am wondering if we can avoid the feature bit
> all together based on that logic.
>
> 1) Driver writes 0 to status register - device initiates reset sequence
> 2) Driver reads status - return 0x40 (VIRTIO_CONFIG_S_NEEDS_RESET)
>    if reset sequence is in progress, otherwise return 0

I don't think we can have the device return NEEDS_RESET in that case;
that status is supposed to indicate "that the device has experienced an
error from which it can’t recover" -- which is not the case here,
especially since the driver has already initiated a reset.

> 3) Driver writes status (ACKNOWLEDGE Bit) - if this happened before reset
>    sequence completed and before we returned 0 for status read, then reject
>    further initialization by setting VIRTIO_CONFIG_S_NEEDS_RESET (until next
>    reset that is)
>
> This should let device detect if we have a driver polling on reset completion or
> not?


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

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

[Srivatsa Vaddagiri]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

> > Thinking on those lines, I am wondering if we can avoid the feature bit
> > all together based on that logic.
> >
> > 1) Driver writes 0 to status register - device initiates reset sequence
> > 2) Driver reads status - return 0x40 (VIRTIO_CONFIG_S_NEEDS_RESET)
> >    if reset sequence is in progress, otherwise return 0
>
> I don't think we can have the device return NEEDS_RESET in that case;
> that status is supposed to indicate "that the device has experienced an
> error from which it can't recover" -- which is not the case here,
> especially since the driver has already initiated a reset.

In that case, how about introducing a new status bit, RESET_IN_PROGRESS (bit 4 or
some other unused bit) and return that to indicate reset is not complete?

Its not clear otherwise what the device is supposed to return in such case.

---

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



[-- Attachment #2: Type: text/html, Size: 2046 bytes --]

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

[Jason Wang]

在 2021/7/27 上午3:03, Michael S. Tsirkin 写道:
> On Mon, Jul 26, 2021 at 02:17:08PM +0000, Srivatsa Vaddagiri wrote:
>>> On Mon, Jul 26, 2021 at 01:36:56PM +0200, Cornelia Huck wrote:
>>>> On Mon, Jul 26 2021, Srivatsa Vaddagiri<svaddagi@qti.qualcomm.com>  wrote:
>>>>
>>>>>>> +indicated by a read of \field{Status} returning 0. Such a device MUST
>> also
>>>>>>> fail to accept
>>>>>>> +FEATURES_OK bit if driver does not negotiate
>> VIRTIO_F_MMIO_RESET_WAIT.
>>>>>> So, this basically means that an older driver that does not know the new
>>>>>> feature bit will not work with devices offering this bit, even if it did
>>>>>> poll? This seems acceptable, I just wanted to spell it out.
>>>>> Yes I believe that's the desired result. Device has no way to know if
>> driver
>>>>> will poll for reset completion or not unless it accepts the feature.
>>>>> Are you suggesting we explicitly put in some text in spec to that
>>>>> effect?
>>>> Not sure whether it is needed (do others have an opinion?), but probably
>>>> not.
>>> What about resets before FEATURES_OK? How are these handled?
>>  From device perspective, it's reset logic will always be same, independent of
>> when reset was performed by driver (before or after feature negotiation). A
>> driver that does not wait for reset completion will see undefined behavior
>> after
>> reset until it discovers that feature negotiation has failed?
>>
>> - vatsa
> Hmm. that doesn't sound too good. Makes using
> feature negotiation for this kind of useless ...


Yes, the tricky part is that the reset happens before the features 
negotiation.


> Device can actually detect a read from status, right?


I'm not sure if MMIO can always behave like a register. E.g can area 
just plain DRAM?


> Maybe if it sees status was not read it can just
> stay in reset state and not exit it?
>
>

Or I wonder we can use increase the version instead.

Thanks

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

[Srivatsa Vaddagiri]

* Jason Wang <jasowang@redhat.com> [2021-08-02 14:06:38]:

> >Maybe if it sees status was not read it can just
> >stay in reset state and not exit it?
> >
> >
> 
> Or I wonder we can use increase the version instead.

What should be the guidance for backend/devices here? Go with version 3 only if
it requires driver to poll on reset? Otherwise it may break existing setups -
lets say Qemu is updated to report version 3 which will break older guest images
that understood version 2 - this is despite the same Qemu being capable of
serving those guests.

- vatsa

---

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: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jason Wang]

在 2021/8/11 下午6:05, Srivatsa Vaddagiri 写道:
> * Jason Wang <jasowang@redhat.com> [2021-08-02 14:06:38]:
>
>>> Maybe if it sees status was not read it can just
>>> stay in reset state and not exit it?
>>>
>>>
>> Or I wonder we can use increase the version instead.
> What should be the guidance for backend/devices here? Go with version 3 only if
> it requires driver to poll on reset? Otherwise it may break existing setups -
> lets say Qemu is updated to report version 3 which will break older guest images
> that understood version 2 - this is despite the same Qemu being capable of
> serving those guests.


My understanding is that qemu should stick to version 2 since it works fine.

Thanks


>
> - vatsa
>
> ---
>
> 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: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

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

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 10:09:34AM +0800, Jason Wang wrote:
> 
> 在 2021/8/11 下午6:05, Srivatsa Vaddagiri 写道:
> > * Jason Wang <jasowang@redhat.com> [2021-08-02 14:06:38]:
> > 
> > > > Maybe if it sees status was not read it can just
> > > > stay in reset state and not exit it?
> > > > 
> > > > 
> > > Or I wonder we can use increase the version instead.
> > What should be the guidance for backend/devices here? Go with version 3 only if
> > it requires driver to poll on reset? Otherwise it may break existing setups -
> > lets say Qemu is updated to report version 3 which will break older guest images
> > that understood version 2 - this is despite the same Qemu being capable of
> > serving those guests.
> 
> 
> My understanding is that qemu should stick to version 2 since it works fine.
> 
> Thanks

Yes breaking all guests does not sound like a good plan.
Which is unfortunate generally.

So thinking about all this, quite early in the setup process we
have:

        /* Figure out what features the device supports. */
        device_features = dev->config->get_features(dev);


isn't this sufficient? this will flush out
all writes and device can defer responding to reads
until reset is complete.

I know it's not elegant since there are a bunch of writes like
acknowledge and driver, but hey.

How about we just add a non-normative section explaining that trick?


> 
> > 
> > - vatsa
> > 
> > ---
> > 
> > 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: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 

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

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
> * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
> 
> > So thinking about all this, quite early in the setup process we
> > have:
> > 
> >         /* Figure out what features the device supports. */
> >         device_features = dev->config->get_features(dev);
> > 
> > 
> > isn't this sufficient? this will flush out
> > all writes and device can defer responding to reads
> > until reset is complete.
> 
> Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
> felt some of the MMIO devices, their registers could act as plain DRAM for
> reads, which means device can't block such reads?
> 
> - vatsa

Is it worth bothering about theoretical issues though? Jason what is
your take?


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

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

[Srivatsa Vaddagiri]

* Michael S. Tsirkin <mst@redhat.com> [2021-08-16 07:48:56]:

> On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
> > * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
> > 
> > > So thinking about all this, quite early in the setup process we
> > > have:
> > > 
> > >         /* Figure out what features the device supports. */
> > >         device_features = dev->config->get_features(dev);
> > > 
> > > 
> > > isn't this sufficient? this will flush out
> > > all writes and device can defer responding to reads
> > > until reset is complete.
> > 
> > Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
> > felt some of the MMIO devices, their registers could act as plain DRAM for
> > reads, which means device can't block such reads?
> > 
> > - vatsa
> 
> Is it worth bothering about theoretical issues though? Jason what is
> your take?

What about the case where device reset fails, which may require driver to
initiate reset again or give up on device? Won't the read of features be forever
deferred in that case?

- 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 07:04:51PM +0530, Srivatsa Vaddagiri wrote:
> * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 07:48:56]:
> 
> > On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
> > > * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
> > > 
> > > > So thinking about all this, quite early in the setup process we
> > > > have:
> > > > 
> > > >         /* Figure out what features the device supports. */
> > > >         device_features = dev->config->get_features(dev);
> > > > 
> > > > 
> > > > isn't this sufficient? this will flush out
> > > > all writes and device can defer responding to reads
> > > > until reset is complete.
> > > 
> > > Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
> > > felt some of the MMIO devices, their registers could act as plain DRAM for
> > > reads, which means device can't block such reads?
> > > 
> > > - vatsa
> > 
> > Is it worth bothering about theoretical issues though? Jason what is
> > your take?
> 
> What about the case where device reset fails, which may require driver to
> initiate reset again or give up on device? Won't the read of features be forever
> deferred in that case?
> 
> - vatsa

That might be worth addressing but I feel your proposal does not
address it sufficiently.

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

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

[Srivatsa Vaddagiri]

* Michael S. Tsirkin <mst@redhat.com> [2021-08-16 10:37:18]:

> > What about the case where device reset fails, which may require driver to
> > initiate reset again or give up on device? Won't the read of features be forever
> > deferred in that case?
> > 
> > - vatsa
> 
> That might be worth addressing but I feel your proposal does not
> address it sufficiently.

Ok let me take a stab at addressing it better in my next version. Basically
here's what I am thinking. Pls let me know if there is anything else I need to
consider:

* Introduce a status bit for device to indicate "reset in progress". This could
  be adopted by other transports also (for ex PCI).

* For v3 MMIO devices, driver, after initiating reset, should wait until status = 0
  or device indicates "failure". Should the spec advise what a driver should do
  in case of failure after reset? Not sure if the outcome would be different if
  driver were to issue reset again. Again this advice should be common to both
  MMIO and PCI I think.

- 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

在 2021/8/16 下午7:48, Michael S. Tsirkin 写道:
> On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
>> * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
>>
>>> So thinking about all this, quite early in the setup process we
>>> have:
>>>
>>>          /* Figure out what features the device supports. */
>>>          device_features = dev->config->get_features(dev);
>>>
>>>
>>> isn't this sufficient? this will flush out
>>> all writes and device can defer responding to reads
>>> until reset is complete.
>> Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
>> felt some of the MMIO devices, their registers could act as plain DRAM for
>> reads, which means device can't block such reads?
>>
>> - vatsa
> Is it worth bothering about theoretical issues though? Jason what is
> your take?


Re-read the spec, and it said

"

MMIO Device Register Layout

"

I believe plain DRAM will not behave like a register. So we don't need 
to worry about that.

But they are indeed devices that act as a plain DRAM for their control 
path, which might requires new transports[1].

Another question is that what makes PCI differ from MMIO. (E.g PCI 
requires a re-read but MMIO doesn't)

[1] https://lkml.org/lkml/2020/9/1/255


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

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

[Michael S. Tsirkin]

On Tue, Aug 17, 2021 at 01:45:12PM +0800, Jason Wang wrote:
> 
> 在 2021/8/16 下午7:48, Michael S. Tsirkin 写道:
> > On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
> > > * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
> > > 
> > > > So thinking about all this, quite early in the setup process we
> > > > have:
> > > > 
> > > >          /* Figure out what features the device supports. */
> > > >          device_features = dev->config->get_features(dev);
> > > > 
> > > > 
> > > > isn't this sufficient? this will flush out
> > > > all writes and device can defer responding to reads
> > > > until reset is complete.
> > > Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
> > > felt some of the MMIO devices, their registers could act as plain DRAM for
> > > reads, which means device can't block such reads?
> > > 
> > > - vatsa
> > Is it worth bothering about theoretical issues though? Jason what is
> > your take?
> 
> 
> Re-read the spec, and it said
> 
> "
> 
> MMIO Device Register Layout
> 
> "
> 
> I believe plain DRAM will not behave like a register. So we don't need to
> worry about that.
> 
> But they are indeed devices that act as a plain DRAM for their control path,
> which might requires new transports[1].
> 
> Another question is that what makes PCI differ from MMIO. (E.g PCI requires
> a re-read but MMIO doesn't)

I don't think PCI requires a re-read either.  IIRC from that meeting, it
was intended for a hypervisor so it does not have to block the guest:
the guest can do something else then re-read.
We did similar things elsewhere.

See
https://issues.oasis-open.org/projects/VIRTIO/issues/VIRTIO-103
for the original proposal.

In any case, I would say status register is unlikely to be in regular
DRAM, after all write has side effects.
So before we move on I'd like to know whether we do something as drastic
as incrementing the version number for a theoretical or practical
benefit.


> [1] https://lkml.org/lkml/2020/9/1/255
> 
> 
> > 
> > 
> > > ---
> > > 
> > > Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
> > > non-member to the virtio-dev mailing list for consideration and inclusion.

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

[Jason Wang]

在 2021/8/17 下午3:51, Michael S. Tsirkin 写道:
> On Tue, Aug 17, 2021 at 01:45:12PM +0800, Jason Wang wrote:
>> 在 2021/8/16 下午7:48, Michael S. Tsirkin 写道:
>>> On Mon, Aug 16, 2021 at 12:05:50PM +0530, Srivatsa Vaddagiri wrote:
>>>> * Michael S. Tsirkin <mst@redhat.com> [2021-08-16 01:35:46]:
>>>>
>>>>> So thinking about all this, quite early in the setup process we
>>>>> have:
>>>>>
>>>>>           /* Figure out what features the device supports. */
>>>>>           device_features = dev->config->get_features(dev);
>>>>>
>>>>>
>>>>> isn't this sufficient? this will flush out
>>>>> all writes and device can defer responding to reads
>>>>> until reset is complete.
>>>> Hmm not sure if that's possible in all MMIO devices? I think a while back Jason
>>>> felt some of the MMIO devices, their registers could act as plain DRAM for
>>>> reads, which means device can't block such reads?
>>>>
>>>> - vatsa
>>> Is it worth bothering about theoretical issues though? Jason what is
>>> your take?
>>
>> Re-read the spec, and it said
>>
>> "
>>
>> MMIO Device Register Layout
>>
>> "
>>
>> I believe plain DRAM will not behave like a register. So we don't need to
>> worry about that.
>>
>> But they are indeed devices that act as a plain DRAM for their control path,
>> which might requires new transports[1].
>>
>> Another question is that what makes PCI differ from MMIO. (E.g PCI requires
>> a re-read but MMIO doesn't)
> I don't think PCI requires a re-read either.  IIRC from that meeting, it
> was intended for a hypervisor so it does not have to block the guest:
> the guest can do something else then re-read.
> We did similar things elsewhere.


Yes, but it has implication for the device implication.


>
> See
> https://issues.oasis-open.org/projects/VIRTIO/issues/VIRTIO-103
> for the original proposal.
>
> In any case, I would say status register is unlikely to be in regular
> DRAM, after all write has side effects.
> So before we move on I'd like to know whether we do something as drastic
> as incrementing the version number for a theoretical or practical
> benefit.


I agree.

Thanks


>
>
>> [1] https://lkml.org/lkml/2020/9/1/255
>>
>>
>>>
>>>> ---
>>>>
>>>> Qualcomm Innovation Center, Inc. is submitting the attached "feedback" as a
>>>> non-member to the virtio-dev mailing list for consideration and inclusion.

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

[Srivatsa Vaddagiri]

* Michael S. Tsirkin <mst@redhat.com> [2021-08-17 03:51:47]:

> So before we move on I'd like to know whether we do something as drastic
> as incrementing the version number for a theoretical or practical
> benefit.

We initially stumbled on this reset issue when doing some optimization in
Qualcomm Type-1 hypervisor for virtio. In our case, virtio frontend and backend
drivers are in separate VMs. The Android VM that hosts backend driver is
considered untrusted, which among other things meant front-end could see large
latencies for its MMIO register r/w requests (largely owing to scheduling
delays). In some cases, I measured 5-10 ms for a single MMIO register read or
write request. Few of the registers are accessed in hot-path (like
VIRTIO_MMIO_QUEUE_NOTIFY or VIRTIO_MMIO_INTERRUPT_ACK) which we wanted to be of
low-latency as much as possible.

The optimization we have to help reduce this latency is for hypervisor to
acknowledge MMIO writes without waiting on backend to respond. For example, when
VM writes to VIRTIO_MMIO_QUEUE_NOTIFY, it causes a trap and normally hypervisor
would have required to stall the vcpu until backend acknowledges it. In our
case, hypervisor would unblock the vcpu immediately after injecting an interrupt
to backend (to let backend know that there is a queue_notify event). Handling
writes to VIRTIO_MMIO_INTERRUPT_ACK was bit tricky but we managed that with few
changes in backend (especially any awareness backend had about front-end being
still in a interrupt handler). Similarly other registers that are written are
completely handled in hypervisor without requiring intervention from backend.

Handling reset is the only open issue we have, as guest triggering reset has
currently no provision to poll for reset completion and hypervisor itself cannot
handle reset completly. This is where we observed discrepancy between PCI vs
MMIO in handling reset, which we wanted to address with this discussion.

I think the option we discussed earlier of a new feature bit seems less
intrusive than incrementing MMIO version?

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

- vatsa

--

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

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

[Srivatsa Vaddagiri]

* Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> [2021-08-17 15:33:33]:

> Handling reset is the only open issue we have, as guest triggering reset has
> currently no provision to poll for reset completion and hypervisor itself cannot
> handle reset completly. This is where we observed discrepancy between PCI vs
> MMIO in handling reset, which we wanted to address with this discussion.
> 
> I think the option we discussed earlier of a new feature bit seems less
> intrusive than incrementing MMIO version?
> 
> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html

Alternately would it be fine to merge a change like this to Linux virtio_mmio
driver w/o requiring any further changes to spec?

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 56128b9..9db81e6 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -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))
+		msleep(1);
 }
 
 

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

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

[Jason Wang]

在 2021/8/17 下午8:48, Srivatsa Vaddagiri 写道:
> * Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> [2021-08-17 15:33:33]:
>
>> Handling reset is the only open issue we have, as guest triggering reset has
>> currently no provision to poll for reset completion and hypervisor itself cannot
>> handle reset completly. This is where we observed discrepancy between PCI vs
>> MMIO in handling reset, which we wanted to address with this discussion.
>>
>> I think the option we discussed earlier of a new feature bit seems less
>> intrusive than incrementing MMIO version?
>>
>> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> Alternately would it be fine to merge a change like this to Linux virtio_mmio
> driver w/o requiring any further changes to spec?
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 56128b9..9db81e6 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -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))
> +		msleep(1);
>   }
>   
>   


I think we still need some clarification on the spec.

Note that doing this, with an untrusted device the system tool like 
modprobe may block at this forever

We probably need a way to fail the reset.

VDUSE meets similar issue.

Thanks


>

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

[Jason Wang]

在 2021/8/17 下午6:03, Srivatsa Vaddagiri 写道:
> * Michael S. Tsirkin <mst@redhat.com> [2021-08-17 03:51:47]:
>
>> So before we move on I'd like to know whether we do something as drastic
>> as incrementing the version number for a theoretical or practical
>> benefit.
> We initially stumbled on this reset issue when doing some optimization in
> Qualcomm Type-1 hypervisor for virtio. In our case, virtio frontend and backend
> drivers are in separate VMs. The Android VM that hosts backend driver is
> considered untrusted, which among other things meant front-end could see large
> latencies for its MMIO register r/w requests (largely owing to scheduling
> delays). In some cases, I measured 5-10 ms for a single MMIO register read or
> write request. Few of the registers are accessed in hot-path (like
> VIRTIO_MMIO_QUEUE_NOTIFY or VIRTIO_MMIO_INTERRUPT_ACK) which we wanted to be of
> low-latency as much as possible.
>
> The optimization we have to help reduce this latency is for hypervisor to
> acknowledge MMIO writes without waiting on backend to respond. For example, when
> VM writes to VIRTIO_MMIO_QUEUE_NOTIFY, it causes a trap and normally hypervisor
> would have required to stall the vcpu until backend acknowledges it.


This looks not necessary. Qemu/KVM allows an eventfd to be installed 
here. Then we can unblock the vcpu immediately after event is signaled 
and we don't need to wait for the acknowledge.


> In our
> case, hypervisor would unblock the vcpu immediately after injecting an interrupt
> to backend (to let backend know that there is a queue_notify event). Handling
> writes to VIRTIO_MMIO_INTERRUPT_ACK was bit tricky but we managed that with few
> changes in backend (especially any awareness backend had about front-end being
> still in a interrupt handler). Similarly other registers that are written are
> completely handled in hypervisor without requiring intervention from backend.
>
> Handling reset is the only open issue we have, as guest triggering reset has
> currently no provision to poll for reset completion and hypervisor itself cannot
> handle reset completly. This is where we observed discrepancy between PCI vs
> MMIO in handling reset, which we wanted to address with this discussion.


It looks like we may suffer from similar issue for other untrusted 
devices like VDUSE.


>
> I think the option we discussed earlier of a new feature bit seems less
> intrusive than incrementing MMIO version?
>
> https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html


Using features will result some interesting question:

1) the drivers usually reset the device before feature negotiation
2) it means the driver must mandate this behavior even before feature 
negotiation is done

We don't have those issue if we increase the version (but it looks more 
intrusive).

Thanks


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

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

[Srivatsa Vaddagiri]

* Jason Wang <jasowang@redhat.com> [2021-08-18 10:54:37]:

> >I think the option we discussed earlier of a new feature bit seems less
> >intrusive than incrementing MMIO version?
> >
> >https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> 
> 
> Using features will result some interesting question:
> 
> 1) the drivers usually reset the device before feature negotiation
> 2) it means the driver must mandate this behavior even before feature
> negotiation is done
> 
> We don't have those issue if we increase the version (but it looks more
> intrusive).

Hmm ..are you suggesting that we read the Version register as part of reset code
and do the wait part only for v3 device? Otherwise we have the same issue as
feature bit with version change also. 

Can't the driver do the same with feature register also? Do poll if
WAIT_FOR_RESET_COMPLETION feature bit is set, which can happen before feature
negotiation is complete.

- vatsa

-- 
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: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jason Wang]

On Wed, Aug 18, 2021 at 1:15 PM Srivatsa Vaddagiri
<quic_svaddagi@quicinc.com> wrote:
>
> * Jason Wang <jasowang@redhat.com> [2021-08-18 10:54:37]:
>
> > >I think the option we discussed earlier of a new feature bit seems less
> > >intrusive than incrementing MMIO version?
> > >
> > >https://lists.oasis-open.org/archives/virtio-dev/202107/msg00168.html
> >
> >
> > Using features will result some interesting question:
> >
> > 1) the drivers usually reset the device before feature negotiation
> > 2) it means the driver must mandate this behavior even before feature
> > negotiation is done
> >
> > We don't have those issue if we increase the version (but it looks more
> > intrusive).
>
> Hmm ..are you suggesting that we read the Version register as part of reset code
> and do the wait part only for v3 device?

We only need to read the version once (see virtio_mmio_probe()). So it
could be something like

if (vm_dev->version == 0x3) {
    while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
        msleep(1);
}

> Otherwise we have the same issue as
> feature bit with version change also.
>
> Can't the driver do the same with feature register also? Do poll if
> WAIT_FOR_RESET_COMPLETION feature bit is set, which can happen before feature
> negotiation is complete.

The possible issue is that the feature negotiation is not done at that time.

Thanks

>
> - 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

* Jason Wang <jasowang@redhat.com> [2021-08-18 13:40:45]:

> > Hmm ..are you suggesting that we read the Version register as part of reset code
> > and do the wait part only for v3 device?
> 
> We only need to read the version once (see virtio_mmio_probe()). So it

Ok sorry saw that now.

> could be something like
> 
> if (vm_dev->version == 0x3) {
>     while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
>         msleep(1);
> }
> 
> > Otherwise we have the same issue as
> > feature bit with version change also.
> >
> > Can't the driver do the same with feature register also? Do poll if
> > WAIT_FOR_RESET_COMPLETION feature bit is set, which can happen before feature
> > negotiation is complete.
> 
> The possible issue is that the feature negotiation is not done at that time.

Fine let me take a stab at the next version which will have this:

* Introduce a status bit for device to indicate "reset in progress". This could
  be adopted by other transports also (for ex PCI).

* For v3 MMIO devices, driver, after initiating reset, should wait until
  status = 0 or device indicates "failure". What the driver does when it sees
  failure can be implementation defined (it can give up or try reset few more
  times)

- vatsa

-- 
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: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jason Wang]

On Wed, Aug 18, 2021 at 1:51 PM Srivatsa Vaddagiri
<quic_svaddagi@quicinc.com> wrote:
>
> * Jason Wang <jasowang@redhat.com> [2021-08-18 13:40:45]:
>
> > > Hmm ..are you suggesting that we read the Version register as part of reset code
> > > and do the wait part only for v3 device?
> >
> > We only need to read the version once (see virtio_mmio_probe()). So it
>
> Ok sorry saw that now.
>
> > could be something like
> >
> > if (vm_dev->version == 0x3) {
> >     while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
> >         msleep(1);
> > }
> >
> > > Otherwise we have the same issue as
> > > feature bit with version change also.
> > >
> > > Can't the driver do the same with feature register also? Do poll if
> > > WAIT_FOR_RESET_COMPLETION feature bit is set, which can happen before feature
> > > negotiation is complete.
> >
> > The possible issue is that the feature negotiation is not done at that time.
>
> Fine let me take a stab at the next version which will have this:
>
> * Introduce a status bit for device to indicate "reset in progress". This could
>   be adopted by other transports also (for ex PCI).

I may miss something discussion but any reason that the current
VIRTIO_MMIO_STATUS is not sufficient?

>
> * For v3 MMIO devices, driver, after initiating reset, should wait until
>   status = 0 or device indicates "failure". What the driver does when it sees
>   failure can be implementation defined (it can give up or try reset few more
>   times)

Yes, but let's wait a little bit to see if it is agreed by others.

Thanks

>
> - 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

* Jason Wang <jasowang@redhat.com> [2021-08-18 14:04:04]:

> > Fine let me take a stab at the next version which will have this:
> >
> > * Introduce a status bit for device to indicate "reset in progress". This could
> >   be adopted by other transports also (for ex PCI).
> 
> I may miss something discussion but any reason that the current
> VIRTIO_MMIO_STATUS is not sufficient?

I think Mike suggested we specify what device would return while reset is in
progress, rather than any arbitrary non-zero value:

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

> > * For v3 MMIO devices, driver, after initiating reset, should wait until
> >   status = 0 or device indicates "failure". What the driver does when it sees
> >   failure can be implementation defined (it can give up or try reset few more
> >   times)
> 
> Yes, but let's wait a little bit to see if it is agreed by others.

Sure. 

Mike,
	Do you suggest we add this as well for v3 device?

"after writing 0 to Status and before reading any fields, the driver
MUST read Status and wait for a read to return 0 or indicate failure"

- 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Srivatsa Vaddagiri]

* Srivatsa Vaddagiri <quic_svaddagi@quicinc.com> [2021-08-18 11:43:05]:

> * Jason Wang <jasowang@redhat.com> [2021-08-18 14:04:04]:
> 
> > > Fine let me take a stab at the next version which will have this:
> > >
> > > * Introduce a status bit for device to indicate "reset in progress". This could
> > >   be adopted by other transports also (for ex PCI).
> > 
> > I may miss something discussion but any reason that the current
> > VIRTIO_MMIO_STATUS is not sufficient?
> 
> I think Mike suggested we specify what device would return while reset is in
> progress, rather than any arbitrary non-zero value:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202108/msg00070.html
> 
> > > * For v3 MMIO devices, driver, after initiating reset, should wait until
> > >   status = 0 or device indicates "failure". What the driver does when it sees
> > >   failure can be implementation defined (it can give up or try reset few more
> > >   times)
> > 
> > Yes, but let's wait a little bit to see if it is agreed by others.
> 
> Sure. 

Does anyone disagree with above approach to go with MMIO version change to
handle the reset issue? I can send the next version based on it and addressing
various comments received so far.

> Mike,
> 	Do you suggest we add this as well for v3 device?
> 
> "after writing 0 to Status and before reading any fields, the driver
> MUST read Status and wait for a read to return 0 or indicate failure"


- 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: [virtio-dev] Re: [PATCH v1] virtio-mmio: Specify wait needed in driver during reset

[Jason Wang]

在 2021/8/16 下午1:35, Michael S. Tsirkin 写道:
> On Mon, Aug 16, 2021 at 10:09:34AM +0800, Jason Wang wrote:
>> 在 2021/8/11 下午6:05, Srivatsa Vaddagiri 写道:
>>> * Jason Wang <jasowang@redhat.com> [2021-08-02 14:06:38]:
>>>
>>>>> Maybe if it sees status was not read it can just
>>>>> stay in reset state and not exit it?
>>>>>
>>>>>
>>>> Or I wonder we can use increase the version instead.
>>> What should be the guidance for backend/devices here? Go with version 3 only if
>>> it requires driver to poll on reset? Otherwise it may break existing setups -
>>> lets say Qemu is updated to report version 3 which will break older guest images
>>> that understood version 2 - this is despite the same Qemu being capable of
>>> serving those guests.
>>
>> My understanding is that qemu should stick to version 2 since it works fine.
>>
>> Thanks
> Yes breaking all guests does not sound like a good plan.
> Which is unfortunate generally.
>
> So thinking about all this, quite early in the setup process we
> have:
>
>          /* Figure out what features the device supports. */
>          device_features = dev->config->get_features(dev);
>
>
> isn't this sufficient? this will flush out
> all writes and device can defer responding to reads
> until reset is complete.


This works unless we mandate the deferring in the spec. And the racy 
part is here:

         /* 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);

If we don't read, the reset can be finished after add_status()?

Thanks


>
> I know it's not elegant since there are a bunch of writes like
> acknowledge and driver, but hey.
>
> How about we just add a non-normative section explaining that trick?
>
>
>>> - vatsa
>>>
>>> ---
>>>
>>> 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: virtio-dev-unsubscribe@lists.oasis-open.org
>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>

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

[Michael S. Tsirkin]

On Fri, Aug 20, 2021 at 11:56:15AM +0800, Jason Wang wrote:
> 
> 在 2021/8/16 下午1:35, Michael S. Tsirkin 写道:
> > On Mon, Aug 16, 2021 at 10:09:34AM +0800, Jason Wang wrote:
> > > 在 2021/8/11 下午6:05, Srivatsa Vaddagiri 写道:
> > > > * Jason Wang <jasowang@redhat.com> [2021-08-02 14:06:38]:
> > > > 
> > > > > > Maybe if it sees status was not read it can just
> > > > > > stay in reset state and not exit it?
> > > > > > 
> > > > > > 
> > > > > Or I wonder we can use increase the version instead.
> > > > What should be the guidance for backend/devices here? Go with version 3 only if
> > > > it requires driver to poll on reset? Otherwise it may break existing setups -
> > > > lets say Qemu is updated to report version 3 which will break older guest images
> > > > that understood version 2 - this is despite the same Qemu being capable of
> > > > serving those guests.
> > > 
> > > My understanding is that qemu should stick to version 2 since it works fine.
> > > 
> > > Thanks
> > Yes breaking all guests does not sound like a good plan.
> > Which is unfortunate generally.
> > 
> > So thinking about all this, quite early in the setup process we
> > have:
> > 
> >          /* Figure out what features the device supports. */
> >          device_features = dev->config->get_features(dev);
> > 
> > 
> > isn't this sufficient? this will flush out
> > all writes and device can defer responding to reads
> > until reset is complete.
> 
> 
> This works unless we mandate the deferring in the spec. And the racy part is
> here:
> 
>         /* 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);
> 
> If we don't read, the reset can be finished after add_status()?
> 
> Thanks

It can, but VIRTIO_CONFIG_S_ACKNOWLEDGE is mostly for hypervisor's
benefit.  It's a basic system to debug the binding process.  If device
cares it can hold the write in a buffer while it's being reset.  If it
does not it can just drop this bit, nothing that's too bad will happen.

> 
> > 
> > I know it's not elegant since there are a bunch of writes like
> > acknowledge and driver, but hey.
> > 
> > How about we just add a non-normative section explaining that trick?
> > 
> > 
> > > > - vatsa
> > > > 
> > > > ---
> > > > 
> > > > 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: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > 

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

[Michael S. Tsirkin]

On Mon, Jul 26, 2021 at 01:03:02PM +0200, Cornelia Huck wrote:
> On Fri, Jul 23 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
> 
> > Changes since v0:
> >
> > Introduce a new feature bit, VIRTIO_F_MMIO_RESET_WAIT, which controls whether a driver polls
> > for reset completion or not.
> 
> Nit: changelog should go into a "---" section, next to the diffstat (git
> format-patch should give you the correct base to edit.)
> 
> >
> > ===
> >
> > Reset of a virtio-mmio device is accomplished by writing 0 to its Status register.
> > 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 the case for virtio-mmio devices implemented in hardware
> > directly. A write could complete before the reset operation inside device is
> > completed. Introduce a new feature, VIRTIO_F_MMIO_RESET_WAIT, that such devices
> > will offer as means to indicate to driver that they need to poll for reset
> > completion, indicated by a read of Status register returning 0.
> >
> > Signed-off-by: Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com>
> >
> > diff --git a/content.tex b/content.tex
> > index 5c70a3c..1990a5c 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -1924,7 +1924,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> >  do not represent events which took place MUST be zero.
> >
> >  Upon reset, the device MUST clear all bits in \field{InterruptStatus} and ready bits in the
> > -\field{QueueReady} register for all queues in the device.
> > +\field{QueueReady} register for all queues in the device. The device MUST also offer
> > +VIRTIO_F_MMIO_RESET_WAIT if it requires driver to poll for reset completion
> 
> s/driver/the driver/
> 
> > +indicated by a read of \field{Status} returning 0. Such a device MUST also fail to accept
> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
> 
> So, this basically means that an older driver that does not know the new
> feature bit will not work with devices offering this bit, even if it did
> poll? This seems acceptable, I just wanted to spell it out.

Maybe should or even may is enough here. For example it is reasonable to have
hypervisor block guest until reset is complete.


> >
> >  The device MUST change value returned in \field{ConfigGeneration} if there is any risk of a
> >  driver seeing an inconsistent configuration state.
> > @@ -1944,6 +1947,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> >  8 bit wide fields, 16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide and
> >  aligned accesses for 32 and 64 bit wide fields.
> >
> > +The driver MUST accept VIRTIO_F_MMIO_RESET_WAIT if offered by device. During
> 
> s/device/the device/
> 
> > +reset of such a device that offers VIRTIO_F_MMIO_RESET_WAIT, driver MUST poll for
> 
> s/driver/the driver/
> 
> But maybe use "if VIRTIO_F_MMIO_RESET_WAIT has been negotiated" instead?
> This is implied by the statements above, but the deciding factor here is
> that the feature has been negotiated, and not that it has been offered.
> 
> 
> > +reset completion indicated by a read of \field{Status} returning 0.
> > +
> >  The driver MUST ignore a device with \field{MagicValue} which is not 0x74726976,
> >  although it MAY report an error.
> >
> > @@ -6672,6 +6679,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >    transport specific.
> >    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
> >
> > +  \item[VIRTIO_F_MMIO_RESET_WAIT(40)] This feature could be offered by a device
> > +  using MMIO transport and indicates that the driver needs to poll for reset
> 
> "This feature is specific to the MMIO transport and indicates that the
> device requires the driver to poll..."
> 
> ?
> 
> > +  completion indicated by a read of \field{Status} returning 0.
> > +
> >  \end{description}
> >
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > @@ -6708,6 +6719,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >
> >  A driver SHOULD accept VIRTIO_F_NOTIF_CONFIG_DATA if it is offered.
> >
> > +A driver SHOULD accept VIRTIO_F_MMIO_RESET_WAIT if it is offered.
> > +
> 
> But this was specified as MUST above, wasn't it?
> 
> >  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >
> >  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further

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

[Cornelia Huck]

On Mon, Jul 26 2021, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 26, 2021 at 01:03:02PM +0200, Cornelia Huck wrote:
>> On Fri, Jul 23 2021, Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com> wrote:
>> > Reset of a virtio-mmio device is accomplished by writing 0 to its Status register.
>> > 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 the case for virtio-mmio devices implemented in hardware
>> > directly. A write could complete before the reset operation inside device is
>> > completed. Introduce a new feature, VIRTIO_F_MMIO_RESET_WAIT, that such devices
>> > will offer as means to indicate to driver that they need to poll for reset
>> > completion, indicated by a read of Status register returning 0.
>> >
>> > Signed-off-by: Srivatsa Vaddagiri <svaddagi@qti.qualcomm.com>
>> >
>> > diff --git a/content.tex b/content.tex
>> > index 5c70a3c..1990a5c 100644
>> > --- a/content.tex
>> > +++ b/content.tex
>> > @@ -1924,7 +1924,10 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>> >  do not represent events which took place MUST be zero.
>> >
>> >  Upon reset, the device MUST clear all bits in \field{InterruptStatus} and ready bits in the
>> > -\field{QueueReady} register for all queues in the device.
>> > +\field{QueueReady} register for all queues in the device. The device MUST also offer
>> > +VIRTIO_F_MMIO_RESET_WAIT if it requires driver to poll for reset completion
>> 
>> s/driver/the driver/
>> 
>> > +indicated by a read of \field{Status} returning 0. Such a device MUST also fail to accept
>> > +FEATURES_OK bit if driver does not negotiate VIRTIO_F_MMIO_RESET_WAIT.
>> 
>> So, this basically means that an older driver that does not know the new
>> feature bit will not work with devices offering this bit, even if it did
>> poll? This seems acceptable, I just wanted to spell it out.
>
> Maybe should or even may is enough here. For example it is reasonable to have
> hypervisor block guest until reset is complete.

Maybe MAY reject, and SHOULD if there's no mitigating mechnism
available?


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