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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/8/25 10:39, 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 carve 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>
> Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@xxxxxxxxx>
> ---
> Cc: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Cc: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes since v1:
>  - Introduce a DEVICE BUG prefix.
>  - Remove extra newline.
>  - Fix typo in commit message.
> ---
>  xen/drivers/vpci/msix.c | 50 ++++++++++++++++++++++++++++++++++++-----
>  xen/include/xen/lib.h   |  3 +++
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 54a5070733aa..4ddcefbcb274 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c

Do we need to #include <xen/lib.h>?

> @@ -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 DEV_BUG_PREFIX
> +                   "%pp: MSI-X %s table with out of range BIR %u\n",
> +                   &pdev->sbdf, name, bir);
> + invalid:
> +            xfree(msix);
> +            return -ENODEV;
> +        }
> +
> +        type = bars[bir].type;
> +        if ( type != VPCI_BAR_MEM32 && type != VPCI_BAR_MEM64_LO )
> +        {
> +            printk(XENLOG_ERR DEV_BUG_PREFIX
> +                   "%pp: MSI-X %s table at invalid BAR%u with type %u\n",
> +                   &pdev->sbdf, name, bir, type);
> +            goto invalid;
> +        }
> +
> +        if ( (uint64_t)offset + size > bars[bir].size )
> +        {
> +            printk(XENLOG_ERR DEV_BUG_PREFIX
> +                   "%pp: MSI-X %s table offset %#x size %#x outside of BAR%u 
> size %#lx\n",
> +                   &pdev->sbdf, name, offset, size, bir, bars[bir].size);
> +            goto invalid;
> +        }
> +    }
> +
>      rc = vpci_add_register(pdev->vpci, control_read, control_write,
>                             msix_control_reg(msix_offset), 2, msix);
>      if ( rc )
> @@ -686,11 +731,6 @@ static int cf_check init_msix(struct pci_dev *pdev)
>      msix->max_entries = max_entries;
>      msix->pdev = pdev;
>  
> -    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));
> -
>      for ( i = 0; i < max_entries; i++)
>      {
>          msix->entries[i].masked = true;
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index c434dd5f16e4..c4ac4823920f 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -60,6 +60,9 @@ static inline void
>  debugtrace_printk(const char *fmt, ...) {}
>  #endif
>  
> +/* Common log prefixes for platform related issues. */
> +#define DEV_BUG_PREFIX "DEVICE BUG: "
> +
>  extern void printk(const char *fmt, ...)
>      __attribute__ ((format (printf, 1, 2), cold));
>  void vprintk(const char *fmt, va_list args)