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.
