Re: [PATCH v5 04/10] vpci: Refactor REGISTER_VPCI_INIT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2025 05:52, Chen, Jiqian wrote:
> On 2025/6/9 18:40, Roger Pau Monné wrote:
>> On Mon, Jun 09, 2025 at 10:18:42AM +0000, Chen, Jiqian wrote:
>>> On 2025/6/9 17:29, Roger Pau Monné wrote:
>>>> On Mon, Jun 09, 2025 at 07:50:21AM +0000, Chen, Jiqian wrote:
>>>>> On 2025/6/6 17:14, Roger Pau Monné wrote:
>>>>>> On Fri, Jun 06, 2025 at 09:05:48AM +0200, Jan Beulich wrote:
>>>>>>> On 06.06.2025 08:29, Chen, Jiqian wrote:
>>>>>>>> On 2025/6/5 20:50, Roger Pau Monné wrote:
>>>>>>>>> On Mon, May 26, 2025 at 05:45:53PM +0800, Jiqian Chen wrote: 
>>>>>>>>>> +  }; \
>>>>>>>>>> +  static vpci_capability_t *const finit##_entry  \
>>>>>>>>>> +               __used_section(".data.vpci") = &finit##_t
>>>>>>>>>
>>>>>>>>> IMO this should better use .rodata instead of .data. 
>>>>>>>> Is below change correct?
>>>>>>>>
>>>>>>>> +    static const vpci_capability_t *const finit##_entry  \
>>>>>>>> +        __used_section(".rodata") = &finit##_t
>>>>>>>
>>>>>>> No, specifically because ...
>>>>>>>
>>>>>>>>> Not that it matters much in practice, as we place it in .rodata 
>>>>>>>>> anyway.  Note
>>>>>>>>> however you will have to move the placement of the VPCI_ARRAY in the
>>>>>>>>> linker script ahead of *(.rodata.*), otherwise that section match will
>>>>>>>>> consume the vPCI data.
>>>>>>>> I am sorry, how to move it ahead of *(.rodata.*) ?
>>>>>>>> Is below change correct?
>>>>>>>>
>>>>>>>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>>>>>>>> index 793d0e11450c..3817642135aa 100644
>>>>>>>> --- a/xen/include/xen/xen.lds.h
>>>>>>>> +++ b/xen/include/xen/xen.lds.h
>>>>>>>> @@ -188,7 +188,7 @@
>>>>>>>>  #define VPCI_ARRAY               \
>>>>>>>>         . = ALIGN(POINTER_ALIGN); \
>>>>>>>>         __start_vpci_array = .;   \
>>>>>>>> -       *(SORT(.data.vpci.*))     \
>>>>>>>> +       *(.rodata)             \
>>>>>>>
>>>>>>> ... this isn't - you'd move _all_ of .rodata into here, which definitely
>>>>>>> isn't what you want. What I understand Roger meant was a .rodata-like
>>>>>>> section, e.g. .rodata.vpci.* (much like it was .data.vpci.* before).
>>>>>>
>>>>>> Indeed, my suggestion was merely to use .rodata instead of .data, as
>>>>>> that's more accurate IMO.  I think it should be *(.rodata.vpci) (and
>>>>>> same section change for the __used_section() attribute.
>>>>>
>>>>> If I understand correctly, the next version will be:
>>>>>
>>>>> +    static const vpci_capability_t *const finit##_entry  \
>>>>> +        __used_section(".rodata.vpci") = &finit##_t
>>>>> +
>>>>>
>>>>> and
>>>>>
>>>>>  #define VPCI_ARRAY               \
>>>>>         . = ALIGN(POINTER_ALIGN); \
>>>>>         __start_vpci_array = .;   \
>>>>> -       *(SORT(.data.vpci.*))     \
>>>>> +       *(.rodata.vpci)           \
>>>>>         __end_vpci_array = .;
>>>>
>>>> Did you also move the instances of VPCI_ARRAY in the linker scripts so
>>>> it's before the catch-all *(.rodata.*)?
>>>>
>>>>>
>>>>> But, that encountered an warning when compiling.
>>>>> " {standard input}: Assembler messages:
>>>>> {standard input}:1160: Warning: setting incorrect section attributes for 
>>>>> .rodata.vpci
>>>>> {standard input}: Assembler messages:
>>>>> {standard input}:3034: Warning: setting incorrect section attributes for 
>>>>> .rodata.vpci
>>>>> {standard input}: Assembler messages:
>>>>> {standard input}:6686: Warning: setting incorrect section attributes for 
>>>>> .rodata.vpci "
>>>>
>>>> What are the attributes for .rodata.vpci in the object files?  You can
>>>> get those using objdump or readelf, for example:
>>>>
>>>> $ objdump -h xen/drivers/vpci/msi.o
>>>> [...]
>>>>  17 .data.vpci.9  00000008  0000000000000000  0000000000000000  00000a50  
>>>> 2**3
>>>>                   CONTENTS, ALLOC, LOAD, RELOC, DATA
>>>>
>>>> It should be READONLY, otherwise you will get those messages.
>>>>
>>>>> And, during booting Xen, all value of __start_vpci_array is incorrect.
>>>>> Do I miss anything?
>>>>
>>>> I think that's likely because you haven't moved the instance of
>>>> VPCI_ARRAY in the linker script?
>>> Oh, right. Sorry, I forgot to move it.
>>> After changing this, it works now.
>>>
>>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>>> index bf956b6c5fc0..c88fd62f4f0d 100644
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -134,6 +134,7 @@ SECTIONS
>>>         BUGFRAMES
>>>
>>>         *(.rodata)
>>> +       VPCI_ARRAY
>>>         *(.rodata.*)
>>>         *(.data.rel.ro)
>>>         *(.data.rel.ro.*)
>>> @@ -148,7 +149,6 @@ SECTIONS
>>>         *(.note.gnu.build-id)
>>>         __note_gnu_build_id_end = .;
>>>  #endif
>>> -       VPCI_ARRAY
>>>    } PHDR(text)
>>
>> FWIW, I would put it ahead of *(.rodata).  Remember to also modify the
>> linker script for all the other arches, not just x86.
> 
> Whether before *(.rodata.*) or before *(.rodata), there still is the warning 
> " Warning: setting incorrect section attributes for .rodata.vpci "
> And the objdump shows "rodata.vpci" has no "readonly" word.

Did you check what gcc emits? It may be requesting "aw" instead of the
wanted "a" in the section attributes. Since there are relocations here,
".rodata." may not be the correct prefix to use; it may instead need to
be ".data.rel.ro.".

Jan