On 5/17/24 04:08, Chen, Jiqian wrote:
> On 2024/5/16 21:08, Jan Beulich wrote:
>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>> @@ -67,6 +68,41 @@ ret_t pci_physdev_op(int cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> + 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);
>>> + pcidevs_unlock();
>>
>> Can't this be done right after the write_lock()?
> You are right, I will move it in next version.
So that we are clear on the proposed order of calls here:
+ write_lock(&pdev->domain->pci_lock);
+ pcidevs_unlock();
+ ret = vpci_reset_device_state(pdev);
+ write_unlock(&pdev->domain->pci_lock);
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -115,6 +115,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>
>>> return rc;
>>> }
>>> +
>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>> +{
>>> + ASSERT(pcidevs_locked());
>>
>> Is this necessary for ...
>>
>>> + ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>>> +
>>> + vpci_deassign_device(pdev);
>>> + return vpci_assign_device(pdev);
>>
>> ... either of these calls? Both functions do themselves only have the
>> 2nd of the asserts you add.
> I checked codes again; I will remove the two asserts here in next version.
Nit: I think it's okay to keep the
ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
