On 14.02.22 14:57, Jan Beulich wrote:
> On 14.02.2022 12:37, Oleksandr Andrushchenko wrote:
>>
>> On 14.02.22 13:25, Roger Pau Monné wrote:
>>> On Mon, Feb 14, 2022 at 11:15:27AM +0000, Oleksandr Andrushchenko wrote:
>>>> On 14.02.22 13:11, Roger Pau Monné wrote:
>>>>> On Mon, Feb 14, 2022 at 10:53:43AM +0000, Oleksandr Andrushchenko wrote:
>>>>>> On 14.02.22 12:34, Roger Pau Monné wrote:
>>>>>>> On Mon, Feb 14, 2022 at 09:36:39AM +0000, Oleksandr Andrushchenko wrote:
>>>>>>>> On 11.02.22 13:40, Roger Pau Monné wrote:
>>>>>>>>> +
>>>>>>>>>>>> for ( i = 0; i < msix->max_entries; i++ )
>>>>>>>>>>>> {
>>>>>>>>>>>> const struct vpci_msix_entry *entry =
>>>>>>>>>>>> &msix->entries[i];
>>>>>>>>>>> Since this function is now called with the per-domain rwlock read
>>>>>>>>>>> locked it's likely not appropriate to call process_pending_softirqs
>>>>>>>>>>> while holding such lock (check below).
>>>>>>>>>> You are right, as it is possible that:
>>>>>>>>>>
>>>>>>>>>> process_pending_softirqs -> vpci_process_pending -> read_lock
>>>>>>>>>>
>>>>>>>>>> Even more, vpci_process_pending may also
>>>>>>>>>>
>>>>>>>>>> read_unlock -> vpci_remove_device -> write_lock
>>>>>>>>>>
>>>>>>>>>> in its error path. So, any invocation of process_pending_softirqs
>>>>>>>>>> must not hold d->vpci_rwlock at least.
>>>>>>>>>>
>>>>>>>>>> And also we need to check that pdev->vpci was not removed
>>>>>>>>>> in between or *re-created*
>>>>>>>>>>> We will likely need to re-iterate over the list of pdevs assigned to
>>>>>>>>>>> the domain and assert that the pdev is still assigned to the same
>>>>>>>>>>> domain.
>>>>>>>>>> So, do you mean a pattern like the below should be used at all
>>>>>>>>>> places where we need to call process_pending_softirqs?
>>>>>>>>>>
>>>>>>>>>> read_unlock
>>>>>>>>>> process_pending_softirqs
>>>>>>>>>> read_lock
>>>>>>>>>> pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>>>>>>>>>> if ( pdev && pdev->vpci && is_the_same_vpci(pdev->vpci) )
>>>>>>>>>> <continue processing>
>>>>>>>>> Something along those lines. You likely need to continue iterate using
>>>>>>>>> for_each_pdev.
>>>>>>>> How do we tell if pdev->vpci is the same? Jan has already brought
>>>>>>>> this question before [1] and I was about to use some ID for that
>>>>>>>> purpose:
>>>>>>>> pdev->vpci->id = d->vpci_id++ and then we use pdev->vpci->id for
>>>>>>>> checks
>>>>>>> Given this is a debug message I would be OK with just doing the
>>>>>>> minimal checks to prevent Xen from crashing (ie: pdev->vpci exists)
>>>>>>> and that the resume MSI entry is not past the current limit. Otherwise
>>>>>>> just print a message and move on to the next device.
>>>>>> Agree, I see no big issue (probably) if we are not able to print
>>>>>>
>>>>>> How about this one:
>>>>>>
>>>>>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>>>>>> index 809a6b4773e1..50373f04da82 100644
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -171,10 +171,31 @@ static int __init apply_map(struct domain *d,
>>>>>> const struct pci_dev *pdev,
>>>>>> struct rangeset *mem, uint16_t cmd)
>>>>>> {
>>>>>> struct map_data data = { .d = d, .map = true };
>>>>>> + pci_sbdf_t sbdf = pdev->sbdf;
>>>>>> int rc;
>>>>>>
>>>>>> + ASSERT(rw_is_write_locked(&pdev->domain->vpci_rwlock));
>>>>>> +
>>>>>> while ( (rc = rangeset_consume_ranges(mem, map_range, &data))
>>>>>> == -ERESTART )
>>>>>> + {
>>>>>> +
>>>>>> + /*
>>>>>> + * process_pending_softirqs may trigger vpci_process_pending
>>>>>> which
>>>>>> + * may need to acquire pdev->domain->vpci_rwlock in read mode.
>>>>>> + */
>>>>>> + write_unlock(&pdev->domain->vpci_rwlock);
>>>>>> process_pending_softirqs();
>>>>>> + write_lock(&pdev->domain->vpci_rwlock);
>>>>>> +
>>>>>> + /* Check if pdev still exists and vPCI was not removed or
>>>>>> re-created. */
>>>>>> + if (pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn)
>>>>>> != pdev)
>>>>>> + if ( vpci is NOT the same )
>>>>>> + {
>>>>>> + rc = 0;
>>>>>> + break;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> rangeset_destroy(mem);
>>>>>> if ( !rc )
>>>>>> modify_decoding(pdev, cmd, false);
>>>>>>
>>>>>> This one also wants process_pending_softirqs to run so it *might*
>>>>>> want pdev and vpci checks. But at the same time apply_map runs
>>>>>> at ( system_state < SYS_STATE_active ), so defer_map won't be
>>>>>> running yet, thus no vpci_process_pending is possible yet (in terms
>>>>>> it has something to do yet). So, I think we just need:
>>>>>>
>>>>>> write_unlock(&pdev->domain->vpci_rwlock);
>>>>>> process_pending_softirqs();
>>>>>> write_lock(&pdev->domain->vpci_rwlock);
>>>>>>
>>>>>> and this should be enough
>>>>> Given the context apply_map is called from (dom0 specific init code),
>>>>> there's no need to check for the pdev to still exits, or whether vpci
>>>>> has been recreated, as it's not possible. Just add a comment to
>>>>> explicitly note that the context of the function is special, and thus
>>>>> there's no possibility of either the device or vpci going away.
>>>> Does it really need write_unlock/write_lock given the context?...
>>> I think it's bad practice to call process_pending_softirqs while
>>> holding any locks. This is a very specific context so it's likely fine
>>> to not drop the lock, but would still seem incorrect to me.
>> Ok
>>>> I think it doesn't as there is no chance defer_map is called, thus
>>>> process_pending_softirqs -> vpci_process_pending -> read_lock
>>> Indeed, there's no chance of that because process_pending_softirqs
>>> will never try to do a scheduling operation that would result in our
>>> context being scheduled out.
>> while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) ==
>> -ERESTART )
>> {
>> /*
>> * FIXME: Given the context apply_map is called from (dom0 specific
>> * init code at system_state < SYS_STATE_active) it is not strictly
>> * required that pdev->domain->vpci_rwlock is unlocked before
>> calling
>> * process_pending_softirqs as there is no contention possible
>> between
>> * this code and vpci_process_pending trying to acquire the lock in
>> * read mode. But running process_pending_softirqs with any lock
>> held
>> * doesn't seem to be a good practice, so drop the lock and
>> re-acquire
>> * it right again.
>> */
>> write_unlock(&pdev->domain->vpci_rwlock);
>> process_pending_softirqs();
>> write_lock(&pdev->domain->vpci_rwlock);
>> }
> I'm afraid that's misleading at best. apply_map() is merely a specific
> example where you know the lock is going to be taken. But really any
> softirq handler could be acquiring any lock, so requesting to process
> softirqs cannot ever be done with any lock held.
>
> What you instead want to explain is why, after re-acquiring the lock,
> no further checking is needed for potentially changed state.
How about:
/*
* FIXME: Given the context apply_map is called from (dom0 specific
* init code at system_state < SYS_STATE_active) there is no contention
* possible between this code and vpci_process_pending trying to acquire
* the lock in read mode and destroy pdev->vpci in its error path.
* Neither pdev may be disposed yet, so it is not required to check if the
* relevant pdev still exists after re-acquiring the lock.
*/
>
> Jan
>
Thank you,
Oleksandr
