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

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

On 2025/6/27 14:05, Jan Beulich wrote:
> On 27.06.2025 04:59, Chen, Jiqian wrote:
>> 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.
> 
> Well, that's odd. In e.g. xen/sched.h we have
Yes, I see.
That's so strange.
Even I do below also get the error message. But if open-code, it works.

diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 51573baabc..350eb5f289 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -13,12 +13,17 @@ typedef uint32_t vpci_read_t(const struct pci_dev *pdev, 
unsigned int reg,
 typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
                           uint32_t val, void *data);

+#ifdef __aligned
+#undef __aligned
+#define __aligned(a) __attribute__((__aligned__(a)))
+#endif
+
 struct vpci_capability {
     unsigned int id;
     bool is_ext;
     int (* const init)(struct pci_dev *pdev);
     int (* const cleanup)(struct pci_dev *pdev);
} __aligned(16);


Anyway, I will wait for your patch merged.

> 
> struct domain
> {
>     ...
> } __aligned(PAGE_SIZE);
> 
> which clearly must be working fine. The error message from the compiler
> doesn't say very much alone. For informational diagnostics the compiler
> normally also emits may help, or else it would take looking at the
> pre-processed output to understand what's going on 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 {
>> 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.
> 
> It would be a last resort, yes, but imo (a) we ought to strive to avoid
> unnecessary indirection and (b) the same underlying issue could become a
> problem elsewhere as well.
> 
> Jan

-- 
Best regards,
Jiqian Chen.