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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?
At the first glance the changes above are what I would have expected.

Jan