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é <[email protected]> > >>>> --- > >>>> Cc: Stewart Hildebrand <[email protected]> > >>>> Cc: Jan Beulich <[email protected]> > >>>> Cc: Oleksii Kurochko <[email protected]> > >>>> > >>>> 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. > What's unclear is whether AMD can even fix this with a firmware update. > I would have expected that the PCIe hardblock would have prevented > making this mistake, but clearly not... I didn't want to point fingers :), I have no idea if it can be fixed in firmware. Thanks, Roger.
