On Wed, May 07, 2025 at 05:59:52AM +0000, Chen, Jiqian wrote:
> On 2025/5/6 22:37, Roger Pau Monné wrote:
> > On Mon, Apr 21, 2025 at 02:18:57PM +0800, Jiqian Chen wrote:
> >> Refactor REGISTER_VPCI_INIT to contain more capability specific
> >> information, this is benifit for follow-on changes to hide capability
> >> which initialization fails.
> >>
> >> What's more, change the definition of init_header() since it is
> >> not a capability and it is needed for all devices' PCI config space.
> >>
> >> Note:
> >> Call vpci_make_msix_hole() in the end of init_msix() since the
> >> change of sequence of init_header() and init_msix().
> >> The fini hook will be implemented in follow-on changes.
> >
> > I would maybe add that the cleanup hook is also added in this change,
> > even if it's still unused. Further changes will make use of it.
> Do you mean I need to add empty fini_x() function for MSI, MSIx, Rebar in
> this patch?
> Or just need to add this sentence to the commit message?
Oh, no, sorry if it wasn't clear, I meant just adding the sentence to
the commit message.
> >
> >>
> >> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
> >> ---
> >> cc: "Roger Pau Monné" <roger....@citrix.com>
> >> cc: Andrew Cooper <andrew.coop...@citrix.com>
> >> cc: Anthony PERARD <anthony.per...@vates.tech>
> >> cc: Michal Orzel <michal.or...@amd.com>
> >> cc: Jan Beulich <jbeul...@suse.com>
> >> cc: Julien Grall <jul...@xen.org>
> >> cc: Stefano Stabellini <sstabell...@kernel.org>
> >> ---
> >> v2->v3 changes:
> >> * This is separated from patch "vpci: Hide capability when it fails to
> >> initialize" of v2.
> >> * Delete __maybe_unused attribute of "out" in function vpci_assign_devic().
> >> * Rename REGISTER_VPCI_EXTEND_CAP to REGISTER_VPCI_EXTENDED_CAP.
> >>
> >> 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/header.c | 3 +--
> >> xen/drivers/vpci/msi.c | 2 +-
> >> xen/drivers/vpci/msix.c | 8 +++++--
> >> xen/drivers/vpci/rebar.c | 2 +-
> >> xen/drivers/vpci/vpci.c | 48 +++++++++++++++++++++++++++++++--------
> >> xen/include/xen/vpci.h | 28 ++++++++++++++++-------
> >> xen/include/xen/xen.lds.h | 2 +-
> >> 7 files changed, 68 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> >> index ee94ad8e5037..afe4bcdfcb30 100644
> >> --- a/xen/drivers/vpci/header.c
> >> +++ b/xen/drivers/vpci/header.c
> >> @@ -842,7 +842,7 @@ static int vpci_init_ext_capability_list(struct
> >> pci_dev *pdev)
> >> return 0;
> >> }
> >>
> >> -static int cf_check init_header(struct pci_dev *pdev)
> >> +int vpci_init_header(struct pci_dev *pdev)
> >> {
> >> uint16_t cmd;
> >> uint64_t addr, size;
> >> @@ -1038,7 +1038,6 @@ static int cf_check init_header(struct pci_dev *pdev)
> >> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> >> return rc;
> >> }
> >> -REGISTER_VPCI_INIT(init_header, VPCI_PRIORITY_MIDDLE);
> >>
> >> /*
> >> * Local variables:
> >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> >> index 66e5a8a116be..ea7dc0c060ea 100644
> >> --- a/xen/drivers/vpci/msi.c
> >> +++ b/xen/drivers/vpci/msi.c
> >> @@ -270,7 +270,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
> >>
> >> return 0;
> >> }
> >> -REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
> >> +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi, NULL);
> >>
> >> void vpci_dump_msi(void)
> >> {
> >> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> >> index 6bd8c55bb48e..0228ffd9fda9 100644
> >> --- a/xen/drivers/vpci/msix.c
> >> +++ b/xen/drivers/vpci/msix.c
> >> @@ -751,9 +751,13 @@ static int cf_check init_msix(struct pci_dev *pdev)
> >> pdev->vpci->msix = msix;
> >> list_add(&msix->next, &d->arch.hvm.msix_tables);
> >>
> >> - return 0;
> >> + spin_lock(&pdev->vpci->lock);
> >> + rc = vpci_make_msix_hole(pdev);
> >> + spin_unlock(&pdev->vpci->lock);
> >> +
> >> + return rc;
> >> }
> >> -REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH);
> >> +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, NULL);
> >>
> >> /*
> >> * Local variables:
> >> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> >> index 793937449af7..026f8f7972d9 100644
> >> --- a/xen/drivers/vpci/rebar.c
> >> +++ b/xen/drivers/vpci/rebar.c
> >> @@ -118,7 +118,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
> >>
> >> return 0;
> >> }
> >> -REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
> >> +REGISTER_VPCI_EXTENDED_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, NULL);
> >>
> >> /*
> >> * Local variables:
> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >> index 3349b98389b8..5474b66668c1 100644
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -36,8 +36,8 @@ struct vpci_register {
> >> };
> >>
> >> #ifdef __XEN__
> >> -extern vpci_register_init_t *const __start_vpci_array[];
> >> -extern vpci_register_init_t *const __end_vpci_array[];
> >> +extern vpci_capability_t *const __start_vpci_array[];
> >> +extern vpci_capability_t *const __end_vpci_array[];
> >> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
> >>
> >> #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> >> @@ -83,6 +83,36 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
> >>
> >> #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> >>
> >> +static int vpci_init_capabilities(struct pci_dev *pdev)
> >> +{
> >> + for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
> >> + {
> >> + const vpci_capability_t *capability = __start_vpci_array[i];
> >> + const unsigned int cap = capability->id;
> >> + const bool is_ext = capability->is_ext;
> >> + unsigned int pos;
> >> + int rc;
> >> +
> >> + if ( !is_hardware_domain(pdev->domain) && is_ext )
> >> + continue;
> >> +
> >> + if ( !is_ext )
> >> + pos = pci_find_cap_offset(pdev->sbdf, cap);
> >> + else
> >> + pos = pci_find_ext_capability(pdev->sbdf, cap);
> >> +
> >> + if ( !pos || !capability->init )
> >
> > Isn't it bogus to have a vpci_capability_t entry with a NULL init
> > function?
> Since I don't add fini_x() function for capabilities and also add check "if (
> capability->fini )" below,
> so I add this NULL check here.
> I will remove it if you think it is unnecessary.
> Should I also remove the NULL check for fini?
I think `fini` is fine to be NULL, but I don't see a case for `init`
being NULL?
Maybe I'm missing some use-case, but I expect capabilities will always
need some kind of initialization (iow: setting up handlers) otherwise
it's just a no-op.
> >> + if ( rc )
> >> + return rc;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> void vpci_deassign_device(struct pci_dev *pdev)
> >> {
> >> unsigned int i;
> >> @@ -128,7 +158,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
> >>
> >> int vpci_assign_device(struct pci_dev *pdev)
> >> {
> >> - unsigned int i;
> >> const unsigned long *ro_map;
> >> int rc = 0;
> >>
> >> @@ -159,14 +188,13 @@ int vpci_assign_device(struct pci_dev *pdev)
> >> goto out;
> >> #endif
> >>
> >> - for ( i = 0; i < NUM_VPCI_INIT; i++ )
> >> - {
> >> - rc = __start_vpci_array[i](pdev);
> >> - if ( rc )
> >> - break;
> >> - }
> >> + rc = vpci_init_header(pdev);
> >> + if ( rc )
> >> + goto out;
> >> +
> >> + rc = vpci_init_capabilities(pdev);
> >>
> >> - out: __maybe_unused;
> >> + out:
> >> if ( rc )
> >> vpci_deassign_device(pdev);
> >>
> >> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> >> index 9d47b8c1a50e..8e815b418b7d 100644
> >> --- a/xen/include/xen/vpci.h
> >> +++ b/xen/include/xen/vpci.h
> >> @@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev
> >> *pdev, unsigned int reg,
> >> typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
> >> uint32_t val, void *data);
> >>
> >> -typedef int vpci_register_init_t(struct pci_dev *dev);
> >> -
> >> -#define VPCI_PRIORITY_HIGH "1"
> >> -#define VPCI_PRIORITY_MIDDLE "5"
> >> -#define VPCI_PRIORITY_LOW "9"
> >> +typedef struct {
> >> + unsigned int id;
> >> + bool is_ext;
> >> + int (*init)(struct pci_dev *pdev);
> >> + void (*fini)(struct pci_dev *pdev);
> >> +} vpci_capability_t;
> >>
> >> #define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12)
> >>
> >> @@ -29,9 +30,20 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
> >> */
> >> #define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
> >>
> >> -#define REGISTER_VPCI_INIT(x, p) \
> >> - static vpci_register_init_t *const x##_entry \
> >> - __used_section(".data.vpci." p) = (x)
> >> +#define REGISTER_VPCI_CAP(cap, x, y, ext) \
> >
> > x and y are not very helpful identifier names, better use some more
> > descriptive naming, init and fini? Same below.
> init and fini seems not good. They are conflict with the hook name of below
> vpci_capability_t.
> Maybe init_func and fini_func ?
Oh, I see. Can I recommend to name fields init and destroy or cleanup
(instead of fini), and then use finit and fdestroy/fclean as macro
parameters?
I don't think it's common in Xen to name cleanup functions 'fini'. I
understand this is a question of taste, it's mostly for coherence with
the rest of the code base.
Thanks, Roger.
