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.
Let me know how strong you feel about the usage of a label here vs one
at the tail of the function.
>
> > + invalid:
> > + xfree(msix);
> > + return -ENODEV;
> > +
>
> Extraneous newline.
Thanks, Roger.
