On Tue Oct 7, 2025 at 9:16 AM CEST, Roger Pau Monné wrote:
> On Mon, Oct 06, 2025 at 03:29:11PM +0100, Andrew Cooper wrote:
>> On 06/10/2025 2:55 pm, Alejandro Vallejo wrote:
>> > On Tue Sep 30, 2025 at 2:57 PM CEST, Roger Pau Monné wrote:
>> >> On Tue, Sep 30, 2025 at 11:15:01AM +0200, Alejandro Vallejo wrote:
>> >>> On Mon Sep 29, 2025 at 10:41 AM CEST, 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
>> >>>> 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) )
>> >>>> + {
>> >>>> + printk(XENLOG_ERR "%pp: MSI-X %s table with out of range
>> >>>> BIR %u\n",
>> >>>> + &pdev->sbdf, name, bir);
>> >>> Would it be worth adding something here such that a device vendor
>> >>> testing their
>> >>> hardware under Xen can trivially grep for device bugs?
>> >>>
>> >>> Something akin to "[Firmware bug]" on Linux, like "[Device bug]" or some
>> >>> such.
>> >>>
>> >>> It would also let anyone not very knowledgeable about PCI know that a
>> >>> device
>> >>> they own is being unreasonable. Same below in the other XENLOG_ERR
>> >>> messages.
>> >> We could add indeed. I don't think we haven't done so in the past.
>> >> If we go that route I would suggest that I add a:
>> >>
>> >> #define DEVICE_BUG_PREFIX "[Device bug] "
>> >>
>> >> in lib.h or similar, to make sure we use the same prefix uniformly.
>> >> TBH
>>
>> We have several FIRMWARE BUG's in Xen already, and several more that
>> ought to move to this pattern.
>>
>> Given that Linux has definitely been booted on this hardware, we should
>> match whichever prefix they use for messages about this.
>
> I don't think Linux prints any message about this, it simply ignores
> the capability.
>
> We have another instance of having to support buggy devices in vPCI:
> when a device places registers in the same 4K page as the MSI-X vector
> or PBA tables. In that case the offending device was an Intel
> Wireless card.
>
> I'm happy to use "[Device Bug]", will adjust the patch this afternoon.
>
In Linux's printk.h
/*
* FW_BUG
* Add this to a message where you are sure the firmware is buggy or
behaves
* really stupid or out of spec. Be aware that the responsible BIOS
developer
* should be able to fix this issue or at least get a concrete idea of
the
* problem by reading your message without the need of looking at the
kernel
* code.
*
* Use it for definite and high priority BIOS bugs.
*
* FW_WARN
* Use it for not that clear (e.g. could the kernel messed up things
already?)
* and medium priority BIOS bugs.
*
* FW_INFO
* Use this one if you want to tell the user or vendor about something
* suspicious, but generally harmless related to the firmware.
*
* Use it for information or very low priority BIOS bugs.
*/
#define FW_BUG "[Firmware Bug]: "
#define FW_WARN "[Firmware Warn]: "
#define FW_INFO "[Firmware Info]: "
/*
* HW_ERR
* Add this to a message for hardware errors, so that user can report
* it to hardware vendor instead of LKML or software vendor.
*/
#define HW_ERR "[Hardware Error]: "
HW_ERR seems wired to MCEs and the like rather than everything covered by
quirks.c in Linux, but using [Hardware Error] might help grep scripts by
matching existing messages in Linux's dmesg.
Otherwise, "[Devive Bug]" works just as well.
Cheers,
Alejandro
