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

[Jan Beulich <jbeulich@xxxxxxxx>]

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