Re: [PATCH v5 1/3] x86/msi: harden stale pdev handling

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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