Re: [PATCH v5 06/10] vpci: Hide extended capability when it fails to initialize

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

On 2025/6/5 22:47, Roger Pau Monné wrote:
> On Mon, May 26, 2025 at 05:45:55PM +0800, Jiqian Chen wrote:
>> When vpci fails to initialize a extended capability of device, it
>> just returns an error and vPCI gets disabled for the whole device.
>>
>> So, add 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>
>> cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> cc: Michal Orzel <michal.orzel@xxxxxxx>
>> cc: Jan Beulich <jbeulich@xxxxxxxx>
>> cc: Julien Grall <julien@xxxxxxx>
>> cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> ---
>> v4->v5 changes:
>> * Modify the hex digits of PCI_EXT_CAP_NEXT_MASK and PCI_EXT_CAP_NEXT to be 
>> low case.
>> * Rename vpci_ext_capability_mask to vpci_ext_capability_hide.
>>
>> v3->v4 changes:
>> * Change definition of PCI_EXT_CAP_NEXT to be "#define 
>> PCI_EXT_CAP_NEXT(header) (MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK) & 
>> 0xFFCU)" to avoid redundancy.
>> * Modify the commit message.
>> * Change vpci_ext_capability_mask() to return error instead of using ASSERT.
>> * Set the capability ID part to be zero when we need to hide the capability 
>> of position 0x100U.
>> * Add check "if ( !offset )" in vpci_ext_capability_mask().
>>
>> 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    | 100 +++++++++++++++++++++++++++++++++++--
>>  xen/include/xen/pci_regs.h |   5 +-
>>  2 files changed, 100 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 60e7654ec377..2d4794ff3dea 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -176,6 +176,98 @@ static int vpci_capability_hide(struct pci_dev *pdev, 
>> unsigned int cap)
>>      return 0;
>>  }
>>  
>> +static struct vpci_register *vpci_get_previous_ext_cap_register(
>> +    struct vpci *vpci, unsigned int offset)
>> +{
>> +    uint32_t header;
>> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
>> +    struct vpci_register *r;
>> +
>> +    if ( offset <= PCI_CFG_SPACE_SIZE )
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        return NULL;
>> +    }
>> +
>> +    r = vpci_get_register(vpci, pos, 4);
>> +    if ( !r )
>> +        return NULL;
>> +
>> +    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);
>> +        if ( !r )
>> +            return NULL;
>> +        header = (uint32_t)(uintptr_t)r->private;
>> +        pos = PCI_EXT_CAP_NEXT(header);
>> +    }
>> +
>> +    if ( pos <= PCI_CFG_SPACE_SIZE )
>> +        return NULL;
> 
> Same comment as in the previous patch, I think the proposed for loop
> there can also be used here to reduce a bit the code size (and unify
> the return paths).
> 
>> +
>> +    return r;
>> +}
>> +
>> +static int vpci_ext_capability_hide(struct pci_dev *pdev, unsigned int cap)
>> +{
>> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
>> +    struct vpci_register *rm, *prev_r;
> 
> s/rm/r/
> 
>> +    struct vpci *vpci = pdev->vpci;
>> +    uint32_t header, pre_header;
>> +
>> +    if ( !offset )
> 
> I think you want offset < PCI_CFG_SPACE_SIZE here?
> 
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        return 0;
>> +    }
>> +
>> +    spin_lock(&vpci->lock);
>> +    rm = vpci_get_register(vpci, offset, 4);
>> +    if ( !rm )
>> +    {
>> +        spin_unlock(&vpci->lock);
>> +        return -ENODEV;
>> +    }
>> +
>> +    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
>> +            /*
>> +             * If this case removes target capability of position 0x100U, 
>> then
>> +             * it needs to move 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 here 
>> chooses to
>> +             * set the capability ID part to be zero.
> 
> /*
>  * The first extended capability (0x100) cannot be removed from the linked
>  * list, so instead mask its capability ID to return 0 and force OSes
>  * to skip it.
>  */
> 
> Is simpler IMO and conveys the same message.
> 
>> +             */
>> +            rm->private = (void *)(uintptr_t)(header &
>> +                                              ~PCI_EXT_CAP_ID(header));
>> +
>> +        spin_unlock(&vpci->lock);
>> +        return 0;
>> +    }
>> +
>> +    prev_r = vpci_get_previous_ext_cap_register(vpci, offset);
>> +    if ( !prev_r )
>> +    {
>> +        spin_unlock(&vpci->lock);
>> +        return -ENODEV;
>> +    }
>> +
>> +    pre_header = (uint32_t)(uintptr_t)prev_r->private;
>> +    prev_r->private = (void *)(uintptr_t)((pre_header &
>> +                                           ~PCI_EXT_CAP_NEXT_MASK) |
>> +                                          (header & PCI_EXT_CAP_NEXT_MASK));
> 
> No strong opinion (and your code is correct), but it might be easier
> to read as:
> 
> pre_header &= ~PCI_EXT_CAP_NEXT_MASK;
> pre_header |= header & PCI_EXT_CAP_NEXT_MASK;
> prev_r->private = (void *)(uintptr_t)pre_header;
> 
> It's still tree lines of code at the end.  I would also add a newline
> to separate from the removal of rm.
> 
>> +    list_del(&rm->node);
>> +    spin_unlock(&vpci->lock);
>> +    xfree(rm);
> 
> Newline before the return preferably.
Will change my patch according to all your comments.

> 
>> +    return 0;
>> +}
>> +
>>  static int vpci_init_capabilities(struct pci_dev *pdev)
>>  {
>>      for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
>> @@ -209,11 +301,11 @@ static int vpci_init_capabilities(struct pci_dev *pdev)
>>                     pdev->domain, &pdev->sbdf,
>>                     is_ext ? "extended" : "legacy", cap);
>>              if ( !is_ext )
>> -            {
>>                  rc = vpci_capability_hide(pdev, cap);
>> -                if ( rc )
>> -                    return rc;
>> -            }
>> +            else
>> +                rc = vpci_ext_capability_hide(pdev, cap);
>> +            if ( rc )
>> +                return rc;
> 
> Could the code in the previous patch be:
> 
> if ( !is_ext )
>     rc = vpci_capability_hide(pdev, cap);
> 
> if ( rc )
>     return rc;
> 
> So that your introduction here is simpler?
OK, but the logic of the previous patch will become a little strange.
Anyway, the strange will disappear after applying this patch.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.