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

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