[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [PATCH v2] Add device reset timeout field
On Fri, Oct 15 2021, Parav Pandit <firstname.lastname@example.org> wrote: >> From: Cornelia Huck <email@example.com> >> Sent: Friday, October 15, 2021 12:22 PM >> >> On Fri, Oct 15 2021, Parav Pandit <firstname.lastname@example.org> wrote: >> >> >> From: Michael S. Tsirkin <email@example.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? > >> >> > >> >> >> >> > 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. >> >> >> > For pci a value of zero indicates it isn't supported. >> > For mmio DeviceResetTimeout at offset 0x04c indicates same. >> > Currently only these 2 transports have the use. >> >> I don't think we want to force all transports, including not yet existing ones, to >> use that mechanism. They might as well use a command to retrieve the value >> and fail the command, for example. >> > ok. it is interesting for me to understand that virtio spec care for even an undefined transport. > > So when such undefined transport gets defined in virtio spec, a spec will get updated anyway to describes that new_transport performs reset via cmd interface. > > Though I do not well understand how driver can reset a device via a command without establishing a command channel. > A device usually needs to respond/accept the command channel bits to accept the command. [As a side note: s390 channel I/O works just like that. You have the channel subsystem (and the control units) take care of the mechanism, no device involvement required until the command is actually processed, and even that is not needed for all commands.] > But there may be some transport that behave differently that I didn't anticipate yet. > > So let's define such transport specific changes when such transport will be done for real in virtio spec. 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. If we say that the driver retrieves that information in a transport-specific way without any mention of config spaces, we should be fine.