[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 06/10] vpci: Hide extended capability when it fails to initialize
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. > + 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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |