Re: [PATCH v7 2/8] vpci: Refactor REGISTER_VPCI_INIT

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/7/21 22:37, Roger Pau Monné wrote:
> On Fri, Jul 04, 2025 at 03:07:57PM +0800, Jiqian Chen wrote:
>> Refactor REGISTER_VPCI_INIT to contain more capability specific
>> information, this will benefit further follow-on changes to hide
>> capability when 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.
>>
>> After refactor, the "priority" of initializing capabilities isn't
>> needed anymore, so delete its related codes.
>>
>> Note:
>> Call vpci_make_msix_hole() in the end of init_msix() since the change
>> of sequence of init_header() and init_msix(). And delete the call of
>> vpci_make_msix_hole() in modify_decoding() since it is not needed.
>>
>> 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@xxxxxxx>
>> ---
>> There is a byte alignment problem in the array __start_vpci_array, which can 
>> be solved after
>> "[PATCH] x86: don't have gcc over-align data" is merged.
>> ---
>> 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>
>> ---
>> v6->v7 changes:
>> * Change the pointer parameter of cleanup hook of vpci_capability_t to be 
>> const.
>>   If change parameter of init hook to be const will affect init_msix, and it 
>> assigns pdev
>>   to struct vpci_msix, so keep no const to expanding the impact.
>> * Delete the vpci_make_msix_hole() call in modify_decoding().
>> * Change __start_vpci_array from vpci_capability_t* array to 
>> vpci_capability_t array.
>> * Change the name "finit##_t" to be "name##_entry" and add a "name" 
>> parameter to macro
>>   REGISTER_VPCI_CAPABILITY.
>>
>> v5->v6 changes:
>> * Rename REGISTER_PCI_CAPABILITY to REGISTER_VPCI_CAPABILITY.
>> * Move vpci_capability_t entry from ".data.vpci" to ".data.rel.ro.vpci" and
>>   move the instances of VPCI_ARRAY in the linker scripts before 
>> *(.data.rel.ro).
>> * Change _start/end_vpci_array[] to be const pointer array.
>>
>> v4->v5 changes:
>> * Rename REGISTER_VPCI_CAP to REGISTER_PCI_CAPABILITY, rename 
>> REGISTER_VPCI_LEGACY_CAP to
>>   REGISTER_VPCI_CAP, rename REGISTER_VPCI_EXTENDED_CAP to 
>> REGISTER_VPCI_EXTCAP.
>> * Change cleanup hook of vpci_capability_t from void to int.
>>
>> v3->v4 changes
>> * Delete the useless trailing dot of section ".data.vpci".
>> * Add description about priority since this patch removes the initializing 
>> priority of
>>   capabilities and priority is not needed anymore.
>> * Change the hook name from fini to cleanup.
>> * Change the name x and y to be finit and fclean.
>> * Remove the unnecessary check "!capability->init"
>>
>> 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/arch/arm/xen.lds.S    |  3 +--
>>  xen/arch/ppc/xen.lds.S    |  3 +--
>>  xen/arch/riscv/xen.lds.S  |  3 +--
>>  xen/arch/x86/xen.lds.S    |  2 +-
>>  xen/drivers/vpci/header.c | 16 +-------------
>>  xen/drivers/vpci/msi.c    |  2 +-
>>  xen/drivers/vpci/msix.c   | 11 +++++++---
>>  xen/drivers/vpci/rebar.c  |  2 +-
>>  xen/drivers/vpci/vpci.c   | 44 ++++++++++++++++++++++++++++++---------
>>  xen/include/xen/vpci.h    | 32 ++++++++++++++++++----------
>>  xen/include/xen/xen.lds.h |  2 +-
>>  11 files changed, 71 insertions(+), 49 deletions(-)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 5bfbe1e92c1e..9f30c3a13ed1 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -57,6 +57,7 @@ SECTIONS
>>  
>>         *(.rodata)
>>         *(.rodata.*)
>> +       VPCI_ARRAY
>>         *(.data.rel.ro)
>>         *(.data.rel.ro.*)
>>  
>> @@ -64,8 +65,6 @@ SECTIONS
>>         __proc_info_start = .;
>>         *(.proc.info)
>>         __proc_info_end = .;
>> -
>> -       VPCI_ARRAY
>>    } :text
>>  
>>  #if defined(BUILD_ID)
>> diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
>> index 1366e2819eed..1de0b77fc6b9 100644
>> --- a/xen/arch/ppc/xen.lds.S
>> +++ b/xen/arch/ppc/xen.lds.S
>> @@ -51,11 +51,10 @@ SECTIONS
>>  
>>          *(.rodata)
>>          *(.rodata.*)
>> +        VPCI_ARRAY
>>          *(.data.rel.ro)
>>          *(.data.rel.ro.*)
>>  
>> -        VPCI_ARRAY
>> -
>>          . = ALIGN(POINTER_ALIGN);
>>      } :text
>>  
>> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
>> index 8c3c06de01f6..edcadff90bfe 100644
>> --- a/xen/arch/riscv/xen.lds.S
>> +++ b/xen/arch/riscv/xen.lds.S
>> @@ -46,11 +46,10 @@ SECTIONS
>>  
>>          *(.rodata)
>>          *(.rodata.*)
>> +        VPCI_ARRAY
>>          *(.data.rel.ro)
>>          *(.data.rel.ro.*)
>>  
>> -        VPCI_ARRAY
>> -
>>          . = ALIGN(POINTER_ALIGN);
>>      } :text
>>  
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index 636c7768aa3c..8e9cac75b09e 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -135,6 +135,7 @@ SECTIONS
>>  
>>         *(.rodata)
>>         *(.rodata.*)
>> +       VPCI_ARRAY
>>         *(.data.rel.ro)
>>         *(.data.rel.ro.*)
>>  
>> @@ -148,7 +149,6 @@ SECTIONS
>>         *(.note.gnu.build-id)
>>         __note_gnu_build_id_end = .;
>>  #endif
>> -       VPCI_ARRAY
>>    } PHDR(text)
>>  
>>  #if defined(CONFIG_PVH_GUEST) && !defined(EFI)
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 8ee8052cd4a3..069253b5f721 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -122,19 +122,6 @@ static void modify_decoding(const struct pci_dev *pdev, 
>> uint16_t cmd,
>>      bool map = cmd & PCI_COMMAND_MEMORY;
>>      unsigned int i;
>>  
>> -    /*
>> -     * Make sure there are no mappings in the MSIX MMIO areas, so that 
>> accesses
>> -     * can be trapped (and emulated) by Xen when the memory decoding bit is
>> -     * enabled.
>> -     *
>> -     * FIXME: punching holes after the p2m has been set up might be racy for
>> -     * DomU usage, needs to be revisited.
>> -     */
>> -#ifdef CONFIG_HAS_PCI_MSI
>> -    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
>> -        return;
>> -#endif
> 
> I think you need to keep this.  What about BARs being repositioned by
> dom0 over reserved region(s), and thus needing the MSI-X hole to be
> craved out there?  It's not a common use-case, but we should support
> dom0 moving BARs around.
> 
> I think you need both the added chunk in init_msix(), plus the code
> above to not regress the current functionality.
OK, will do.
As Jan required me to add some comment to describe the chunk in init_msix() if 
not to delete here.
Do you think below is appropriate?

    /*
     * To make sure there's a hole for the MSIX table/PBA in the p2m since
     * init_msix is called after init_header. Here and the calling in another
     * place are not redundant, another is to support dom0 moving BARs.
     */
    spin_lock(&pdev->vpci->lock);
    rc = vpci_make_msix_hole(pdev);
    spin_unlock(&pdev->vpci->lock);

    return rc;
}
REGISTER_VPCI_CAP(MSIX, init_msix, cleanup_msix);

