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.
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.
Finally I don't think -EFAULT is appropriate here. Imo it should be -ENODEV.
Jan
