Re: [PATCH v4 04/10] vpci: Refactor REGISTER_VPCI_INIT

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

On 2025/5/18 22:34, Jan Beulich wrote:
> On 09.05.2025 11:05, Jiqian Chen wrote:
>> --- 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);
> 
> To keep identifier length bounded, how about REGISTER_VPCI_CAP() here
> and ...
> 
>> --- 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);
> 
> ... and REGISTER_VPCI_EXTCAP() here?

If so, I need to change the name of REGISTER_VPCI_CAP to be _REGISTER_VPCI_CAP ?

#define REGISTER_VPCI_CAP(cap, finit, fclean, ext) \
  static vpci_capability_t finit##_t = { \
        .id = (cap), \
        .init = (finit), \
        .cleanup = (fclean), \
        .is_ext = (ext), \
  }; \
  static vpci_capability_t *const finit##_entry  \
               __used_section(".data.vpci") = &finit##_t

> 
>> @@ -83,6 +83,35 @@ 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;
> 
> Fold this into ...
> 
>> +        if ( !is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>> +        else
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
> 
> ... this by adding a middle "else if()"?
It seems better, will do.
Thanks.

> 
> Jan

-- 
Best regards,
Jiqian Chen.