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

["Roger Pau Monné" <roger.pau@citrix.com>]

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?

Thanks, Roger.