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é <[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.
Aren't we planning to deviate rule 15.2? See [1] [1] https://lore.kernel.org/xen-devel/[email protected]/
