[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 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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]