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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

As to the comment: First, it wants to be as close to what is being commented as
possible. Hence

struct __aligned(16) vpci_capability {

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. Instead I think you want
to base your patch on top of mine. Which in turn would eliminate the need for
any commentary here.

Jan