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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 11, 2025 at 10:19:34AM +0000, Chen, Jiqian wrote:
> 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" ?

Yeah, it's longer but it's the correct section prefix to use.

Thanks, Roger.