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

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

On 2025/4/23 15:36, Jan Beulich wrote:
> 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, ...
I will add description about "priority" removal in commit message in next 
version.

> 
>> 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?
Thanks for catching this problem.
I forgot to delete the dot and ".*", will delete them in next version.

> 
> (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?)
Oh, I will delete the parentheses in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.