RE: [PATCH v2] Add device reset timeout field

[Parav Pandit <parav@nvidia.com>]

Hi Cornelia,

I wasn't able to respond last few days.
Sorry for the delayed response below.

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Friday, October 15, 2021 2:56 PM

> On Fri, Oct 15 2021, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Friday, October 15, 2021 12:22 PM
> >>
> >> On Fri, Oct 15 2021, Parav Pandit <parav@nvidia.com> wrote:
> >>
> >> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> >> Sent: Friday, October 15, 2021 3:59 AM
> >> >>
> >> >> On Thu, Oct 14, 2021 at 05:35:37PM +0000, Parav Pandit wrote:
> >> >> > Below changes are good for v3?
> >> >> > 1. driver should use device reset time during initialization
> >> >> > stage
> >> >>
> >> >> How does driver identify this though?
> >> > Existence of device_reset_timeout field in struct
> >> > virtio_pci_common_cfg
> >> indicates that this field exists.
> >> > If device support it, it will place non zero value and driver knows
> >> > that this
> >> field should be used.
> >>
> >> But how does the driver know? The very first step in the
> >> initialization sequence is "reset the device", before it may read the config
> space.
> > PCI configuration space is always accessible when a PCI device exists at PCI
> level even before virtio layer can operate it.
> > So struct virtio_pci_common_cfg is accessible to the virtio transport layer
> before resetting the device.
> > It needs to contains what the timeout is, just like reset of the fields.
> >
> > I made mistake in understanding last time between virtio config space such
> as (struct virtio_net_config) vs PCI config space.
> > So want to double check, did you mean pci config space and struct
> virtio_pci_common_cfg or something else when you mentioned
> _config_space_?
> > Virtio_net_config certainly accessible only after reset.
> > But transport level pci capabilities are accessible before reset by the PCI
> spec.
> 
> So, how does that work for non-pci? Can mmio access its config space like
> that?
Yes. MMIO device initialization starts in transport layer.
This device init starts by reading the magicvalue and version registers from the MMIO config space.
After this is done, it follows the device init flow described in the spec section 4.2.3.1.1.

> 
> My point is: don't make it harder for future transports than really neccessary. If
> you look at today's transports, ccw is already substantially different from pci in
> many ways, and yet both are functional virtio transports. Take the concept of
> "config space": ccw devices do not have such a thing. I created two channel
> commands to basically emulate that concept for *virtio* *only*. Should we
> want to introduce the timeout value for ccw (which is not likely, but let's go for
> it for the sake of the argument), we cannot put it into a non-existing config
> space; it needs to use something else, like a command to retrieve the value. A
> future transport might be in a similar position, and "put it into the config
> space" may simply not be possible.
> 
> Look at it that way: You come from the PCI perspective, and you have an idea
> how to model things there. I come from a channel I/O background, and I have
> an idea how to model things there. Both are quite different; if we can come up
> with something that can work for both transports, chances are good that a
> future transport can also work with it. 

In this proposal, for pci and mmio transports, the new timeout register is located in the transport specific device config space.
This is because these two transports initialization is based on this config space.
Both the transport clearly describe that they follow the spec 3.1 device initialization flow.

For ccw as you well described above and as part of the spec there is no link to spec 3.1 device initialization flow.

> If we say that the driver retrieves that
> information in a transport-specific way without any mention of config spaces,
> we should be fine.
Yes. I totally agree with you.
Its transport specific way.
In the current proposal, in the device reset timeout description section 2.4.1, 2.4.2, there is no mention of config space.
Each transport (pci, mmio) extends their own config space and describes what is the transport specific extension it is to interact with device.

So if new non config space based transport or ccw wants to do this in future, they will use their transport specific way (as you described channel command interface).

I didn't muddy the device reset description for ccw exceptions because for rest the device reset piece, it doesn't have any link to ccw differences.