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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 23, 2025 at 07:20:27AM +0000, Chen, Jiqian wrote:
> 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);

I would use:

/*
 * vPCI header initialization will have mapped the whole BAR into the
 * p2m, as MSI-X capability was not yet initialized.  Crave a hole for
 * the MSI-X table here, so that Xen can trap accesses.
 */

I think referencing "another is to support dom0..." is not helpful,
and likely to get out of sync if we ever change that code.  If
anything, the comment in modify_decoding() needs updating, but not via
a cross reference from a different context.

Thanks, Roger.