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

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

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)

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.