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

2025-05-19 Thread Jan Beulich 
On 19.05.2025 08:56, Chen, Jiqian wrote:
> 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

That's a reserved name then. Since it's used only twice (to produce the
other two macros), REGISTER_PCI_CAPABILITY() maybe? Or one of
REGISTER_PCI__CAP() / REGISTER_PCI_CAP_()?

Jan

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

2025-05-18 Thread Chen, Jiqian 
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.

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

2025-05-18 Thread Jan Beulich 
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?

> @@ -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()"?

Jan