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.
Thanks, Roger.
