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.
