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

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

On 2025/7/24 22:28, Roger Pau Monné wrote:
> On Thu, Jul 24, 2025 at 01:50:00PM +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().
>>
>> 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>
>> ---
>> 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>
>> ---
>> v7->v8 changes:
>> * Recover vpci_make_msix_hole() call in modify_decoding(), which was  
>> deleted by wrong.
>> * Add some comment to describe why need to add vpci_make_msix_hole() in 
>> init_msix().
>>
>> 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 |  3 +--
>>  xen/drivers/vpci/msi.c    |  2 +-
>>  xen/drivers/vpci/msix.c   | 13 ++++++++++--
>>  xen/drivers/vpci/rebar.c  |  2 +-
>>  xen/drivers/vpci/vpci.c   | 44 ++++++++++++++++++++++++++++++---------
>>  xen/include/xen/vpci.h    | 29 +++++++++++++++++++-------
>>  xen/include/xen/xen.lds.h |  2 +-
>>  11 files changed, 74 insertions(+), 32 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 f537f3f25d2a..469f4977441a 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -858,7 +858,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;
>> @@ -1054,7 +1054,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..eb3e7dcd1068 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -703,9 +703,18 @@ 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;
>> +    /*
>> +     * 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.
>> +     */
>> +    spin_lock(&pdev->vpci->lock);
>> +    rc = vpci_make_msix_hole(pdev);
>> +    spin_unlock(&pdev->vpci->lock);
> 
> I should have asked in the last version, but why do you take the vPCI
> lock here?
> 
> The path that ASSERTs the lock is held should never be taken when
> called from init_msix().  Is there some path I'm missing in
> vpci_make_msix_hole() that requires the vCPI lock to be held?
> 
> The rest LGTM.
Sorry to forget to delete this.
Feel free to change when submit.
Or I will change by sending a new version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.