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

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

[Roger Pau Monne]

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@citrix.com>
Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
---
Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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
@@ -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)
-- 
2.51.0

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

[Stewart Hildebrand]

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@citrix.com>
> Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> ---
> Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> 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)

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

[Roger Pau Monné]

On Wed, Oct 08, 2025 at 09:17:30AM -0400, Stewart Hildebrand wrote:
> 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@citrix.com>
> > Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> > ---
> > Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
> > Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > 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>?

Can do, it's the same header that has the declarations for the printk
functions that we already use in msix.c, so if it wasn't indirectly
included the file won't build already.

Thanks, Roger.

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

[Stewart Hildebrand]

On 10/9/25 03:23, Roger Pau Monné wrote:
> On Wed, Oct 08, 2025 at 09:17:30AM -0400, Stewart Hildebrand wrote:
>> 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

s/severs/servers/

>>> 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@citrix.com>
>>> Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>>> ---
>>> Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>> 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>?
> 
> Can do,

With that adjusted:
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

> it's the same header that has the declarations for the printk
> functions that we already use in msix.c, so if it wasn't indirectly
> included the file won't build already.
> 
> Thanks, Roger.

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

[Roger Pau Monné]

On Wed, Oct 08, 2025 at 04:39:23PM +0200, 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@citrix.com>
> Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> ---
> Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> 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
> @@ -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. */

I had the following local change that I forgot to update the patch
with before sending:

/* Check that the referenced BAR(s) regions are valid. */

I think this is a better wording.

Thanks, Roger.

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

[Stewart Hildebrand]

On 10/8/25 10:59, Roger Pau Monné wrote:
> On Wed, Oct 08, 2025 at 04:39:23PM +0200, 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@citrix.com>
>> Released-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>> 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
>> @@ -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. */
> 
> I had the following local change that I forgot to update the patch
> with before sending:
> 
> /* Check that the referenced BAR(s) regions are valid. */
> 
> I think this is a better wording.

+1