Re: [PATCH V5 17/25] vfio-pci: cpr part 2

[Zheng Chuan]

Hi

On 2021/7/20 2:38, Steven Sistare wrote:
> On 7/19/2021 2:10 PM, Alex Williamson wrote:
>> On Mon, 19 Jul 2021 13:44:08 -0400
>> Steven Sistare <steven.sistare@oracle.com> wrote:
>>
>>> On 7/16/2021 4:51 PM, Alex Williamson wrote:
>>>> On Wed,  7 Jul 2021 10:20:26 -0700
>>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>   
>>>>> Finish cpr for vfio-pci by preserving eventfd's and vector state.
>>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> ---
>>>>>  hw/vfio/pci.c | 118 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 116 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>>> index 0f5c542..07bd360 100644
>>>>> --- a/hw/vfio/pci.c
>>>>> +++ b/hw/vfio/pci.c  
>>>> ...  
>>>>> @@ -3295,14 +3329,91 @@ static void vfio_merge_config(VFIOPCIDevice  
>>>> *vdev)  
>>>>>      g_free(phys_config);
>>>>>  }
>>>>>  
>>>>> +static int vfio_pci_pre_save(void *opaque)
>>>>> +{
>>>>> +    VFIOPCIDevice *vdev = opaque;
>>>>> +    PCIDevice *pdev = &vdev->pdev;
>>>>> +    int i;
>>>>> +
>>>>> +    if (vfio_pci_read_config(pdev, PCI_INTERRUPT_PIN, 1)) {
>>>>> +        error_report("%s: cpr does not support vfio-pci INTX",
>>>>> +                     vdev->vbasedev.name);
>>>>> +    }  
>>>>
>>>> You're not only not supporting INTx, but devices that support INTx, so
>>>> this only works on VFs.  Why?  Is this just out of scope or is there
>>>> something fundamentally difficult about it?
>>>>
>>>> This makes me suspect there's a gap in INTx routing setup if it's more
>>>> than just another eventfd to store and setup.  If we hot-add a device
>>>> using INTx after cpr restart, are we going to find problems?  Thanks,  
>>>
>>> It could be supported, but requires more code (several event fd's plus 
>>> other state in VFIOINTx
>>> to save and restore) for a case that does not seem very useful (a directly 
>>> assigned device that
>>> only supports INTx ?). 
>>
>> It's not testing that the device *only* supports INTx, it's testing
>> that the device supports INTx _at_all_.  That effectively means this
>> excludes anything other than an SR-IOV VF.  There are plenty of valid
>> and useful cases of assigning PFs, most of which support INTx even if
>> we don't expect that's their primary operational mode.  Thanks,
> 
> OK, I'll look into it.  If this proves problematic, how do you feel about 
> deferring
> INTx support to a later patch?
> 
I am curious about that does cpr restart mode work for GPU passthrough?
> - Steve
> 
> .
> 

-- 
Regards.
Chuan