On 27.06.2025 11:00, Chen, Jiqian wrote:
> 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
>>
>> 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.
>
> I add some codes to print the macro __align, the codes are:
>
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 51573baabc..8f6af1c822 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -13,12 +13,16 @@ 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);
>
> +#define STRINGIFY(x) #x
> +#define TOSTRING(x) STRINGIFY(x)
> +#pragma message("__aligned(16) expands to: " TOSTRING(__aligned(16)))
> +
> struct vpci_capability {
> unsigned int id;
> bool is_ext;
> int (* init)(const struct pci_dev *pdev);
> int (* cleanup)(const struct pci_dev *pdev);
> } __aligned(16);
>
> The result are:
>
> In file included from ./include/xen/sched.h:25,
> from arch/x86/x86_64/asm-offsets.c:11:
> ./include/xen/vpci.h:18:9: note: ‘#pragma message: __aligned(16) expands to:
> __attribute__((__aligned__(16)))’
> 18 | #pragma message("__aligned(16) expands to: " TOSTRING(__aligned(16)))
> | ^~~~~~~
> In file included from ./include/xen/sched.h:25,
> from drivers/vpci/vpci.c:20:
> ./include/xen/vpci.h:18:9: note: ‘#pragma message: __aligned(16) expands to:
> __attribute__((__aligned__(16)))’
> 18 | #pragma message("__aligned(16) expands to: " TOSTRING(__aligned(16)))
> | ^~~~~~~
> In file included from emul.h:88,
> from vpci.c:18:
> vpci.h:15:9: note: ‘#pragma message: __aligned(16) expands to: __aligned(16)’
> 15 | #pragma message("__aligned(16) expands to: " TOSTRING(__aligned(16)))
> | ^~~~~~~
> vpci.h:22:13: error: expected declaration specifiers or ‘...’ before numeric
> constant
> 22 | } __aligned(16);
> | ^~
> In file included from emul.h:88,
> from main.c:19:
> vpci.h:15:9: note: ‘#pragma message: __aligned(16) expands to: __aligned(16)’
> 15 | #pragma message("__aligned(16) expands to: " TOSTRING(__aligned(16)))
> | ^~~~~~~
> vpci.h:22:13: error: expected declaration specifiers or ‘...’ before numeric
> constant
> 22 | } __aligned(16);
> | ^~
> make[6]: *** [Makefile:18: test_vpci] Error 1
> make[5]: ***
> [/home/cjq/code/upstream/xen/tools/tests/../../tools/Rules.mk:194:
> subdir-install-vpci] Error 2
> make[4]: ***
> [/home/cjq/code/upstream/xen/tools/tests/../../tools/Rules.mk:189:
> subdirs-install] Error 2
> make[3]: *** [/home/cjq/code/upstream/xen/tools/../tools/Rules.mk:194:
> subdir-install-tests] Error 2
> make[2]: *** [/home/cjq/code/upstream/xen/tools/../tools/Rules.mk:189:
> subdirs-install] Error 2
> make[1]: *** [Makefile:64: install] Error 2
> make: *** [Makefile:147: install-tools] Error 2
> make: *** Waiting for unfinished jobs....
Well, it would have helped a lot if you had said from the very beginning that
it's
the test harness where you observe the build issue. These test harnesses are
always
a little special, due to their intention to re-use the core source file(s). (And
no, pulling in compiler.h likely wouldn't be the right workaround.)
Jan
