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

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

On 2025/6/25 18:03, Jan Beulich wrote:
> On 25.06.2025 11:27, Chen, Jiqian wrote:
>> 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;
> 
> You'll need to check whether this has the intended effect. There are yet more
> peculiarities when it comes to attributes on structs, typedefs, and the
> combination of the two. I wonder though: Do we really need a typedef here?
> Going with just struct vcpi_capability would eliminate concerns over those
> peculiarities.
Yes, on x86 this works now.
As for the typedef, that's fine for me to just use struct vpci_capability.

> 
> Also, as said - you will need to check whether other architectures are
> different from x86-64 in this regard. We better wouldn't leave a trap here,
> for them to fall into when they enable vPCI support. I.e. my recommendation
> would be that if in doubt, we put the __aligned() there unconditionally.
That's difficult for me to check on all different platforms since I don't have 
them all.
So you mean I should remove "#ifdef CONFIG_X86"? Just let __aligned(16) for all 
platforms?

> 
> Jan

-- 
Best regards,
Jiqian Chen.