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

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Tue Oct 7, 2025 at 9:16 AM CEST, Roger Pau Monné wrote:
> On Mon, Oct 06, 2025 at 03:29:11PM +0100, Andrew Cooper wrote:
>> On 06/10/2025 2:55 pm, Alejandro Vallejo wrote:
>> > On Tue Sep 30, 2025 at 2:57 PM CEST, Roger Pau Monné wrote:
>> >> On Tue, Sep 30, 2025 at 11:15:01AM +0200, Alejandro Vallejo wrote:
>> >>> On Mon Sep 29, 2025 at 10:41 AM CEST, 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
>> >>>> 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) )
>> >>>> +        {
>> >>>> +            printk(XENLOG_ERR "%pp: MSI-X %s table with out of range 
>> >>>> BIR %u\n",
>> >>>> +                   &pdev->sbdf, name, bir);
>> >>> Would it be worth adding something here such that a device vendor 
>> >>> testing their
>> >>> hardware under Xen can trivially grep for device bugs?
>> >>>
>> >>> Something akin to "[Firmware bug]" on Linux, like "[Device bug]" or some 
>> >>> such.
>> >>>
>> >>> It would also let anyone not very knowledgeable about PCI know that a 
>> >>> device
>> >>> they own is being unreasonable. Same below in the other XENLOG_ERR 
>> >>> messages.
>> >> We could add indeed.  I don't think we haven't done so in the past.
>> >> If we go that route I would suggest that I add a:
>> >>
>> >> #define DEVICE_BUG_PREFIX "[Device bug] "
>> >>
>> >> in lib.h or similar, to make sure we use the same prefix uniformly.
>> >> TBH
>> 
>> We have several FIRMWARE BUG's in Xen already, and several more that
>> ought to move to this pattern.
>> 
>> Given that Linux has definitely been booted on this hardware, we should
>> match whichever prefix they use for messages about this.
>
> I don't think Linux prints any message about this, it simply ignores
> the capability.
>
> We have another instance of having to support buggy devices in vPCI:
> when a device places registers in the same 4K page as the MSI-X vector
> or PBA tables.  In that case the offending device was an Intel
> Wireless card.
>
> I'm happy to use "[Device Bug]", will adjust the patch this afternoon.
>

In Linux's printk.h

        /*
         * FW_BUG
         * Add this to a message where you are sure the firmware is buggy or 
behaves
         * really stupid or out of spec. Be aware that the responsible BIOS 
developer
         * should be able to fix this issue or at least get a concrete idea of 
the
         * problem by reading your message without the need of looking at the 
kernel
         * code.
         *
         * Use it for definite and high priority BIOS bugs.
         *
         * FW_WARN
         * Use it for not that clear (e.g. could the kernel messed up things 
already?)
         * and medium priority BIOS bugs.
         *
         * FW_INFO
         * Use this one if you want to tell the user or vendor about something
         * suspicious, but generally harmless related to the firmware.
         *
         * Use it for information or very low priority BIOS bugs.
         */
        #define FW_BUG          "[Firmware Bug]: "
        #define FW_WARN         "[Firmware Warn]: "
        #define FW_INFO         "[Firmware Info]: "

        /*
         * HW_ERR
         * Add this to a message for hardware errors, so that user can report
         * it to hardware vendor instead of LKML or software vendor.
         */
        #define HW_ERR          "[Hardware Error]: "

HW_ERR seems wired to MCEs and the like rather than everything covered by
quirks.c in Linux, but using [Hardware Error] might help grep scripts by
matching existing messages in Linux's dmesg.

Otherwise, "[Devive Bug]" works just as well.

Cheers,
Alejandro