[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v8 02/13] vpci: use per-domain PCI lock to protect vpci structure


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Date: Thu, 27 Jul 2023 10:31:56 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=zLa84VPEgrBJX2PUmcYAtZebknww0/wNa0EDs0+4GFg=; b=QQ6dY3K+D+s+OVu8vrc5mOwob9Q4TvO7SVKveWkNSq9m55t75XwKU62hbt+2AZvPUcZZSbKEetms9M6ux9o2ehxUU/xYbgFun/vJ3FJhIy7frQ9z5Pdyotwn0MdZ05lvvpXYqkxTq7yR0rIi/LkuBJ4e+5Ro+HhrkMcILuB3mvcNecZYnJL5eHI4kzvf3SvH+RbJ8Sx+yfLzvVmhsDHe1D/q66MTd6DvNiHO+z0DcdWp5hzLziCa1edUYR8DCCKdyXNMVXxEzjEoL0Gp3Y2KlIj/hgiUbWgPWG+b8MU5JlXQYdx8Jzvuotof7rCibxL6JpBK4CXlADk25ubvqsuHwQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TS6qD5nrfDxX23JYyCPhWAclKxBm5cSTudtUo89WA5WFqkG0sZuhVKB0VdJFOKCh/02tMfWxLEQX49BlHmg95pAq30R11hv6sWHiseBwKaYt+IYvrb/M5wwKSMo/muqg7uHPJiRPoyR/23jkMBznGsx8x0ejTITx8sMNgSEii8B5po7eNeTSOQEyrlbeRuMuOjB2cdusMeh9pGwi8804WcyilHLT5so6yZzRHQfRD5LO5iF8EaFZaYT8xWtdeRf4R25JHRRsRctbwu2xo9foJ0biYxdQ/VTd0nyBnnEFscy67hBomdxI4Jfx5EtBSM8E3re57uOVtoOdGSxEqptFcw==
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 27 Jul 2023 10:32:19 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHZuqGrLMJPz1ipBEu4urExXp8yi6/CgxUAgAWEoICAA8vwgIAA+VsAgAB5WACAAC7UgA==
  • Thread-topic: [PATCH v8 02/13] vpci: use per-domain PCI lock to protect vpci structure

Hi Jan

Jan Beulich <jbeulich@xxxxxxxx> writes:

> On 27.07.2023 02:56, Volodymyr Babchuk wrote:
>> Hi Roger,
>> 
>> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
>> 
>>> On Wed, Jul 26, 2023 at 01:17:58AM +0000, Volodymyr Babchuk wrote:
>>>>
>>>> Hi Roger,
>>>>
>>>> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
>>>>
>>>>> On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>> @@ -498,6 +537,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
>>>>>> unsigned int size,
>>>>>>          ASSERT(data_offset < size);
>>>>>>      }
>>>>>>      spin_unlock(&pdev->vpci->lock);
>>>>>> +    unlock_locks(d);
>>>>>
>>>>> There's one issue here, some handlers will cal pcidevs_lock(), which
>>>>> will result in a lock over inversion, as in the previous patch we
>>>>> agreed that the locking order was pcidevs_lock first, d->pci_lock
>>>>> after.
>>>>>
>>>>> For example the MSI control_write() handler will call
>>>>> vpci_msi_arch_enable() which takes the pcidevs lock.  I think I will
>>>>> have to look into using a dedicated lock for MSI related handling, as
>>>>> that's the only place where I think we have this pattern of taking the
>>>>> pcidevs_lock after the d->pci_lock.
>>>>
>>>> I'll mention this in the commit message. Is there something else that I
>>>> should do right now?
>>>
>>> Well, I don't think we want to commit this as-is with a known lock
>>> inversion.
>>>
>>> The functions that require the pcidevs lock are:
>>>
>>> pt_irq_{create,destroy}_bind()
>>> unmap_domain_pirq()
>>>
>>> AFAICT those functions require the lock in order to assert that the
>>> underlying device doesn't go away, as they do also use d->event_lock
>>> in order to get exclusive access to the data fields.  Please double
>>> check that I'm not mistaken.
>> 
>> You are right, all three function does not access any of PCI state
>> directly. However...
>> 
>>> If that's accurate you will have to check the call tree that spawns
>>> from those functions in order to modify the asserts to check for
>>> either the pcidevs or the per-domain pci_list lock being taken.
>> 
>> ... I checked calls for PT_IRQ_TYPE_MSI case, there is only call that
>> bothers me: hvm_pi_update_irte(), which calls IO-MMU code via
>> vmx_pi_update_irte():
>> 
>> amd_iommu_msi_msg_update_ire() or msi_msg_write_remap_rte().
>> 
>> Both functions read basic pdev fields like sbfd or type. I see no
>> problem there, as values of those fields are not supposed to be changed.
>
> But whether fields are basic or will never change doesn't matter when
> the pdev struct itself suddenly disappears.

This is not a problem, as it is expected that d->pci_lock is being held,
so pdev structure will not disappear. I am trying to answer another
question: is d->pci_lock enough or pcidevs_lock is also should required?

-- 
WBR, Volodymyr

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.