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

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

On 2025/6/27 16:16, 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
> 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
Sorry, this should be under the end of the struct, then it works.
It seems __aligned is over-write?

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