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.
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.
Jan
