Re: [virtio-dev] Re: [PATCH v4 1/1] Add suspend support for virtio PCI devices

On 2/16/2024 4:56 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 16, 2024 at 05:24:32PM +0900, David Stevens wrote:
>> Add a virtio power management PCI capability to allow drivers to suspend
>> virtio PCI devices. This allows drivers to suspend devices at the virtio
>> level before suspending them at the PCI transport layer. This allows
>> drivers to do a two phase suspend, which prevents notifications from
>> being ignored or lost if interrupts are reconfigured at the PCI
>> transport layer immediately before or after the device is put into the
>> PCI PM D3 low power state.
>> ---
>>   transport-pci.tex | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index a5c6719ea871..ce77708a9b69 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -188,6 +188,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>>   #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
>>   /* Vendor-specific data */
>>   #define VIRTIO_PCI_CAP_VENDOR_CFG        9
>> +/* Power management configuration */
>> +#define VIRTIO_PCI_CAP_PM_CFG            10
>>   \end{lstlisting}
>>   
>>           Any other value is reserved for future use.
>> @@ -804,6 +806,55 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
>>   specified by some other Virtio Structure PCI Capability
>>   of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
>>   
>> +\subsubsection{Power management capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Power management capability}
>> +
>> +The VIRTIO_PCI_CAP_PM_CFG capability refers to a single byte. The
>> +driver can write to the byte to set the power state of the device,
>> +and it can read from the byte to get the current power state of the
>> +device.
>> +
>> +The valid power states are:
>> +
>> +\begin{lstlisting}
>> +/* Device is operating normally */
>> +#define VIRTIO_PM_STATE_ACTIVE    0
>> +/* Device operation is suspended */
>> +#define VIRTIO_PM_STATE_SUSPENDED 1
>> +\end{lstlisting}
>> +
>> +The device power state has no effect when \field{device status} does
>> +not have the DRIVER_OK bit set or does have the DEVICE_NEEDS_RESET bit
>> +set.
>
> Given this is only after DRIVER_OK, wouldn't a feature bit + status bit
> make more sense? This will make it transport-independent and simplify
> discovery.
I agree with Michael, I have ever proposed a solution suspending the device
by setting _SUSPEND in the device_status field. I will send this suspending
patch again(will cc you) in a few days.

Thanks
Zhu Lingshan
>
>> +\devicenormative{\paragraph}{Power management capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Power management capability}
>> +
>> +A device MUST maintain its state while suspended such that all driver
>> +visible state after resuming exactly matches driver visible state
>> +before suspending.
>> +
>> +A device MUST NOT send notifications while suspended.
>> +
>> +A device MAY operate on any buffers in its virtqueue while suspended.
> How is this reconsiled with state matching exactly? buffers are
> driver-visible ...
>
>> +A device MUST set its power state to VIRTIO_PM_STATE_ACTIVE on reset.
>> +
>> +A device SHOULD take steps to minimize its resource consumption while
>> +suspended, although what this involves is specific to the particular
>> +device implementation.
>> +
>> +\drivernormative{\paragraph}{Power management capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Power management capability}
>> +
>> +A driver MUST NOT access a suspended device's BARs corresponding to
>> +any virtio structures, except for the power management byte.
>> +
>> +A driver MAY suspend a device that has buffers in one of its
>> +virtqueues, but it MUST NOT modify any such buffers while the device
>> +is suspended.
>> +
>> +A driver MUST read from the power management byte after writing to the
>> +byte to verify that the device successfully entered the target power
>> +state.
> Verify how? By checking the value returned? And what should it do with the value
> does not match?
>
>
>> +
>>   \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>>   
>>   Transitional devices MUST present part of configuration
>> -- 
>> 2.44.0.rc0.258.g7320e95886-goog
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]