RE: [PATCH v2] Add device reset timeout field

[Parav Pandit <parav@nvidia.com>]

> 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.

> 
> >
> >>
> >> > 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.
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.

> >
> >> So I propose: maybe a capability like this, with a timeout field?
> > Do you mean a new capability like say VIRTIO_PCI_DEVICE_TIMEOUT like
> VIRTIO_PCI_CAP_COMMON_CFG?
> > This will contain one or more timeout? For example with his proposal it
> contains only device reset timeout.
> > Later same capability will be further extended to contains command timeout
> too? Yes?
> >
> >> And within VMs, we can just do without, since it got out of reset
> >> once it will surely get out of reset again...
> > Yes, VM might not need it. It is really the HV's choice to implement and not
> part of the virtio spec.
> > Our internal cloud passthrough a PF to the VM.
> > It is probably better to let HV to choose if they want to do ctrl+c or have
> timeout.
> 
> But you want it in the spec, don't you? Confused.
It is a SHOULD area of the proposal that recommends to honor the timeout honored.
What I meant to say, that I agree to Michaels point that "VM can do without timeout" -> that a HV can build a device or pass a device to the VM without device reset timeout functionality.