OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: RE: [PATCH v2] Add device reset timeout field


> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Friday, October 8, 2021 3:31 PM
> 
> On Thu, Oct 07 2021, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Thursday, October 7, 2021 9:41 PM On Thu, Oct 07 2021, Parav
> >> Pandit <parav@nvidia.com> wrote:
> >>
> >> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> >> Sent: Thursday, October 7, 2021 2:23 AM On Wed, Oct 06, 2021 at
> >> >> 04:11:19PM +0000, Parav Pandit wrote:
> >> >> >
> >> >> > > From: Michael S. Tsirkin <mst@redhat.com>
> >> >> > > Sent: Wednesday, October 6, 2021 8:52 PM
> >>
> >> >> > > second, I don't see a story around compatibility here.
> >> >> > Device reset timeout is supported only when device offers the
> >> >> > new
> >> >> VIRTIO_DEV_RESET_TIMEOUT.
> >> >> > When such feature is not offered, driver follows the guidance
> >> >> > provided in
> >> >> this proposal.
> >> >> > Compatibility is covered with the feature bit.
> >> >>
> >> >> Well hmm. Problem is, devices need to be reset before feature bits are
> read.
> >> >>
> >> > Yeah, this means that we should remove the feature bit and instead
> >> > follow your suggestion that if the register value is zero, the
> >> > feature is not
> >> offered, If it is non zero, driver should follow the timeout.
> >>
> >> What about old devices -- they don't provide the timeout value at
> >> all, obviously; how can they stay compliant?
> >>
> > If the old device which doesn't provide a timeout, the newer version of the
> spec suggests that driver chooses a reasonable value.
> 
> If we don't guard the new field via a feature bit, the driver will try to access
> something that does not exist for older devices. If we say that the field exists,
> older devices are simply not compliant.
Lets take base line that for reset timeout we cannot add the feature bit because device may not be ready when reset is started.
With that baseline, 
If the field exists, driver should honor it.
If the field doesn't exists, it is old device. So driver chooses a reasonable value of timeout.

> 
> Now, the interaction with the feature bit is obviously a problem... it would
> mean that the driver can only find out what the device offers too late in the
> initial startup sequence. We could specify that the field may or may not exist,
> but that might lead to other problems down the road.
> 
Yes, newer spec will have the field. Value can be zero or non-zero.
Zero = device doesn't support reset timeout
Non_zero = device suggested the timeout

> >
> >> >
> >> >> > > previously pci drivers did wait, other drivers didn't.
> >> >> > >
> >> >> > The newer driver that implements updated spec, will adhere to
> >> >> > newer
> >> >> content of the spec.
> >> >>
> >> >> right but we can't just unconditionally say driver must wait, this
> >> >> would declare old driver non-compliant and we promised not to do that.
> >> >>
> >> > It is not said unconditionally. It comes with spec version, isn't it?
> >> > This change should increment the spec version so old driver are
> >> > compliant to
> >> some older version of the spec.
> >> > Hence, there is no breakage with some old driver.
> >>
> >> We do not really say that a driver or a device is conforming to a
> >> certain version of the spec, though. If it has been written to an
> >> older version of the spec, I still expect it to be compliant, it's just not
> implementing some newer things.
> >>
> > The only way a older driver following older spec can be compliant to
> > newer spec, if the new spec remains same as old spec. :-)
> 
> Eh, no. We just need to make new features optional. A driver written to an
> older version of the spec that does not implement e.g. packed ring is still
> compliant, since packed ring is guarded by a feature bit.
Ok. older driver will just simply ignore the existence of device reset timeout field.
Newer driver will honor the value if the field is non zero.
If its zero, spec suggested newer driver to choose its reasonable value.
Since it is not a must/conformance statement, older driver continue to ignore the field that it doesn't know about.

> 
> > However, the spec is suggesting to have finite timeout as "SHOULD".
> > Timeout is not part of the conformance _must_ clause.
> > So if old driver not following "SHOULD" suggestion of newer spec, does it
> make old driver non-compliant?
> > Your input on this will help me understand the conformance part.
> 
> This depends upon how we introduce the new feature. If (as had been
> suggested upthread) we ditch the feature bit, we specify that the driver needs
> to do something (read the value) which an old driver simply won't do. The only
> way that can work is if we make it optional for the driver to read the timeout
> value. So yes, a driver not following a SHOULD clause is still compliant. We
> need to be careful as to what we actually require; normally, feature bits solve
> this neatly, but it seems problematic here.
> 
True, feature bit solves this neatly as you suggested but unfortunately feature bit is not useful here.
So lets proceed without feature bit as optional for driver.


> >> >> The idea is to have finite behaviour, I got that, but I don't
> >> >> really know where does the requirement come from, in what sense
> >> >> it's deterministic and why do we worry about reset specifically.
> >> >>
> >> >> Recovering from failure when hardware is broken?
> >> >>
> >> > I provided few examples in previous email.
> >> > Terminating a probe sequence of the device which is already on its
> >> > path of
> >> hotunplug.
> >>
> >> For that sequence, don't we need a timeout for *any* interaction with
> >> the device?
> > We do need a command timeout that will handle hotunplug scenario while
> under probe.
> > But handling command timeout has wider use, even without non racing
> hotunplug and plug sequences.
> > So it is better to drive it as separate extension in itself, which is self-
> contained.
> 
> So, why is reset special in that case? As...
> 
Normally composing a device takes a longer or more efforts for the backend compare to serving a command.
So device reset timeout is likely higher than a command timeout value.
So overloading or defining single field for command timeout and reset timeout looks incorrect to me.

Hence, command timeout should be a different feature bit. Given that it is own feature bit, I propose to keep such proposal self-contained and separate from this one.

> >
> >> I thought this proposal was mainly about devices that take a long
> >> time to "warm up"...
> >>
> > Yes, certainly this proposal covers such device scenario.
> > In general, this proposal covers reset timeout as the register suggests.
> 
> ...I do not really understand this, then. We want reset timeouts, because they
> cover a certain scenario, but we don't want something more general?

A command timeout is a new feature bit and a different field than reset timeout.
Lets not combine both into single proposal and single field.

> 
> >
> >> > VQ commands timeout handling is indeed needed but it is a separate
> >> enhancement in itself.
> >> > This one is self-contained enough that doesn't need to combined
> >> > with
> >> command timeout.
> >>
> >> What about e.g. config writes; I think they might trigger some actions?
> >>
> > A driver can do config writes to the PCI virtio device without reset is
> complete.
> > This is very common practice that driver follows to discover device
> properties.
> > It may trigger some action on the device, but PCI device is obligated to
> respond to it for host driver to function properly.
> 
> But isn't that in violation of the spec? I understand it needs to do reset -> (wait
> until complete) -> set ACKNOWLEDGE status -> set DRIVER status -> potentially
> *read* the config space -> set FEATURES_OK and only then *write* to the
> config space?
> 
I think I misunderstood your point about "config space".
Since I am mostly familiar with pci devices, I took "config space" as "pci config space", which must be accessible to the driver before device virtio level registers are accessed.

You mentioned virtio level config space, which as you say, has to be accessed after reset is completed.

> As an aside, not all transports even allow accessing the config space while reset
> is still in progress. I would naively have expected that to be the norm.
True. My bad on interpreting virtio config space = pci config space.


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]