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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> cc: Andrew Cooper <andrew.cooper3@citrix.com>
> cc: Anthony PERARD <anthony.perard@vates.tech>
> cc: Michal Orzel <michal.orzel@amd.com>
> cc: Jan Beulich <jbeulich@suse.com>
> cc: Julien Grall <julien@xen.org>
> cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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.

> +    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?

Thanks, Roger.