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 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;
}

Thanks, Roger.