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é <[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) ) >> >> 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. Jan
