Re: [PATCH v3 07/11] vpci: Hide extended capability when it fails to initialize

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/5/7 16:09, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 07:26:21AM +0000, Chen, Jiqian wrote:
>> 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.Chen@xxxxxxx>
>>>> ---
>>>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>>>> ---
>>>> 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?
> 
> I would probably do something like:
> 
> if ( !offset )
> {
>     ASSERT_UNREACHABLE();
>     return;
> }
How about adding check?

    if ( offset < PCI_CFG_SPACE_SIZE )
    {
        ASSERT_UNREACHABLE();
        return -EINVAL;
    }

Do I need to add similar check in vpci_capability_mask()?

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.