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

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

On 2025/6/25 16:36, Jan Beulich wrote:
> On 25.06.2025 08:51, Chen, Jiqian wrote:
>> 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
> 
> Oh, of course - there's a psABI peculiarity that you run into here: Aggregates
> larger than 8 bytes are required to have 16-byte alignment. Hence while
> sizeof() == 0x18 and __alignof() == 8, the section contributions still are
> accompanied by ".align 16", and thus respective padding is inserted by
> assembler and linker. IOW you end up with two 32-byte entries and a trailing
> 24-byte one. The easiest (and least problematic going forward) approach to
> deal with that is probably going to be to add __aligned(16) to the struct
> decl. (Whether to limit this to just x86 I'm not sure: While other psABI-s may
> be different in this regard, we may want to be on the safe side.)
Thanks for you detailed explanation.
If I understand correctly, I need to change the definition of vpci_capability_t 
to be:

typedef struct {
    unsigned int id;
    bool is_ext;
    int (* init)(const struct pci_dev *pdev);
    int (* cleanup)(const struct pci_dev *pdev);
}
#ifdef CONFIG_X86
__aligned(16)
#endif
vpci_capability_t;

> 
> However, there looks to be another (gcc) anomaly: By default, half-way recent
> gcc aligns this kind of object even to 32-byte boundaries, due to defaulting
> to -malign-data=compat. We will want to consider to use -malign-data=abi
> instead (supported by gcc5 and newer). I'm in the process of preparing a patch
> to propose this more formally.
> 
> Jan

-- 
Best regards,
Jiqian Chen.