Re: [RFC XEN PATCH v4 1/5] xen/vpci: Clear all vpci status of device

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/1/10 22:09, Stewart Hildebrand wrote:
> On 1/10/24 01:24, Chen, Jiqian wrote:
>> On 2024/1/9 23:24, Stewart Hildebrand wrote:
>>> On 1/5/24 02:09, Jiqian Chen wrote:
>>>> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
>>>> index 42db3e6d133c..552ccbf747cb 100644
>>>> --- a/xen/drivers/pci/physdev.c
>>>> +++ b/xen/drivers/pci/physdev.c
>>>> @@ -67,6 +68,39 @@ ret_t pci_physdev_op(int cmd, 
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>          break;
>>>>      }
>>>>  
>>>> +    case PHYSDEVOP_pci_device_state_reset: {
>>>> +        struct physdev_pci_device dev;
>>>> +        struct pci_dev *pdev;
>>>> +        pci_sbdf_t sbdf;
>>>> +
>>>> +        if ( !is_pci_passthrough_enabled() )
>>>> +            return -EOPNOTSUPP;
>>>> +
>>>> +        ret = -EFAULT;
>>>> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
>>>> +            break;
>>>> +        sbdf = PCI_SBDF(dev.seg, dev.bus, 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);
>>>
>>>> +        ret = vpci_reset_device_state(pdev);
>>>
>>>         write_unlock(&pdev->domain->pci_lock);
>> vpci_reset_device_state only reset the vpci state of pdev without deleting 
>> pdev from domain, and here has held pcidevs_lock, it has no need to lock 
>> pci_lock?
> 
> Strictly speaking, it is not enforced yet. However, an upcoming change [1] 
> will expand the scope of d->pci_lock to include protecting the pdev->vpci 
> structure to an extent, so it will be required once that change is committed. 
> In my opinion there is no harm in adding the additional lock now. If you 
> prefer to wait I would not object, but in this case I would at least ask for 
> a TODO comment to help remind us to address it later.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2024-01/msg00446.html
> 
Ok, I see. I will add pci_lock in next version, thank you for reminding me.

>>
>>>
>>>> +        pcidevs_unlock();
>>>> +        if ( ret )
>>>> +            printk(XENLOG_ERR "%pp: failed to reset PCI device state\n", 
>>>> &sbdf);
>>>> +        break;
>>>> +    }
>>>> +
>>>>      default:
>>>>          ret = -ENOSYS;
>>>>          break;
>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>> index 72ef277c4f8e..3c64cb10ccbb 100644
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -107,6 +107,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>>>  
>>>>      return rc;
>>>>  }
>>>> +
>>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>>> +{
>>>> +    ASSERT(pcidevs_locked());
>>>
>>>     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>>>
>>>> +
>>>> +    vpci_remove_device(pdev);
>>>> +    return vpci_add_handlers(pdev);
>>>> +}
>>>> +
>>>>  #endif /* __XEN__ */
>>>>  
>>>>  static int vpci_register_cmp(const struct vpci_register *r1,
>>

-- 
Best regards,
Jiqian Chen.