Re: [XEN PATCH v11 1/8] xen/vpci: Clear all vpci status of device

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.07.2024 04:59, Chen, Jiqian wrote:
> On 2024/7/1 15:18, Jan Beulich wrote:
>> On 30.06.2024 14:33, Jiqian Chen wrote:
>>> When a device has been reset on dom0 side, the vpci on Xen
>>> side won't get notification, so the cached state in vpci is
>>> all out of date compare with the real device state.
>>> To solve that problem, add a new hypercall to clear all vpci
>>> device state. When the state of device is reset on dom0 side,
>>> dom0 can call this hypercall to notify vpci.
>>
>> While the description properly talks about all of this being about device
>> reset, the title suggests otherwise (leaving open what the context is, thus
>> - to me at least - suggesting it's during vPCI init for a particular
>> device).
> Change title to "xen/pci: Add hypercall to support reset of pcidev" ?

Perhaps.

>>> @@ -67,6 +68,63 @@ ret_t pci_physdev_op(int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          break;
>>>      }
>>>  
>>> +    case PHYSDEVOP_pci_device_state_reset:
>>> +    {
>>> +        struct pci_device_state_reset dev_reset;
>>> +        struct pci_dev *pdev;
>>> +        pci_sbdf_t sbdf;
>>> +
>>> +        ret = -EOPNOTSUPP;
>>> +        if ( !is_pci_passthrough_enabled() )
>>> +            break;
>>> +
>>> +        ret = -EFAULT;
>>> +        if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
>>> +            break;
>>> +
>>> +        sbdf = PCI_SBDF(dev_reset.dev.seg,
>>> +                        dev_reset.dev.bus,
>>> +                        dev_reset.dev.devfn);
>>> +
>>> +        ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
>>> +        if ( ret )
>>> +            break;
>>> +
>>> +        pcidevs_lock();
>>> +        pdev = pci_get_pdev(NULL, sbdf);
>>> +        if ( !pdev )
>>> +        {
>>> +            pcidevs_unlock();
>>> +            ret = -ENODEV;
>>> +            break;
>>> +        }
>>> +
>>> +        write_lock(&pdev->domain->pci_lock);
>>> +        pcidevs_unlock();
>>> +        /* Implement FLR, other reset types may be implemented in future */
>>
>> The comment isn't in sync with the code anymore.
> Change to "/* vpci_reset_device_state is called by default for all reset 
> types, other specific operations can be added later as needed */" ?

Counter question: Is such a comment really adding any value?

Jan