> 
>>      for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>>      {
>>          struct vpci_bar *bar = &header->bars[i];
>> @@ -869,7 +856,7 @@ static int vpci_init_ext_capability_list(const 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;
>> @@ -1065,7 +1052,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..c3eba4e14870 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_CAP(MSI, init_msi, NULL);
>>  
>>  void vpci_dump_msi(void)
>>  {
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 74211301ba10..a1692b9d9f6a 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -589,7 +589,8 @@ static const struct hvm_mmio_ops vpci_msix_table_ops = {
>>      .write = msix_write,
>>  };
>>  
>> -int vpci_make_msix_hole(const struct pci_dev *pdev)
>> +/* Make sure there's a hole in the p2m for the MSIX mmio areas. */
>> +static int vpci_make_msix_hole(const struct pci_dev *pdev)
>>  {
>>      struct domain *d = pdev->domain;
>>      unsigned int i;
>> @@ -703,9 +704,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_CAP(MSIX, init_msix, NULL);
>>  
>>  /*
>>   * Local variables:
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> index 793937449af7..3c18792d9bcd 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_EXTCAP(REBAR, init_rebar, NULL);
>>  
>>  /*
>>   * Local variables:
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 8474c0e3b995..e7e5b64f1be4 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 const vpci_capability_t __start_vpci_array[];
>> +extern const vpci_capability_t __end_vpci_array[];
>>  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>  
>>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> @@ -83,6 +83,32 @@ 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 = 0;
>> +        int rc;
>> +
>> +        if ( !is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>> +        else if ( is_hardware_domain(pdev->domain) )
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
>> +
>> +        if ( !pos )
>> +            continue;
>> +
>> +        rc = capability->init(pdev);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  void vpci_deassign_device(struct pci_dev *pdev)
>>  {
>>      unsigned int i;
>> @@ -128,7 +154,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 +184,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 61d16cc8b897..61287e5d2e12 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);
>> +    int (* cleanup)(const struct pci_dev *pdev);
>> +} vpci_capability_t;
>>  
>>  #define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
>>  
>> @@ -29,9 +30,21 @@ 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_CAPABILITY(cap, name, finit, fclean, ext) \
>> +    static const vpci_capability_t name##_entry \
>> +        __used_section(".data.rel.ro.vpci") = { \
>> +        .id = (cap), \
>> +        .init = (finit), \
>> +        .cleanup = (fclean), \
>> +        .is_ext = (ext), \
>> +    }
>> +
>> +#define REGISTER_VPCI_CAP(name, finit, fclean) \
>> +    REGISTER_VPCI_CAPABILITY(PCI_CAP_ID_##name, name, finit, fclean, false)
>> +#define REGISTER_VPCI_EXTCAP(name, finit, fclean) \
>> +    REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##name, name, finit, fclean, 
>> 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);
>> @@ -206,9 +219,6 @@ struct vpci_vcpu {
>>  #ifdef __XEN__
>>  void vpci_dump_msi(void);
>>  
>> -/* Make sure there's a hole in the p2m for the MSIX mmio areas. */
>> -int vpci_make_msix_hole(const struct pci_dev *pdev);
>> -
>>  /* Arch-specific vPCI MSI helpers. */
>>  void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev,
>>                          unsigned int entry, bool mask);
>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>> index 793d0e11450c..eb86305c11c7 100644
>> --- a/xen/include/xen/xen.lds.h
>> +++ b/xen/include/xen/xen.lds.h
>> @@ -188,7 +188,7 @@
>>  #define VPCI_ARRAY               \
>>         . = ALIGN(POINTER_ALIGN); \
>>         __start_vpci_array = .;   \
>> -       *(SORT(.data.vpci.*))     \
>> +       *(.data.rel.ro.vpci)           \
> 
> Indentation of the trailing '\' seems to be off?
Will change.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.