Re: [PATCH v3 05/11] vpci: Refactor REGISTER_VPCI_INIT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2025 05:49, Chen, Jiqian wrote:
> On 2025/4/23 00:03, Jan Beulich wrote:
>> On 21.04.2025 08:18, Jiqian Chen wrote:
>>> Refactor REGISTER_VPCI_INIT to contain more capability specific
>>> information, this is benifit for follow-on changes to hide capability
>>> which initialization fails.
>>>
>>> What's more, change the definition of init_header() since it is
>>> not a capability and it is needed for all devices' PCI config space.
>>>
>>> Note:
>>> Call vpci_make_msix_hole() in the end of init_msix() since the
>>> change of sequence of init_header() and init_msix().
>>> The fini hook will be implemented in follow-on changes.
>>>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>
>> From the description I can't derive the need for ...
>>
>>> --- a/xen/include/xen/xen.lds.h
>>> +++ b/xen/include/xen/xen.lds.h
>>> @@ -187,7 +187,7 @@
>>>  #define VPCI_ARRAY               \
>>>         . = ALIGN(POINTER_ALIGN); \
>>>         __start_vpci_array = .;   \
>>> -       *(SORT(.data.vpci.*))     \
>>> +       *(.data.vpci.*)     \
>>>         __end_vpci_array = .;
>>>  #else
>>>  #define VPCI_ARRAY
>>
>> ... this change.
> As I understand this, this is used for initializing all capabilities 
> according to priority before.
> That is msix > header > other capabilities.
> My patch removes the priority and initializing all capabilities doesn't 
> depend on priority anymore.
> So I think this is not needed anymore.

Perhaps, but the word "priority" doesn't even appear in the description. So
yes, ...

> Do you mean I should add some explanation in the commit message?

... there's something to add there. But there's also the question of why the
change doesn't go further: With the SORT() dropped, what's the trailing .*
in the section name for? That's apparently connected to the puzzling

+  static vpci_capability_t *const x##_entry  \
+               __used_section(".data.vpci.") = &(x##_t)

What's the trailing dot for here?

(As a nit - I also don't see why x##_t would need parenthesizing when
x##_entry doesn't. Is there another Misra gem which makes this necessary?)

Jan