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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jun 06, 2025 at 07:03:02AM +0000, Chen, Jiqian wrote:
> On 2025/6/5 21:35, Roger Pau Monné wrote:
> > On Mon, May 26, 2025 at 05:45:54PM +0800, Jiqian Chen wrote:
> >> When vpci fails to initialize a legacy capability of device, it just
> >> returns an error and vPCI gets disabled for the whole device.  That
> >> most likely renders the device unusable, plus possibly causing issues
> >> to Xen itself if guest attempts to program the native MSI or MSI-X
> >> capabilities if present.
> >>
> >> So, add new function to hide legacy capability when initialization
> >> fails. And remove the failed legacy capability from the vpci emulated
> >> legacy capability list.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> v4->v5 changes:
> >> * Modify vpci_get_register() to delete some unnecessary check, so that I 
> >> don't need to move function vpci_register_cmp().
> >> * Rename vpci_capability_mask() to vpci_capability_hide().
> >>
> >> v3->v4 changes:
> >> * Modify the commit message.
> >> * In function vpci_get_previous_cap_register(), add an 
> >> ASSERT_UNREACHABLE() if offset below 0x40.
> >> * Modify vpci_capability_mask() to return error instead of using ASSERT.
> >> * Use vpci_remove_register to remove PCI_CAP_LIST_ID register instead of 
> >> open code.
> >> * Add check "if ( !offset )" in vpci_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 adds a new helper function vpci_get_register() and uses it 
> >> to get
> >>   target handler and previous handler from vpci->handlers, then remove the 
> >> target.
> >>
> >> 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 | 117 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 113 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >> index 2861557e06d2..60e7654ec377 100644
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -83,6 +83,99 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
> >>  
> >>  #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> >>  
> >> +static struct vpci_register *vpci_get_register(struct vpci *vpci,
> >> +                                               unsigned int offset,
> >> +                                               unsigned int size)
> >> +{
> >> +    struct vpci_register *rm;
> > 
> > Nit: I think you re-used part of the code from vpci_remove_register()
> > that names the local variable rm (because it's for removal).  Here it
> > would better to just name it 'r' (for register).
> Will change.
> > 
> >> +
> >> +    ASSERT(spin_is_locked(&vpci->lock));
> >> +
> >> +    list_for_each_entry ( rm, &vpci->handlers, node )
> >> +    {
> >> +        if ( rm->offset == offset && rm->size == size )
> >> +            return rm;
> >> +
> >> +        if ( offset <= rm->offset )
> >> +            break;
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >> +static struct vpci_register *vpci_get_previous_cap_register(
> >> +    struct vpci *vpci, unsigned int offset)
> >> +{
> >> +    uint32_t next;
> >> +    struct vpci_register *r;
> >> +
> >> +    if ( offset < 0x40 )
> >> +    {
> >> +        ASSERT_UNREACHABLE();
> >> +        return NULL;
> >> +    }
> >> +
> >> +    r = vpci_get_register(vpci, PCI_CAPABILITY_LIST, 1);
> >> +    if ( !r )
> >> +        return NULL;
> >> +
> >> +    next = (uint32_t)(uintptr_t)r->private;
> >> +    while ( next >= 0x40 && next != offset )
> >> +    {
> >> +        r = vpci_get_register(vpci, next + PCI_CAP_LIST_NEXT, 1);
> >> +        if ( !r )
> >> +            return NULL;
> >> +        next = (uint32_t)(uintptr_t)r->private;
> >> +    }
> >> +
> >> +    if ( next < 0x40 )
> >> +        return NULL;
> > 
> > The code below I think it's a bit simpler (and compact) by having a
> > single return instead of multiple ones:
> > 
> > for ( r = vpci_get_register(vpci, PCI_CAPABILITY_LIST, 1); r;
> >       r = next >= 0x40 ? vpci_get_register(vpci,
> >                                            next + PCI_CAP_LIST_NEXT, 1)
> >                        : NULL )
> > {
> >     next = (uint32_t)(uintptr_t)r->private;
> >     ASSERT(next == (uintptr_t)r->private);
> Why need this ASSERT here?

Extra safety to ensure the value is not truncated (which will indicate
something is off).  I would not argue strongly for it to be added if
you don't think it's needed.

Thanks, Roger.