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

["Michael S. Tsirkin" <mst@redhat.com>]

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