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

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

On 2025/6/27 17: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,

Ah, the root cause is that I need to add compiler.h to tests directory 
tools/tests/vpci/
Like below, but we will use your patch, so this problem is not important.
diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index 97359ff67f..11de341ab9 100644
--- a/tools/tests/vpci/Makefile
+++ b/tools/tests/vpci/Makefile
@@ -14,12 +14,12 @@ else
        $(warning HOSTCC != CC, will not run test)
 endif

-$(TARGET): vpci.c vpci.h list.h main.c emul.h
+$(TARGET): vpci.c vpci.h list.h main.c emul.h compiler.h
        $(CC) $(CFLAGS_xeninclude) -g -o $@ vpci.c main.c

 .PHONY: clean
 clean:
-       rm -rf $(TARGET) *.o *~ vpci.h vpci.c list.h
+       rm -rf $(TARGET) *.o *~ vpci.h vpci.c list.h compiler.h

 .PHONY: distclean
 distclean: clean
@@ -38,6 +38,7 @@ vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
        sed -e '/#include/d' -e '1s/^/#include "emul.h"/' <$< >$@

 list.h: $(XEN_ROOT)/xen/include/xen/list.h
+compiler.h: $(XEN_ROOT)/xen/include/xen/compiler.h
 vpci.h: $(XEN_ROOT)/xen/include/xen/vpci.h
-list.h vpci.h:
+list.h compiler.h vpci.h:
        sed -e '/#include/d' <$< >$@

>                  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....
> 
>>
>>>> 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.