On 04.02.2025 12:46, Roger Pau Monné wrote:
> On Wed, Jan 29, 2025 at 11:13:09AM +0100, Jan Beulich wrote:
>> On 28.01.2025 17:27, Roger Pau Monne wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1803,6 +1803,38 @@ int iommu_do_pci_domctl(
>>> return ret;
>>> }
>>>
>>> +struct segment_iter {
>>> + int (*handler)(struct pci_dev *pdev, void *arg);
>>> + void *arg;
>>> +};
>>> +
>>> +static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
>>> +{
>>> + const struct segment_iter *iter = arg;
>>> + struct pci_dev *pdev;
>>> +
>>> + list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> + {
>>> + int rc = iter->handler(pdev, iter->arg);
>>> +
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
>>> + void *arg)
>>> +{
>>> + struct segment_iter iter = {
>>> + .handler = handler,
>>> + .arg = arg,
>>> + };
>>> +
>>> + return pci_segments_iterate(iterate_all, &iter);
>>> +}
>>
>> For the specific purpose during shutdown it may be okay to do all of this
>> without locking (but see below) and without preemption checks. Yet then a
>> warning will want putting here to indicate that from other environments
>> this isn't okay to use as-is.
>>
>> This use then also requires that msi{,x}_set_enable() paths never gain
>> lock-related assertions.
>
> Good point. It might be better to just wrap the code in
> pci_iterate_devices() with pcidevs_{,un}lock().
I'd recommend against doing so. If this was just for ordinary reboot or
shutdown, then yes. But we can crash with this (or any other) lock held.
And this particular lock we hold for sometimes pretty long sequences of
code.
There's anyway the wider issue of how much code we want to involve in
rebooting (or kexec-ing) after a crash: The more we do, the more likely
that we run into a knock-on issue from the earlier crash.
Jan
