Re: [PATCH for-4.21] vpci/msix: improve handling of bogus MSI-X capabilities

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/7/25 10:56, Jan Beulich wrote:
> On 06.10.2025 10:20, Roger Pau Monné wrote:
>> On Fri, Oct 03, 2025 at 11:29:40PM -0400, Stewart Hildebrand wrote:
>>> On 9/29/25 04:41, Roger Pau Monne wrote:
>>>> I've had the luck to come across a PCI card that exposes a MSI-X capability
>>>> where the BIR of the vector and PBA tables points at a BAR that has 0 size.
>>>>
>>>> This doesn't play nice with the code in vpci_make_msix_hole(), as it would
>>>> still use the address of such empty BAR (0) and attempt to crave a hole in
>>>
>>> s/crave/carve/
>>>
>>>> the p2m.  This leads to errors like the one below being reported by Xen:
>>>>
>>>> d0v0 0000:22:00.0: existing mapping (mfn: 181c4300 type: 0) at 0 clobbers 
>>>> MSIX MMIO area
>>>>
>>>> And the device left unable to enable memory decoding due to the failure
>>>> reported by vpci_make_msix_hole().
>>>>
>>>> Introduce checking in init_msix() to ensure the BARs containing the MSI-X
>>>> tables are usable.  This requires checking that the BIR points to a
>>>> non-empty BAR, and the offset and size of the MSI-X tables can fit in the
>>>> target BAR.
>>>>
>>>> This fixes booting PVH dom0 on Supermicro AS -2126HS-TN severs with AMD
>>>> EPYC 9965 processors.  The broken device is:
>>>>
>>>> 22:00.0 SATA controller: Advanced Micro Devices, Inc. [AMD] FCH SATA 
>>>> Controller [AHCI mode] (rev 93)
>>>>
>>>> There are multiple of those integrated controllers in the system, all
>>>> broken in the same way.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> ---
>>>> Cc: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Cc: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>>
>>>> While not strictly a bugfix, I consider this a worthy improvement so that
>>>> PVH dom0 has a chance to boot on hardware that exposes such broken MSI-X
>>>> capabilities.  Hence I think this change should be considered for inclusion
>>>> into 4.21.  There a risk of regressing on hardware that was already working
>>>> with PVH, but given enough testing that should be minimal.
>>>> ---
>>>>  xen/drivers/vpci/msix.c | 50 ++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 45 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>>>> index 54a5070733aa..8458955d5bbb 100644
>>>> --- a/xen/drivers/vpci/msix.c
>>>> +++ b/xen/drivers/vpci/msix.c
>>>> @@ -675,6 +675,51 @@ static int cf_check init_msix(struct pci_dev *pdev)
>>>>      if ( !msix )
>>>>          return -ENOMEM;
>>>>  
>>>> +    msix->tables[VPCI_MSIX_TABLE] =
>>>> +        pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
>>>> +    msix->tables[VPCI_MSIX_PBA] =
>>>> +        pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
>>>> +
>>>> +    /* Check that the provided BAR is valid. */
>>>> +    for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
>>>> +    {
>>>> +        const char *name = (i == VPCI_MSIX_TABLE) ? "vector" : "PBA";
>>>> +        const struct vpci_bar *bars = pdev->vpci->header.bars;
>>>> +        unsigned int bir = msix->tables[i] & PCI_MSIX_BIRMASK;
>>>> +        unsigned int type;
>>>> +        unsigned int offset = msix->tables[i] & ~PCI_MSIX_BIRMASK;
>>>> +        unsigned int size =
>>>> +            (i == VPCI_MSIX_TABLE) ? max_entries * PCI_MSIX_ENTRY_SIZE
>>>> +                                   : ROUNDUP(DIV_ROUND_UP(max_entries, 
>>>> 8), 8);
>>>> +
>>>> +        if ( bir >= ARRAY_SIZE(pdev->vpci->header.bars) )
>>>
>>> This assumes a type 0 header. For type 1 headers, bir values 2 and up are
>>> also reserved.
>>
>> Right, but those BARs will be set as VPCI_BAR_EMPTY for type 1 headers.
>> The check here is to avoid doing an out of bounds array access, the
>> check for validity of the pointed BAR is done below.
>>
>>>
>>>> +        {
>>>> +            printk(XENLOG_ERR "%pp: MSI-X %s table with out of range BIR 
>>>> %u\n",
>>>> +                   &pdev->sbdf, name, bir);
>>>
>>> Nit: placing the cleanup label at the end of the function and using 'rc' 
>>> would
>>> make it more amenable to future uses.
>>
>> The issue with that is that we then end up with a structure like:
>>
>>     return vpci_make_msix_hole();
>>
>>  error:
>>     xfree();
>>     return rc;
>>
>> Which I don't like much because of the double usage of return (it's a
>> taste issue TBH).
>>
>> My motivation for using a goto is that they are conceptually the same
>> error path, but we provide different log messages to aid in debugging
>> the issue.  Otherwise all checks will be done in a single condition.
> 
> I agree here, yet I'd like to point out that (iirc) Misra wants us to use
> only forward goto-s (which imo is a mistake, but I don't expect they're
> going to change their minds). So flipping where the label and goto are
> may be desirable.

Aren't we planning to deviate rule 15.2? See [1]

[1] 
https://lore.kernel.org/xen-devel/7e2993c68fda95d1eda6fd136750fcba@xxxxxxxxxxx/