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

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

On Wed, May 07, 2025 at 08:49:46AM +0000, Chen, Jiqian wrote:
> 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;
>     }

That would work also, however note that pci_find_ext_capability()
should only return 0 if the capability is not found, and other callers
already assume that != 0 implies a valid position.  I will simply
check !offset as that's inline with all the other checks Xen does for
return values of pci_find_ext_capability().

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

Possibly - seems like I didn't comment on that one, sorry.

Regards, Roger.