On 10/15/24 02:58, Jan Beulich wrote:
> On 11.10.2024 17:27, Stewart Hildebrand wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1243,7 +1243,12 @@ int pci_reset_msix_state(struct pci_dev *pdev)
>> {
>> unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
>>
>> - ASSERT(pos);
>> + if ( !pos )
>> + {
>> + pdev->broken = true;
>> + return -EFAULT;
>> + }
>> +
>> /*
>> * Xen expects the device state to be the after reset one, and hence
>> * host_maskall = guest_maskall = false and all entries should have the
>> @@ -1271,7 +1276,12 @@ int pci_msi_conf_write_intercept(struct pci_dev
>> *pdev, unsigned int reg,
>> entry = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
>> pos = entry ? entry->msi_attrib.pos
>> : pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
>> - ASSERT(pos);
>> +
>> + if ( !pos )
>> + {
>> + pdev->broken = true;
>> + return -EFAULT;
>> + }
>>
>> if ( reg >= pos && reg < msix_pba_offset_reg(pos) + 4 )
>> {
>
> There are more instances of pci_find_cap_offset(..., PCI_CAP_ID_MSIX)
> which may want/need dealing with, even if there are no ASSERT()s there.
Yes, and some instances of pci_find_cap_offset(..., PCI_CAP_ID_MSI) too.
> Setting ->broken is of course a perhaps desirable (side) effect. Nevertheless
> I wonder whether latching the capability position once during device init
> wouldn't be an alternative (better?) approach.
I'll give this a try for the next rev.
> Finally I don't think -EFAULT is appropriate here. Imo it should be -ENODEV.
OK
>
> Jan
