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

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

Hi Roger,

On 2025/6/10 15:08, Jan Beulich wrote:
> 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.".

What's your opinion about changing to ".data.rel.ro.vpci" ?

> 
> Jan

-- 
Best regards,
Jiqian Chen.