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

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

On 2025/6/26 20:06, Jan Beulich wrote:
> On 26.06.2025 10:03, Chen, Jiqian wrote:
>> On 2025/6/25 22:07, Jan Beulich wrote:
>>> On 25.06.2025 12:16, Chen, Jiqian wrote:
>>>> On 2025/6/25 18:03, Jan Beulich wrote:
>>>>> 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.
> 
> Note how I used __aligned() here. Why would you ...
> 
>>>> That's difficult for me to check on all different platforms since I don't 
>>>> have them all.
>>>
>>> You don't need to have them. You'd need to carefully go through the 
>>> respective
>>> section(s) of their psABI-s.
>>>
>>>> So you mean I should remove "#ifdef CONFIG_X86"? Just let __aligned(16) 
>>>> for all platforms?
>>>
>>> Yes. And, as also said, with a suitable comment please.
>> Ah, my comment definitely needs your change suggestion.
>> I wrote a draft as below:
>>
>> /*
>>  * Size of vpci_capability is lager than 8 bytes. When it is used as the 
>> entry
>>  * of __start_vpci_array in section, it is 16-byte aligned by assembler, that
>>  * causes the array length (__end_vpci_array - __start_vpci_array) wrong, so
>>  * force its definition to use 16-byte aligned here.
>>  */
>> struct vpci_capability {
>>     unsigned int id;
>>     bool is_ext;
>>     int (* init)(const struct pci_dev *pdev);
>>     int (* cleanup)(const struct pci_dev *pdev);
>> } __attribute__((aligned(16)));
> 
> ... open-code that here?
That because when using __aligned() without CONFIG_X86, I got compile error
vpci.h:18:13: error: expected declaration specifiers or ‘...’ before numeric 
constant
   18 | } __aligned(16);
      |             ^~
I tried some methods, only open-code can fix it.

> 
> As to the comment: First, it wants to be as close to what is being commented 
> as
> possible. Hence
> 
> struct __aligned(16) vpci_capability {
This also got the compile error.
> 
> is likely the better placement. Second, there's nothing here the assembler 
> does
> on its own. It's the compiler which does something (insert alignment 
> directives),
> and only to follow certain rules. (See "x86: don't have gcc over-align data"
> that I Cc-ed you on for some of the relevant aspects.) That is, you don't want
> to "blame" any part of the tool chain, at least not where it's the underlying
> ABI that mandates certain behavior. There's also no strong need to talk about
> the specific effects that it would have if we didn't arrange things properly.
> That is, talking about the effect on arrays in general is fine and helpful.
> Talking about __{start,end}_vpci_array imo is not.
> 
> While further playing with the compiler, I noticed that adding __aligned(16)
> actually has a negative effect as long as that other patch isn't in use: The
> struct instances then are being aligned to even 32-byte boundaries (which 
> means
> __start_vpci_array would then also need aligning as much). When that other
> patch is in use, the __aligned() becomes unnecessary. Therefore I'm no longer
> convinced using __aligned() is the best solution here.
Em, changing __start_vpci_array to be struct vpci_capability array cause those 
concerns, maybe keeping using struct pointer is a compromise method.

> Instead I think you want to base your patch on top of mine. Which in turn 
> would eliminate the need for
> any commentary here.
I am fine to wait until your patch is merged.

> 
> Jan

-- 
Best regards,
Jiqian Chen.