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

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

On 2025/6/24 18:08, Jan Beulich wrote:
> On 24.06.2025 11:29, Chen, Jiqian wrote:
>> On 2025/6/24 16:05, Jan Beulich wrote:
>>> On 24.06.2025 10:02, Chen, Jiqian wrote:
>>>> On 2025/6/20 14:38, Jan Beulich wrote:
>>>>> On 19.06.2025 08:39, Chen, Jiqian wrote:
>>>>>> On 2025/6/18 22:05, Jan Beulich wrote:
>>>>>>> On 12.06.2025 11:29, Jiqian Chen wrote:
>>>>>>>> @@ -29,9 +30,22 @@ 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, finit, fclean, ext) \
>>>>>>>> +    static const vpci_capability_t finit##_t = { \
>>>>>>>> +        .id = (cap), \
>>>>>>>> +        .init = (finit), \
>>>>>>>> +        .cleanup = (fclean), \
>>>>>>>> +        .is_ext = (ext), \
>>>>>>>> +    }; \
>>>>>>>> +    static const vpci_capability_t *const finit##_entry  \
>>>>>>>> +        __used_section(".data.rel.ro.vpci") = &finit##_t
>>>>>>>
>>>>>>> Could you remind me why the extra level of indirection is necessary 
>>>>>>> here?
>>>>>>> That is, why can't .data.rel.ro.vpci be an array of vpci_capability_t?
>>>>>> You mean I should change to be:
>>>>>> #define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
>>>>>>     static const vpci_capability_t finit##_t \
>>>>>>         __used_section(".data.rel.ro.vpci") = { \
>>>>>>         .id = (cap), \
>>>>>>         .init = (finit), \
>>>>>>         .cleanup = (fclean), \
>>>>>>         .is_ext = (ext), \
>>>>>>     }
>>>>>>
>>>>>> Right?
>>>>>
>>>>> Yes, subject to the earlier comments on the identifier choice.
>>>> Got it.
>>>> One more question, if change to be that, then how should I modify the 
>>>> definition of VPCI_ARRAY?
>>>> Is POINTER_ALIGN still right?
>>>
>>> Yes. The struct doesn't require bigger alignment afaics. (In fact in 
>>> principle
>>> no alignment should need specifying there, except that this would require
>>> keeping the section separate in the final image. Which I don't think we 
>>> want.)
>>>
>>>> Since I encountered errors that the values of __start_vpci_array are not 
>>>> right when I use them in vpci_init_capabilities().
>>>
>>> Details please.
>> After changing __start_vpci_array to be vpci_capability_t array, codes will 
>> be (maybe I modified wrong somewhere):
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index c51bbb8abb19..9f2f438b4fdd 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -36,8 +36,8 @@ struct vpci_register {
>>  };
>>
>>  #ifdef __XEN__
>> -extern const vpci_capability_t *const __start_vpci_array[];
>> -extern const vpci_capability_t *const __end_vpci_array[];
>> +extern vpci_capability_t __start_vpci_array[];
>> +extern vpci_capability_t __end_vpci_array[];
> 
> Just fyi: You lost const here.
> 
>> @@ -255,7 +255,7 @@ 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 vpci_capability_t *capability = &__start_vpci_array[i];
>>          const unsigned int cap = capability->id;
>>          const bool is_ext = capability->is_ext;
>>          int rc;
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index f4ec1c25922d..77750dd4131a 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -31,14 +31,13 @@ typedef struct {
>>  #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
>>
>>  #define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
>> -    static const vpci_capability_t finit##_t = { \
>> +    static vpci_capability_t finit##_entry \
>> +        __used_section(".data.rel.ro.vpci") = { \
>>          .id = (cap), \
>>          .init = (finit), \
>>          .cleanup = (fclean), \
>>          .is_ext = (ext), \
>> -    }; \
>> -    static const vpci_capability_t *const finit##_entry  \
>> -        __used_section(".data.rel.ro.vpci") = &finit##_t
>> +    }
>>
>>  #define REGISTER_VPCI_CAP(cap, finit, fclean) \
>>      REGISTER_VPCI_CAPABILITY(cap, finit, fclean, false)
>>
>> I print the value of NUM_VPCI_INIT, it is a strange number 
>> (6148914691236517209).
> 
> What are the addresses of the two symbols __start_vpci_array and 
> __end_vpci_array?
__end_vpci_array is 0xffff82d0404251b8
__start_vpci_array is 0xffff82d040425160
NUM_VPCI_INIT is 0x5555555555555559
sizeof(vpci_capability_t) is 0x18

> At the first glance the changes above are what I would have expected.
> 
> Jan

-- 
Best regards,
Jiqian Chen.