Re: [PATCH v2] Add device reset timeout field

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

On Thu, Oct 14, 2021 at 05:35:37PM +0000, Parav Pandit wrote:
> Hi Michael, Cornelia,	
> 
> > From: Parav Pandit
> > Sent: Tuesday, October 12, 2021 2:42 PM
> > 
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, October 12, 2021 2:32 PM
> > >
> > > On Tue, Oct 12, 2021 at 08:51:34AM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Monday, October 11, 2021 9:30 PM
> > > > >
> > > > > On Mon, Oct 11, 2021 at 03:44:14PM +0000, Parav Pandit wrote:
> > > > > > > > This is unlikely to work the reset is completed. Because a
> > > > > > > > real device
> > > > > > > implementing this would prefer to do this in fw for 1000
> > > > > > > virtio devices sitting on the physical card.
> > > > > > > > And it is very much driven by such implementation at device devel.
> > > > > > > > So it cannot update the counter value if reset is not
> > > > > > > > completed for the
> > > > > device.
> > > > > > > >
> > > > > > > > I think read only device reset timeout is most elegant
> > > > > > > > option during device
> > > > > > > initialization phase that eliminates infinite loop of today.
> > > > > > >
> > > > > > > Why can't a driver just go ahead and do a timeout regardless?
> > > > > > o.k. lets consider this thought exercise. What is the timeout
> > > > > > value that driver
> > > > > will choose if device doesn't specify one?
> > > > > > I explained in previous thread and you acked that actual fw
> > > > > > based device
> > > > > may take longer to initialize than pure sw implementation backend.
> > > > > > In second example a pre-boot device can take even longer
> > > > > > initialization
> > > time.
> > > > > > Sriov VF device may initialize lot faster.
> > > > > > Instead of driver having such transport, and device specific
> > > > > > checks, (or some
> > > > > very short or very long timeout), we propose, that let device
> > > > > mention such timeout value.
> > > > >
> > > > > Parav I think you are conflating reset with initialization time.
> > > > > initialization is just for host boot which takes seconds anyway -
> > > > > but no, minutes is not reasonable their, either.
> > > > > reset affects guest boot. This needs to complete in milliseconds.
> > > > >
> > > > I cannot promise, but with newer generation devices usually
> > > > functionality
> > > improves.
> > > > Enforcing in milliseconds doesn't look practical for type of devices.
> > > > Some of the block devices may need to establish TCP connections in
> > > > the
> > > backend.
> > > > It is more useful to wait for few more seconds to initialize device
> > > > after power
> > > on the system, instead of giving up booting the server completely.
> > > > For example, a nvme block device starts with a minimum timeout of
> > > 500msec.
> > > >
> > > > Yes, I agree to your point that a device given to a guest VM will
> > > > likely have
> > > very short reset time that should complete in milliseconds.
> > > >
> > > > > This conflation is IMHO one of the problems with this proposal.
> > > >
> > > > Device initialization consist of device reset from the spec section 3.1.1.
> > >
> > > It does. But maybe we need to create a way for driver to distinguish
> > > between the two. When under reset, use a driver supplied timeout.
> > This make sense, because as we discussed when device undergo a reset with
> > active DMA, after timeout expires, driver still cannot cleanup.
> > So this can be short driver decided value as longer timeout is not useful.
> > 
> > > When powering up, use a longer device supplied one.
> > In v0, v1 I initially considered only the powering up case of the device
> > initialization. There was text around that.
> > And v2 I removed the initialization text, and I totally missed the above case with
> > active DMA.
> > This should work.
> > We should word this part of the spec accordingly.
> 
> Below changes are good for v3?
> 1. driver should use device reset time during initialization stage

How does driver identify this though?

> 2. remove feature bit as feature bits are only readable after reset is completed
> 3. device reset timeout field of zero indicates that device doesn't support it.

I'm not sure about 3. I think each transport will need its own way to do it.

So I propose: maybe a capability like this, with a timeout field?
And within VMs, we can just do without, since it got out of reset once
it will surely get out of reset again...

-- 
MST