Re: [PATCH v6 1/1] content: Add new feature VIRTIO_F_PRESERVE_RESOURCES

["Chen, Jiqian" <Jiqian.Chen@amd.com>]

On 2023/10/24 18:51, Parav Pandit wrote:
> 
>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>> Sent: Tuesday, October 24, 2023 4:06 PM
>>
>> On 2023/10/23 21:35, Parav Pandit wrote:
>>>
>>>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>>>> Sent: Monday, October 23, 2023 4:09 PM
>>>
>>>>> I think this can be done without introducing the new register.
>>>>> Can you please check if the PM register itself can serve the purpose
>>>>> instead
>>>> of new virtio level register?
>>>> Do you mean the system PM register?
>>> No, the device's PM register at transport level.
>> I tried to find this register(pci level or virtio pci level or virtio driver level), but I
>> didn't find it in Linux kernel or Qemu codes.
>> May I know which register you are referring to specifically? Or which PM state
>> bit you mentioned below?
>>
> PCI spec's PCI Power Management Capability Structure in section 7.5.2.
Yes, what you point to is PM capability for PCIe device. 
But the problem is still that in Qemu code, it will check the condition(pci_bus_is_express or pci_is_express) of all virtio-pci devices in function virtio_pci_realize(), if the virtio devices aren't a PCIe device, it will not add PM capability for them.
And another problem is how about the MMIO transport devices? Since preserve resources is need for all transport type devices.

>>>
>>>> I think it is unreasonable to let virtio- device listen the PM state
>>>> of Guest system.
>>> Guest driver performs any work on the guest systems PM callback events in
>> the virtio driver.
>> I didn't find any PM state callback in the virtio driver.
>>
> There are virtio_suspend and virtio_resume in case of Linux.
I think what you said virtio_suspend/resume is freeze/restore callback from "struct virtio_driver" or suspend/resume callback from "static const struct dev_pm_ops virtio_pci_pm_ops".
And yes, I agree, if virtio devices have PM capability, maybe we can set PM state in those callback functions.

> 
>>>
>>>> It's more suitable that each device
>>>> gets notifications from driver, and then do preserving resources operation.
>>> I agree that each device gets the notification from driver.
>>> The question is, should it be virtio driver, or existing pci driver which
>> transitions the state from d0->d3 and d3->d0 is enough.
>> It seems there isn't existing pci driver to transitions d0 or d3 state. Could you
>> please tell me which one it is specifically? I am very willing to give a try.
>>
> Virtio-pci modern driver of Linux should be able to.
Yes, I know, it is the VIRTIO_PCI_FLAG_INIT_PM_BIT. But still the two problems I said above.

> 
>>> Can you please check that?
>>>
>>>>>> --- a/transport-pci.tex
>>>>>> +++ b/transport-pci.tex
>>>>>> @@ -325,6 +325,7 @@ \subsubsection{Common configuration structure
>>>>>> layout}\label{sec:Virtio Transport
>>>>>>          /* About the administration virtqueue. */
>>>>>>          le16 admin_queue_index;         /* read-only for driver */
>>>>>>          le16 admin_queue_num;         /* read-only for driver */
>>>>>> +        le16 preserve_resources;        /* read-write */
>>>>> Preserving these resources in the device implementation takes finite
>>>>> amount
>>>> of time.
>>>>> Possibly more than 40nsec (time of PCIe write TLP).
>>>>> Hence this register must be a polling register to indicate that
>>>> preservation_done.
>>>>> This will tell the guest when the preservation is done, and when
>>>>> restoration is
>>>> done, so that it can resume upper layers.
>>>>>
>>>>> Please refer to queue_reset definition to learn more about such
>>>>> register
>>>> definition.
>>>> Thanks, I will refer to "queue_reset". So, I need three values,
>>>> driver write 1 to let device do preserving resources, driver write 2
>>>> to let device do restoring resources, device write 0 to tell driver
>>>> that preserving or restoring done, am I right?
>>>>
>>> Right.
>>>
>>> And if the existing pcie pm state bits can do, we can leverage that.
>>> If it cannot be used, lets add that reasoning in the commit log to describe this
>> register.
>>>
>>>>>
>>>>> Lets please make sure that PCIe PM level registers are
>>>>> sufficient/not-sufficient
>>>> to decide the addition of this register.
>>>> But if the device is not a PCIe device, it doesn't have PM
>>>> capability, then this will not work. Actually in my local
>>>> environment, pci_is_express() return false in Qemu, they are not PCIe
>> device.
>>> It is reasonable to ask to plug in as PCIe device in 2023 to get new
>>> functionality that too you mentioned a gpu device. ð
>>> Which does not have very long history of any backward compatibility.
>> Do you suggest me to add PM capability for virtio-gpu or change virtio-gpu to a
>> PCIe device?
>>
> PCI Power Management Capability Structure does not seem to be limited to PCIe.
I am not sure, but in current Qemu code, I can see the check "pci_is_express" for all virtio-pci devices. If we want to add PM capability for virtio-pci devices, we need to change them to PCIe device I think.

> 

-- 
Best regards,
Jiqian Chen.