On 2025/5/7 00:21, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
>> When vpci fails to initialize a extended capability of device for dom0,
>> it just return error instead of catching and processing exception. That
>> makes the entire device unusable.
>>
>> So, add new a function to hide extended capability when initialization
>> fails. And remove the failed extended capability handler from vpci
>> extended capability list.
>>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger....@citrix.com>
>> ---
>> v2->v3 changes:
>> * Separated from the last version patch "vpci: Hide capability when it fails
>> to initialize".
>> * Whole implementation changed because last version is wrong.
>> This version gets target handler and previous handler from vpci->handlers,
>> then remove the target.
>> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
>> because it may change the offset of next capability when the offset of
>> target
>> capability is 0x100U(the first extended capability), my implementation is
>> just to
>> ignore and let hardware to handle the target capability.
>>
>> v1->v2 changes:
>> * Removed the "priorities" of initializing capabilities since it isn't used
>> anymore.
>> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
>> remove failed capability from list.
>> * Called vpci_make_msix_hole() in the end of init_msix().
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>> xen/drivers/vpci/vpci.c | 79 ++++++++++++++++++++++++++++++++++++++
>> xen/include/xen/pci_regs.h | 1 +
>> 2 files changed, 80 insertions(+)
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index f97c7cc460a0..8ff5169bdd18 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
>> xfree(next_r);
>> }
>>
>> +static struct vpci_register *vpci_get_previous_ext_cap_register
>> + (struct vpci *vpci, const unsigned int offset)
>> +{
>> + uint32_t header;
>> + unsigned int pos = PCI_CFG_SPACE_SIZE;
>> + struct vpci_register *r;
>> +
>> + if ( offset <= PCI_CFG_SPACE_SIZE )
>> + return NULL;
>> +
>> + r = vpci_get_register(vpci, pos, 4);
>> + ASSERT(r);
>> +
>> + header = (uint32_t)(uintptr_t)r->private;
>> + pos = PCI_EXT_CAP_NEXT(header);
>> + while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
>> + {
>> + r = vpci_get_register(vpci, pos, 4);
>> + ASSERT(r);
>> + header = (uint32_t)(uintptr_t)r->private;
>> + pos = PCI_EXT_CAP_NEXT(header);
>> + }
>> +
>> + if ( pos <= PCI_CFG_SPACE_SIZE )
>> + return NULL;
>> +
>> + return r;
>> +}
>> +
>> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
>> + const unsigned int cap)
>> +{
>> + const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
>> + struct vpci_register *rm, *prev_r;
>> + struct vpci *vpci = pdev->vpci;
>> + uint32_t header, pre_header;
>
> Maybe sanity check that offset is correct?
What do you mean sanity check?
Do I need to add something?
>
>> + spin_lock(&vpci->lock);
>> + rm = vpci_get_register(vpci, offset, 4);
>> + if ( !rm )
>> + {
>> + spin_unlock(&vpci->lock);
>> + return;
>> + }
>> +
>> + header = (uint32_t)(uintptr_t)rm->private;
>> + if ( offset == PCI_CFG_SPACE_SIZE )
>> + {
>> + if ( PCI_EXT_CAP_NEXT(header) <= PCI_CFG_SPACE_SIZE )
>> + rm->private = (void *)(uintptr_t)0;
>> + else
>> + /*
>> + * Else case needs to remove the capability in position 0x100U
>> and
>> + * moves the next capability to be in position 0x100U, that
>> would
>> + * cause the offset of next capability in vpci different from
>> the
>> + * hardware, then cause error accesses, so just ignore it here
>> and
>> + * hope hardware would handle the capability well.
>> + */
>> + printk(XENLOG_ERR "%pd %pp: ext cap %u is first cap, can't mask
>> it\n",
>> + pdev->domain, &pdev->sbdf, cap);
>
> In this case, could you maybe replace just the capability ID part of
> the header to return 0? That will likely cause the OS to continue
> scanning the list, while ID 0 won't match which any known
> capability.
OK, will do in next version.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
