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.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Andrew Cooper <andrew.cooper3@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
cc: Michal Orzel <michal.orzel@amd.com>
cc: Jan Beulich <jbeulich@suse.com>
cc: Julien Grall <julien@xen.org>
cc: Stefano Stabellini <sstabellini@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 )
+            continue;
+
+        rc = capability->init(pdev);
+
+        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) \
+  static vpci_capability_t x##_t = { \
+        .id = (cap), \
+        .init = (x), \
+        .fini = (y), \
+        .is_ext = (ext), \
+  }; \
+  static vpci_capability_t *const x##_entry  \
+               __used_section(".data.vpci.") = &(x##_t)
+
+#define REGISTER_VPCI_LEGACY_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, false)
+#define REGISTER_VPCI_EXTENDED_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, true)
+
+int __must_check vpci_init_header(struct pci_dev *pdev);
 
 /* Assign vPCI to device by adding handlers. */
 int __must_check vpci_assign_device(struct pci_dev *pdev);
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 16a9b1ba03db..c73222112dd3 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -187,7 +187,7 @@
 #define VPCI_ARRAY               \
        . = ALIGN(POINTER_ALIGN); \
        __start_vpci_array = .;   \
-       *(SORT(.data.vpci.*))     \
+       *(.data.vpci.*)     \
        __end_vpci_array = .;
 #else
 #define VPCI_ARRAY
-- 
2.34.1

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.

> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> cc: Andrew Cooper <andrew.cooper3@citrix.com>
> cc: Anthony PERARD <anthony.perard@vates.tech>
> cc: Michal Orzel <michal.orzel@amd.com>
> cc: Jan Beulich <jbeulich@suse.com>
> cc: Julien Grall <julien@xen.org>
> cc: Stefano Stabellini <sstabellini@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?

> +            continue;
> +
> +        rc = capability->init(pdev);
> +

I wouldn't add a newline between the function call and checking the
return value, but that's just my taste.

> +        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.

> +  static vpci_capability_t x##_t = { \
> +        .id = (cap), \
> +        .init = (x), \
> +        .fini = (y), \
> +        .is_ext = (ext), \
> +  }; \
> +  static vpci_capability_t *const x##_entry  \
> +               __used_section(".data.vpci.") = &(x##_t)
> +
> +#define REGISTER_VPCI_LEGACY_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, false)
> +#define REGISTER_VPCI_EXTENDED_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, true)
> +
> +int __must_check vpci_init_header(struct pci_dev *pdev);
>  
>  /* Assign vPCI to device by adding handlers. */
>  int __must_check vpci_assign_device(struct pci_dev *pdev);
> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> index 16a9b1ba03db..c73222112dd3 100644
> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -187,7 +187,7 @@
>  #define VPCI_ARRAY               \
>         . = ALIGN(POINTER_ALIGN); \
>         __start_vpci_array = .;   \
> -       *(SORT(.data.vpci.*))     \
> +       *(.data.vpci.*)     \

Aside from Jan comment, please keep the '\' aligned with the others on
the block.

Thanks, Roger.

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?

> 
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> cc: Anthony PERARD <anthony.perard@vates.tech>
>> cc: Michal Orzel <michal.orzel@amd.com>
>> cc: Jan Beulich <jbeulich@suse.com>
>> cc: Julien Grall <julien@xen.org>
>> cc: Stefano Stabellini <sstabellini@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?

> 
>> +            continue;
>> +
>> +        rc = capability->init(pdev);
>> +
> 
> I wouldn't add a newline between the function call and checking the
> return value, but that's just my taste.
Will do.
> 
>> +        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 ?

> 
>> +  static vpci_capability_t x##_t = { \
>> +        .id = (cap), \
>> +        .init = (x), \
>> +        .fini = (y), \
>> +        .is_ext = (ext), \
>> +  }; \
>> +  static vpci_capability_t *const x##_entry  \
>> +               __used_section(".data.vpci.") = &(x##_t)
>> +
>> +#define REGISTER_VPCI_LEGACY_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, false)
>> +#define REGISTER_VPCI_EXTENDED_CAP(cap, x, y) REGISTER_VPCI_CAP(cap, x, y, true)
>> +
>> +int __must_check vpci_init_header(struct pci_dev *pdev);
>>  
>>  /* Assign vPCI to device by adding handlers. */
>>  int __must_check vpci_assign_device(struct pci_dev *pdev);
>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>> index 16a9b1ba03db..c73222112dd3 100644
>> --- a/xen/include/xen/xen.lds.h
>> +++ b/xen/include/xen/xen.lds.h
>> @@ -187,7 +187,7 @@
>>  #define VPCI_ARRAY               \
>>         . = ALIGN(POINTER_ALIGN); \
>>         __start_vpci_array = .;   \
>> -       *(SORT(.data.vpci.*))     \
>> +       *(.data.vpci.*)     \
> 
> Aside from Jan comment, please keep the '\' aligned with the others on
> the block.
Will do.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

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.Chen@amd.com>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >> cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> cc: Anthony PERARD <anthony.perard@vates.tech>
> >> cc: Michal Orzel <michal.orzel@amd.com>
> >> cc: Jan Beulich <jbeulich@suse.com>
> >> cc: Julien Grall <julien@xen.org>
> >> cc: Stefano Stabellini <sstabellini@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.

On 2025/5/7 16:04, Roger Pau Monné wrote:
> 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:
>>>>
>>>> +        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.
Got it. I will just remove the check of init.

> 
>>>> +        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.
OK, will change to "init    cleanup" and "finit     fclean"

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

On 21.04.2025 08:18, 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.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>

From the description I can't derive the need for ...

> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -187,7 +187,7 @@
>  #define VPCI_ARRAY               \
>         . = ALIGN(POINTER_ALIGN); \
>         __start_vpci_array = .;   \
> -       *(SORT(.data.vpci.*))     \
> +       *(.data.vpci.*)     \
>         __end_vpci_array = .;
>  #else
>  #define VPCI_ARRAY

... this change.

Jan

On 2025/4/23 00:03, Jan Beulich wrote:
> On 21.04.2025 08:18, 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.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> 
> From the description I can't derive the need for ...
> 
>> --- a/xen/include/xen/xen.lds.h
>> +++ b/xen/include/xen/xen.lds.h
>> @@ -187,7 +187,7 @@
>>  #define VPCI_ARRAY               \
>>         . = ALIGN(POINTER_ALIGN); \
>>         __start_vpci_array = .;   \
>> -       *(SORT(.data.vpci.*))     \
>> +       *(.data.vpci.*)     \
>>         __end_vpci_array = .;
>>  #else
>>  #define VPCI_ARRAY
> 
> ... this change.
As I understand this, this is used for initializing all capabilities according to priority before.
That is msix > header > other capabilities.
My patch removes the priority and initializing all capabilities doesn't depend on priority anymore.
So I think this is not needed anymore.
Do you mean I should add some explanation in the commit message?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 23.04.2025 05:49, Chen, Jiqian wrote:
> On 2025/4/23 00:03, Jan Beulich wrote:
>> On 21.04.2025 08:18, 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.
>>>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>
>> From the description I can't derive the need for ...
>>
>>> --- a/xen/include/xen/xen.lds.h
>>> +++ b/xen/include/xen/xen.lds.h
>>> @@ -187,7 +187,7 @@
>>>  #define VPCI_ARRAY               \
>>>         . = ALIGN(POINTER_ALIGN); \
>>>         __start_vpci_array = .;   \
>>> -       *(SORT(.data.vpci.*))     \
>>> +       *(.data.vpci.*)     \
>>>         __end_vpci_array = .;
>>>  #else
>>>  #define VPCI_ARRAY
>>
>> ... this change.
> As I understand this, this is used for initializing all capabilities according to priority before.
> That is msix > header > other capabilities.
> My patch removes the priority and initializing all capabilities doesn't depend on priority anymore.
> So I think this is not needed anymore.

Perhaps, but the word "priority" doesn't even appear in the description. So
yes, ...

> Do you mean I should add some explanation in the commit message?

... there's something to add there. But there's also the question of why the
change doesn't go further: With the SORT() dropped, what's the trailing .*
in the section name for? That's apparently connected to the puzzling

+  static vpci_capability_t *const x##_entry  \
+               __used_section(".data.vpci.") = &(x##_t)

What's the trailing dot for here?

(As a nit - I also don't see why x##_t would need parenthesizing when
x##_entry doesn't. Is there another Misra gem which makes this necessary?)

Jan

On 2025/4/23 15:36, Jan Beulich wrote:
> On 23.04.2025 05:49, Chen, Jiqian wrote:
>> On 2025/4/23 00:03, Jan Beulich wrote:
>>> On 21.04.2025 08:18, 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.
>>>>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>
>>> From the description I can't derive the need for ...
>>>
>>>> --- a/xen/include/xen/xen.lds.h
>>>> +++ b/xen/include/xen/xen.lds.h
>>>> @@ -187,7 +187,7 @@
>>>>  #define VPCI_ARRAY               \
>>>>         . = ALIGN(POINTER_ALIGN); \
>>>>         __start_vpci_array = .;   \
>>>> -       *(SORT(.data.vpci.*))     \
>>>> +       *(.data.vpci.*)     \
>>>>         __end_vpci_array = .;
>>>>  #else
>>>>  #define VPCI_ARRAY
>>>
>>> ... this change.
>> As I understand this, this is used for initializing all capabilities according to priority before.
>> That is msix > header > other capabilities.
>> My patch removes the priority and initializing all capabilities doesn't depend on priority anymore.
>> So I think this is not needed anymore.
> 
> Perhaps, but the word "priority" doesn't even appear in the description. So
> yes, ...
I will add description about "priority" removal in commit message in next version.

> 
>> Do you mean I should add some explanation in the commit message?
> 
> ... there's something to add there. But there's also the question of why the
> change doesn't go further: With the SORT() dropped, what's the trailing .*
> in the section name for? That's apparently connected to the puzzling
> 
> +  static vpci_capability_t *const x##_entry  \
> +               __used_section(".data.vpci.") = &(x##_t)
> 
> What's the trailing dot for here?
Thanks for catching this problem.
I forgot to delete the dot and ".*", will delete them in next version.

> 
> (As a nit - I also don't see why x##_t would need parenthesizing when
> x##_entry doesn't. Is there another Misra gem which makes this necessary?)
Oh, I will delete the parentheses in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.