Refactor REGISTER_VPCI_INIT to contain more capability specific
information, this is benefit for follow-on changes to hide capability
when initialization fails.
What's more, change the definition of init_header() since it is
not a capability and it is needed for all devices' PCI config space.
After refactor, the "priority" of initializing capabilities isn't
needed anymore, so delete its related codes.
Note:
Call vpci_make_msix_hole() in the end of init_msix() since the change
of sequence of init_header() and init_msix().
The cleanup hook is also added in this change, even if it's still
unused. Further changes will make use of it.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Andrew Cooper <andrew.cooper3@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
cc: Michal Orzel <michal.orzel@amd.com>
cc: Jan Beulich <jbeulich@suse.com>
cc: Julien Grall <julien@xen.org>
cc: Stefano Stabellini <sstabellini@kernel.org>
---
v4->v5 changes:
* Rename REGISTER_VPCI_CAP to REGISTER_PCI_CAPABILITY, rename REGISTER_VPCI_LEGACY_CAP to REGISTER_VPCI_CAP, rename REGISTER_VPCI_EXTENDED_CAP to REGISTER_VPCI_EXTCAP.
* Change cleanup hook of vpci_capability_t from void to int.
v3->v4 changes
* Delete the useless trailing dot of section ".data.vpci".
* Add description about priority since this patch removes the initializing priority of capabilities and priority is not needed anymore.
* Change the hook name from fini to cleanup.
* Change the name x and y to be finit and fclean.
* Remove the unnecessary check "!capability->init"
v2->v3 changes:
* This is separated from patch "vpci: Hide capability when it fails to initialize" of v2.
* Delete __maybe_unused attribute of "out" in function vpci_assign_devic().
* Rename REGISTER_VPCI_EXTEND_CAP to REGISTER_VPCI_EXTENDED_CAP.
v1->v2 changes:
* Removed the "priorities" of initializing capabilities since it isn't used anymore.
* Added new function vpci_capability_mask() and vpci_ext_capability_mask() to remove failed capability from list.
* Called vpci_make_msix_hole() in the end of init_msix().
Best regards,
Jiqian Chen.
---
xen/drivers/vpci/header.c | 3 +--
xen/drivers/vpci/msi.c | 2 +-
xen/drivers/vpci/msix.c | 8 +++++--
xen/drivers/vpci/rebar.c | 2 +-
xen/drivers/vpci/vpci.c | 46 ++++++++++++++++++++++++++++++---------
xen/include/xen/vpci.h | 30 ++++++++++++++++++-------
xen/include/xen/xen.lds.h | 2 +-
7 files changed, 68 insertions(+), 25 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4b2f761c9c24..9fa1cda23151 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -872,7 +872,7 @@ static int vpci_init_ext_capability_list(struct pci_dev *pdev)
return 0;
}
-static int cf_check init_header(struct pci_dev *pdev)
+int vpci_init_header(struct pci_dev *pdev)
{
uint16_t cmd;
uint64_t addr, size;
@@ -1068,7 +1068,6 @@ static int cf_check init_header(struct pci_dev *pdev)
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
return rc;
}
-REGISTER_VPCI_INIT(init_header, VPCI_PRIORITY_MIDDLE);
/*
* Local variables:
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 66e5a8a116be..2d45c7867de7 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -270,7 +270,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
return 0;
}
-REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
+REGISTER_VPCI_CAP(PCI_CAP_ID_MSI, init_msi, NULL);
void vpci_dump_msi(void)
{
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 74211301ba10..674815ead025 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -703,9 +703,13 @@ static int cf_check init_msix(struct pci_dev *pdev)
pdev->vpci->msix = msix;
list_add(&msix->next, &d->arch.hvm.msix_tables);
- return 0;
+ spin_lock(&pdev->vpci->lock);
+ rc = vpci_make_msix_hole(pdev);
+ spin_unlock(&pdev->vpci->lock);
+
+ return rc;
}
-REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH);
+REGISTER_VPCI_CAP(PCI_CAP_ID_MSIX, init_msix, NULL);
/*
* Local variables:
diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
index 793937449af7..9cafd80ca2c9 100644
--- a/xen/drivers/vpci/rebar.c
+++ b/xen/drivers/vpci/rebar.c
@@ -118,7 +118,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
return 0;
}
-REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
+REGISTER_VPCI_EXTCAP(PCI_EXT_CAP_ID_REBAR, init_rebar, NULL);
/*
* Local variables:
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 8474c0e3b995..2861557e06d2 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -36,8 +36,8 @@ struct vpci_register {
};
#ifdef __XEN__
-extern vpci_register_init_t *const __start_vpci_array[];
-extern vpci_register_init_t *const __end_vpci_array[];
+extern vpci_capability_t *const __start_vpci_array[];
+extern vpci_capability_t *const __end_vpci_array[];
#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
@@ -83,6 +83,34 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
+static int vpci_init_capabilities(struct pci_dev *pdev)
+{
+ for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
+ {
+ const vpci_capability_t *capability = __start_vpci_array[i];
+ const unsigned int cap = capability->id;
+ const bool is_ext = capability->is_ext;
+ unsigned int pos;
+ int rc;
+
+ if ( !is_ext )
+ pos = pci_find_cap_offset(pdev->sbdf, cap);
+ else if ( !is_hardware_domain(pdev->domain) )
+ continue;
+ else
+ pos = pci_find_ext_capability(pdev->sbdf, cap);
+
+ if ( !pos )
+ continue;
+
+ rc = capability->init(pdev);
+ if ( rc )
+ return rc;
+ }
+
+ return 0;
+}
+
void vpci_deassign_device(struct pci_dev *pdev)
{
unsigned int i;
@@ -128,7 +156,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
int vpci_assign_device(struct pci_dev *pdev)
{
- unsigned int i;
const unsigned long *ro_map;
int rc = 0;
@@ -159,14 +186,13 @@ int vpci_assign_device(struct pci_dev *pdev)
goto out;
#endif
- for ( i = 0; i < NUM_VPCI_INIT; i++ )
- {
- rc = __start_vpci_array[i](pdev);
- if ( rc )
- break;
- }
+ rc = vpci_init_header(pdev);
+ if ( rc )
+ goto out;
+
+ rc = vpci_init_capabilities(pdev);
- out: __maybe_unused;
+ out:
if ( rc )
vpci_deassign_device(pdev);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 61d16cc8b897..e5e3f23ca39c 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev *pdev, unsigned int reg,
typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data);
-typedef int vpci_register_init_t(struct pci_dev *dev);
-
-#define VPCI_PRIORITY_HIGH "1"
-#define VPCI_PRIORITY_MIDDLE "5"
-#define VPCI_PRIORITY_LOW "9"
+typedef struct {
+ unsigned int id;
+ bool is_ext;
+ int (*init)(struct pci_dev *pdev);
+ int (*cleanup)(struct pci_dev *pdev);
+} vpci_capability_t;
#define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12)
@@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
*/
#define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
-#define REGISTER_VPCI_INIT(x, p) \
- static vpci_register_init_t *const x##_entry \
- __used_section(".data.vpci." p) = (x)
+#define REGISTER_PCI_CAPABILITY(cap, finit, fclean, ext) \
+ static vpci_capability_t finit##_t = { \
+ .id = (cap), \
+ .init = (finit), \
+ .cleanup = (fclean), \
+ .is_ext = (ext), \
+ }; \
+ static vpci_capability_t *const finit##_entry \
+ __used_section(".data.vpci") = &finit##_t
+
+#define REGISTER_VPCI_CAP(cap, finit, fclean) \
+ REGISTER_PCI_CAPABILITY(cap, finit, fclean, false)
+#define REGISTER_VPCI_EXTCAP(cap, finit, fclean) \
+ REGISTER_PCI_CAPABILITY(cap, finit, fclean, true)
+
+int __must_check vpci_init_header(struct pci_dev *pdev);
/* Assign vPCI to device by adding handlers. */
int __must_check vpci_assign_device(struct pci_dev *pdev);
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 793d0e11450c..84ec506b00da 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.*)) \
+ *(.data.vpci) \
__end_vpci_array = .;
#else
#define VPCI_ARRAY
--
2.34.1
On Mon, May 26, 2025 at 05:45:53PM +0800, Jiqian Chen wrote:
> Refactor REGISTER_VPCI_INIT to contain more capability specific
> information, this is benefit for follow-on changes to hide capability
"this will benefit further follow-on..."
I think it's clearer.
> when initialization fails.
>
> What's more, change the definition of init_header() since it is
> not a capability and it is needed for all devices' PCI config space.
>
> After refactor, the "priority" of initializing capabilities isn't
> needed anymore, so delete its related codes.
>
> Note:
> Call vpci_make_msix_hole() in the end of init_msix() since the change
> of sequence of init_header() and init_msix().
>
> The cleanup hook is also added in this change, even if it's still
> unused. Further changes will make use of it.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> cc: Andrew Cooper <andrew.cooper3@citrix.com>
> cc: Anthony PERARD <anthony.perard@vates.tech>
> cc: Michal Orzel <michal.orzel@amd.com>
> cc: Jan Beulich <jbeulich@suse.com>
> cc: Julien Grall <julien@xen.org>
> cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v4->v5 changes:
> * Rename REGISTER_VPCI_CAP to REGISTER_PCI_CAPABILITY, rename REGISTER_VPCI_LEGACY_CAP to REGISTER_VPCI_CAP, rename REGISTER_VPCI_EXTENDED_CAP to REGISTER_VPCI_EXTCAP.
> * Change cleanup hook of vpci_capability_t from void to int.
>
> v3->v4 changes
> * Delete the useless trailing dot of section ".data.vpci".
> * Add description about priority since this patch removes the initializing priority of capabilities and priority is not needed anymore.
> * Change the hook name from fini to cleanup.
> * Change the name x and y to be finit and fclean.
> * Remove the unnecessary check "!capability->init"
>
> v2->v3 changes:
> * This is separated from patch "vpci: Hide capability when it fails to initialize" of v2.
> * Delete __maybe_unused attribute of "out" in function vpci_assign_devic().
> * Rename REGISTER_VPCI_EXTEND_CAP to REGISTER_VPCI_EXTENDED_CAP.
>
> v1->v2 changes:
> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to remove failed capability from list.
> * Called vpci_make_msix_hole() in the end of init_msix().
>
> Best regards,
> Jiqian Chen.
> ---
> xen/drivers/vpci/header.c | 3 +--
> xen/drivers/vpci/msi.c | 2 +-
> xen/drivers/vpci/msix.c | 8 +++++--
> xen/drivers/vpci/rebar.c | 2 +-
> xen/drivers/vpci/vpci.c | 46 ++++++++++++++++++++++++++++++---------
> xen/include/xen/vpci.h | 30 ++++++++++++++++++-------
> xen/include/xen/xen.lds.h | 2 +-
> 7 files changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 4b2f761c9c24..9fa1cda23151 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -872,7 +872,7 @@ static int vpci_init_ext_capability_list(struct pci_dev *pdev)
> return 0;
> }
>
> -static int cf_check init_header(struct pci_dev *pdev)
> +int vpci_init_header(struct pci_dev *pdev)
> {
> uint16_t cmd;
> uint64_t addr, size;
> @@ -1068,7 +1068,6 @@ static int cf_check init_header(struct pci_dev *pdev)
> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> return rc;
> }
> -REGISTER_VPCI_INIT(init_header, VPCI_PRIORITY_MIDDLE);
>
> /*
> * Local variables:
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index 66e5a8a116be..2d45c7867de7 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -270,7 +270,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
>
> return 0;
> }
> -REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
> +REGISTER_VPCI_CAP(PCI_CAP_ID_MSI, init_msi, NULL);
>
> void vpci_dump_msi(void)
> {
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 74211301ba10..674815ead025 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -703,9 +703,13 @@ static int cf_check init_msix(struct pci_dev *pdev)
> pdev->vpci->msix = msix;
> list_add(&msix->next, &d->arch.hvm.msix_tables);
>
> - return 0;
> + spin_lock(&pdev->vpci->lock);
> + rc = vpci_make_msix_hole(pdev);
> + spin_unlock(&pdev->vpci->lock);
> +
> + return rc;
> }
> -REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH);
> +REGISTER_VPCI_CAP(PCI_CAP_ID_MSIX, init_msix, NULL);
>
> /*
> * Local variables:
> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> index 793937449af7..9cafd80ca2c9 100644
> --- a/xen/drivers/vpci/rebar.c
> +++ b/xen/drivers/vpci/rebar.c
> @@ -118,7 +118,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>
> return 0;
> }
> -REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
> +REGISTER_VPCI_EXTCAP(PCI_EXT_CAP_ID_REBAR, init_rebar, NULL);
>
> /*
> * Local variables:
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 8474c0e3b995..2861557e06d2 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -36,8 +36,8 @@ struct vpci_register {
> };
>
> #ifdef __XEN__
> -extern vpci_register_init_t *const __start_vpci_array[];
> -extern vpci_register_init_t *const __end_vpci_array[];
> +extern vpci_capability_t *const __start_vpci_array[];
> +extern vpci_capability_t *const __end_vpci_array[];
> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>
> #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> @@ -83,6 +83,34 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
>
> #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>
> +static int vpci_init_capabilities(struct pci_dev *pdev)
> +{
> + for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
> + {
> + const vpci_capability_t *capability = __start_vpci_array[i];
> + const unsigned int cap = capability->id;
> + const bool is_ext = capability->is_ext;
> + unsigned int pos;
> + int rc;
> +
> + if ( !is_ext )
> + pos = pci_find_cap_offset(pdev->sbdf, cap);
> + else if ( !is_hardware_domain(pdev->domain) )
> + continue;
> + else
> + pos = pci_find_ext_capability(pdev->sbdf, cap);
Nit: it's more compact if you do something like:
unsigned int pos = 0;
if ( !is_ext )
pos = pci_find_cap_offset(pdev->sbdf, cap);
else if ( is_hardware_domain(pdev->domain) )
pos = pci_find_ext_capability(pdev->sbdf, cap);
if ( !pos )
continue;
> +
> + if ( !pos )
> + continue;
> +
> + rc = capability->init(pdev);
> + if ( rc )
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> void vpci_deassign_device(struct pci_dev *pdev)
> {
> unsigned int i;
> @@ -128,7 +156,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
>
> int vpci_assign_device(struct pci_dev *pdev)
> {
> - unsigned int i;
> const unsigned long *ro_map;
> int rc = 0;
>
> @@ -159,14 +186,13 @@ int vpci_assign_device(struct pci_dev *pdev)
> goto out;
> #endif
>
> - for ( i = 0; i < NUM_VPCI_INIT; i++ )
> - {
> - rc = __start_vpci_array[i](pdev);
> - if ( rc )
> - break;
> - }
> + rc = vpci_init_header(pdev);
> + if ( rc )
> + goto out;
> +
> + rc = vpci_init_capabilities(pdev);
>
> - out: __maybe_unused;
> + out:
> if ( rc )
> vpci_deassign_device(pdev);
>
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 61d16cc8b897..e5e3f23ca39c 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev *pdev, unsigned int reg,
> typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
> uint32_t val, void *data);
>
> -typedef int vpci_register_init_t(struct pci_dev *dev);
> -
> -#define VPCI_PRIORITY_HIGH "1"
> -#define VPCI_PRIORITY_MIDDLE "5"
> -#define VPCI_PRIORITY_LOW "9"
> +typedef struct {
> + unsigned int id;
> + bool is_ext;
> + int (*init)(struct pci_dev *pdev);
> + int (*cleanup)(struct pci_dev *pdev);
> +} vpci_capability_t;
>
> #define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12)
>
> @@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
> */
> #define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
>
> -#define REGISTER_VPCI_INIT(x, p) \
> - static vpci_register_init_t *const x##_entry \
> - __used_section(".data.vpci." p) = (x)
> +#define REGISTER_PCI_CAPABILITY(cap, finit, fclean, ext) \
REGISTER_VPCI_CAPABILITY() if possible (note the added V).
> + static vpci_capability_t finit##_t = { \
static const?
> + .id = (cap), \
> + .init = (finit), \
> + .cleanup = (fclean), \
> + .is_ext = (ext), \
Indent should be 4 spaces here I think.
> + }; \
> + static vpci_capability_t *const finit##_entry \
> + __used_section(".data.vpci") = &finit##_t
IMO this should better use .rodata instead of .data. 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.
> +
> +#define REGISTER_VPCI_CAP(cap, finit, fclean) \
> + REGISTER_PCI_CAPABILITY(cap, finit, fclean, false)
> +#define REGISTER_VPCI_EXTCAP(cap, finit, fclean) \
> + REGISTER_PCI_CAPABILITY(cap, finit, fclean, true)
Since you are modifying those, can use 4 spaces as indentation?
There's no need to have such padding.
Thanks, Roger.
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
> 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) \
__end_vpci_array = .;
>
>> +
>> +#define REGISTER_VPCI_CAP(cap, finit, fclean) \
>> + REGISTER_PCI_CAPABILITY(cap, finit, fclean, false)
>> +#define REGISTER_VPCI_EXTCAP(cap, finit, fclean) \
>> + REGISTER_PCI_CAPABILITY(cap, finit, fclean, true)
>
> Since you are modifying those, can use 4 spaces as indentation?
> There's no need to have such padding.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
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).
Jan
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.
Thanks, Roger.
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 = .;
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 "
And, during booting Xen, all value of __start_vpci_array is incorrect.
Do I miss anything?
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
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.
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.
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.
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.
But starting Xen dom0 is OK.
I attached the new this patch and the result of "objdump -h xen/drivers/vpci/msi.o"
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
./xen/drivers/vpci/msi.o: file format elf64-x86-64
Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00000000 0000000000000000 0000000000000000 00000040 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
1 .data 00000000 0000000000000000 0000000000000000 00000040 2**0
CONTENTS, ALLOC, LOAD, DATA
2 .bss 00000000 0000000000000000 0000000000000000 00000040 2**0
ALLOC
3 .text._can_read_lock 00000052 0000000000000000 0000000000000000 00000040 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
4 .text.address_read 0000000b 0000000000000000 0000000000000000 00000092 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
5 .text.address_hi_read 00000010 0000000000000000 0000000000000000 0000009d 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
6 .text.data_read 0000000d 0000000000000000 0000000000000000 000000ad 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
7 .text.mask_read 0000000c 0000000000000000 0000000000000000 000000ba 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
8 .text.mask_write 00000078 0000000000000000 0000000000000000 000000c6 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
9 .text.address_hi_write 00000027 0000000000000000 0000000000000000 0000013e 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
10 .text.control_read 0000004e 0000000000000000 0000000000000000 00000165 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
11 .text.control_write 00000144 0000000000000000 0000000000000000 000001b3 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
12 .altinstructions 00000070 0000000000000000 0000000000000000 000002f7 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
13 .discard 00000010 0000000000000000 0000000000000000 00000367 2**0
CONTENTS, ALLOC, LOAD, READONLY, DATA
14 .altinstr_replacement 00000000 0000000000000000 0000000000000000 00000377 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
15 .text._read_trylock 00000090 0000000000000000 0000000000000000 00000377 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
16 .text._read_unlock 00000036 0000000000000000 0000000000000000 00000407 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
17 .text.cleanup_msi 000000dc 0000000000000000 0000000000000000 0000043d 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
18 .rodata.init_msi.str1.1 0000002a 0000000000000000 0000000000000000 00000519 2**0
CONTENTS, ALLOC, LOAD, READONLY, DATA
19 .text.init_msi 00000270 0000000000000000 0000000000000000 00000543 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
20 .bug_frames.3 00000020 0000000000000000 0000000000000000 000007b4 2**2
CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
21 .text.address_write 00000039 0000000000000000 0000000000000000 000007d4 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
22 .text.data_write 00000028 0000000000000000 0000000000000000 0000080d 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
23 .rodata.vpci_dump_msi.str1.1 0000009a 0000000000000000 0000000000000000 00000835 2**0
CONTENTS, ALLOC, LOAD, READONLY, DATA
24 .rodata.vpci_dump_msi.str1.8 0000007f 0000000000000000 0000000000000000 000008d0 2**3
CONTENTS, ALLOC, LOAD, READONLY, DATA
25 .text.vpci_dump_msi 000002af 0000000000000000 0000000000000000 0000094f 2**0
CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
26 .rodata.vpci 00000008 0000000000000000 0000000000000000 00000c00 2**3
CONTENTS, ALLOC, LOAD, RELOC, DATA
27 .data.rel.ro.local.init_msi_t 00000018 0000000000000000 0000000000000000 00000c10 2**4
CONTENTS, ALLOC, LOAD, RELOC, DATA
28 .debug_info 0000ae83 0000000000000000 0000000000000000 00000c28 2**0
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
29 .debug_abbrev 00000892 0000000000000000 0000000000000000 0000baab 2**0
CONTENTS, READONLY, DEBUGGING, OCTETS
30 .debug_loclists 00000ffc 0000000000000000 0000000000000000 0000c33d 2**0
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
31 .debug_aranges 00000120 0000000000000000 0000000000000000 0000d339 2**0
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
32 .debug_rnglists 00000212 0000000000000000 0000000000000000 0000d459 2**0
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
33 .debug_line 00000e30 0000000000000000 0000000000000000 0000d66b 2**0
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
34 .debug_str 0000502b 0000000000000000 0000000000000000 0000e49b 2**0
CONTENTS, READONLY, DEBUGGING, OCTETS
35 .debug_line_str 00000470 0000000000000000 0000000000000000 000134c6 2**0
CONTENTS, READONLY, DEBUGGING, OCTETS
36 .comment 0000002c 0000000000000000 0000000000000000 00013936 2**0
CONTENTS, READONLY
37 .note.GNU-stack 00000000 0000000000000000 0000000000000000 00013962 2**0
CONTENTS, READONLY
38 .note.gnu.property 00000020 0000000000000000 0000000000000000 00013968 2**3
CONTENTS, ALLOC, LOAD, READONLY, DATA
39 .debug_frame 000002d0 0000000000000000 0000000000000000 00013988 2**3
CONTENTS, RELOC, READONLY, DEBUGGING, OCTETS
From ff2d43c1b61e9fc1bbba0a4028ab278aa1c21a7e Mon Sep 17 00:00:00 2001
From: Jiqian Chen <Jiqian.Chen@amd.com>
Date: Wed, 16 Apr 2025 16:38:07 +0800
Subject: [PATCH 3/9] vpci: Refactor REGISTER_VPCI_INIT
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Refactor REGISTER_VPCI_INIT to contain more capability specific
information, this will benefit further follow-on changes to hide
capability when initialization fails.
What's more, change the definition of init_header() since it is
not a capability and it is needed for all devices' PCI config space.
After refactor, the "priority" of initializing capabilities isn't
needed anymore, so delete its related codes.
Note:
Call vpci_make_msix_hole() in the end of init_msix() since the change
of sequence of init_header() and init_msix().
The cleanup hook is also added in this change, even if it's still
unused. Further changes will make use of it.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Andrew Cooper <andrew.cooper3@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
cc: Michal Orzel <michal.orzel@amd.com>
cc: Jan Beulich <jbeulich@suse.com>
cc: Julien Grall <julien@xen.org>
cc: Stefano Stabellini <sstabellini@kernel.org>
---
v5->v6 changes:
* Rename REGISTER_PCI_CAPABILITY to REGISTER_VPCI_CAPABILITY.
* Move vpci_capability_t entry from ".data.vpci" to ".rodata.vpci" and
move the instances of VPCI_ARRAY in the linker scripts before *(.rodata).
* Change _start/end_vpci_array[] to be const pointer array.
v4->v5 changes:
* Rename REGISTER_VPCI_CAP to REGISTER_PCI_CAPABILITY, rename REGISTER_VPCI_LEGACY_CAP to
REGISTER_VPCI_CAP, rename REGISTER_VPCI_EXTENDED_CAP to REGISTER_VPCI_EXTCAP.
* Change cleanup hook of vpci_capability_t from void to int.
v3->v4 changes
* Delete the useless trailing dot of section ".data.vpci".
* Add description about priority since this patch removes the initializing priority of
capabilities and priority is not needed anymore.
* Change the hook name from fini to cleanup.
* Change the name x and y to be finit and fclean.
* Remove the unnecessary check "!capability->init"
v2->v3 changes:
* This is separated from patch "vpci: Hide capability when it fails to initialize" of v2.
* Delete __maybe_unused attribute of "out" in function vpci_assign_devic().
* Rename REGISTER_VPCI_EXTEND_CAP to REGISTER_VPCI_EXTENDED_CAP.
v1->v2 changes:
* Removed the "priorities" of initializing capabilities since it isn't used anymore.
* Added new function vpci_capability_mask() and vpci_ext_capability_mask() to remove
failed capability from list.
* Called vpci_make_msix_hole() in the end of init_msix().
Best regards,
Jiqian Chen.
---
xen/arch/arm/xen.lds.S | 3 +--
xen/arch/ppc/xen.lds.S | 3 +--
xen/arch/riscv/xen.lds.S | 3 +--
xen/arch/x86/xen.lds.S | 2 +-
xen/drivers/vpci/header.c | 3 +--
xen/drivers/vpci/msi.c | 2 +-
xen/drivers/vpci/msix.c | 8 +++++--
xen/drivers/vpci/rebar.c | 2 +-
xen/drivers/vpci/vpci.c | 44 ++++++++++++++++++++++++++++++---------
xen/include/xen/vpci.h | 30 +++++++++++++++++++-------
xen/include/xen/xen.lds.h | 2 +-
11 files changed, 70 insertions(+), 32 deletions(-)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 5bfbe1e92c1e..7d9508ed0ff5 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -55,6 +55,7 @@ SECTIONS
BUGFRAMES
+ VPCI_ARRAY
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
@@ -64,8 +65,6 @@ SECTIONS
__proc_info_start = .;
*(.proc.info)
__proc_info_end = .;
-
- VPCI_ARRAY
} :text
#if defined(BUILD_ID)
diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
index 1366e2819eed..0295c921f735 100644
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -49,13 +49,12 @@ SECTIONS
BUGFRAMES
+ VPCI_ARRAY
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
*(.data.rel.ro.*)
- VPCI_ARRAY
-
. = ALIGN(POINTER_ALIGN);
} :text
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 8c3c06de01f6..6df4cb92a6b2 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -44,13 +44,12 @@ SECTIONS
BUGFRAMES
+ VPCI_ARRAY
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
*(.data.rel.ro.*)
- VPCI_ARRAY
-
. = ALIGN(POINTER_ALIGN);
} :text
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index bf956b6c5fc0..ba035d6a673f 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -133,6 +133,7 @@ SECTIONS
BUGFRAMES
+ VPCI_ARRAY
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
@@ -148,7 +149,6 @@ SECTIONS
*(.note.gnu.build-id)
__note_gnu_build_id_end = .;
#endif
- VPCI_ARRAY
} PHDR(text)
#if defined(CONFIG_PVH_GUEST) && !defined(EFI)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index a39bf2b12585..2cf54ce60297 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -872,7 +872,7 @@ static int vpci_init_ext_capability_list(struct pci_dev *pdev)
return 0;
}
-static int cf_check init_header(struct pci_dev *pdev)
+int vpci_init_header(struct pci_dev *pdev)
{
uint16_t cmd;
uint64_t addr, size;
@@ -1068,7 +1068,6 @@ static int cf_check init_header(struct pci_dev *pdev)
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
return rc;
}
-REGISTER_VPCI_INIT(init_header, VPCI_PRIORITY_MIDDLE);
/*
* Local variables:
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 66e5a8a116be..2d45c7867de7 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -270,7 +270,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
return 0;
}
-REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
+REGISTER_VPCI_CAP(PCI_CAP_ID_MSI, init_msi, NULL);
void vpci_dump_msi(void)
{
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 74211301ba10..674815ead025 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -703,9 +703,13 @@ static int cf_check init_msix(struct pci_dev *pdev)
pdev->vpci->msix = msix;
list_add(&msix->next, &d->arch.hvm.msix_tables);
- return 0;
+ spin_lock(&pdev->vpci->lock);
+ rc = vpci_make_msix_hole(pdev);
+ spin_unlock(&pdev->vpci->lock);
+
+ return rc;
}
-REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH);
+REGISTER_VPCI_CAP(PCI_CAP_ID_MSIX, init_msix, NULL);
/*
* Local variables:
diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
index 793937449af7..9cafd80ca2c9 100644
--- a/xen/drivers/vpci/rebar.c
+++ b/xen/drivers/vpci/rebar.c
@@ -118,7 +118,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
return 0;
}
-REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
+REGISTER_VPCI_EXTCAP(PCI_EXT_CAP_ID_REBAR, init_rebar, NULL);
/*
* Local variables:
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 8474c0e3b995..6e768bb32447 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -36,8 +36,8 @@ struct vpci_register {
};
#ifdef __XEN__
-extern vpci_register_init_t *const __start_vpci_array[];
-extern vpci_register_init_t *const __end_vpci_array[];
+extern const vpci_capability_t *const __start_vpci_array[];
+extern const vpci_capability_t *const __end_vpci_array[];
#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
@@ -83,6 +83,32 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
+static int vpci_init_capabilities(struct pci_dev *pdev)
+{
+ for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
+ {
+ const vpci_capability_t *capability = __start_vpci_array[i];
+ const unsigned int cap = capability->id;
+ const bool is_ext = capability->is_ext;
+ int rc;
+ unsigned int pos = 0;
+
+ if ( !is_ext )
+ pos = pci_find_cap_offset(pdev->sbdf, cap);
+ else if ( is_hardware_domain(pdev->domain) )
+ pos = pci_find_ext_capability(pdev->sbdf, cap);
+
+ if ( !pos )
+ continue;
+
+ rc = capability->init(pdev);
+ if ( rc )
+ return rc;
+ }
+
+ return 0;
+}
+
void vpci_deassign_device(struct pci_dev *pdev)
{
unsigned int i;
@@ -128,7 +154,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
int vpci_assign_device(struct pci_dev *pdev)
{
- unsigned int i;
const unsigned long *ro_map;
int rc = 0;
@@ -159,14 +184,13 @@ int vpci_assign_device(struct pci_dev *pdev)
goto out;
#endif
- for ( i = 0; i < NUM_VPCI_INIT; i++ )
- {
- rc = __start_vpci_array[i](pdev);
- if ( rc )
- break;
- }
+ rc = vpci_init_header(pdev);
+ if ( rc )
+ goto out;
+
+ rc = vpci_init_capabilities(pdev);
- out: __maybe_unused;
+ out:
if ( rc )
vpci_deassign_device(pdev);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 61d16cc8b897..6ea6fcf578f1 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev *pdev, unsigned int reg,
typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data);
-typedef int vpci_register_init_t(struct pci_dev *dev);
-
-#define VPCI_PRIORITY_HIGH "1"
-#define VPCI_PRIORITY_MIDDLE "5"
-#define VPCI_PRIORITY_LOW "9"
+typedef struct {
+ unsigned int id;
+ bool is_ext;
+ int (*init)(struct pci_dev *pdev);
+ int (*cleanup)(struct pci_dev *pdev);
+} vpci_capability_t;
#define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12)
@@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
*/
#define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
-#define REGISTER_VPCI_INIT(x, p) \
- static vpci_register_init_t *const x##_entry \
- __used_section(".data.vpci." p) = (x)
+#define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
+ static const vpci_capability_t finit##_t = { \
+ .id = (cap), \
+ .init = (finit), \
+ .cleanup = (fclean), \
+ .is_ext = (ext), \
+ }; \
+ static const vpci_capability_t *const finit##_entry \
+ __used_section(".rodata.vpci") = &finit##_t
+
+#define REGISTER_VPCI_CAP(cap, finit, fclean) \
+ REGISTER_VPCI_CAPABILITY(cap, finit, fclean, false)
+#define REGISTER_VPCI_EXTCAP(cap, finit, fclean) \
+ REGISTER_VPCI_CAPABILITY(cap, finit, fclean, true)
+
+int __must_check vpci_init_header(struct pci_dev *pdev);
/* Assign vPCI to device by adding handlers. */
int __must_check vpci_assign_device(struct pci_dev *pdev);
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 793d0e11450c..ac359c3757a2 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.vpci) \
__end_vpci_array = .;
#else
#define VPCI_ARRAY
--
2.34.1
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.".
Jan
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.
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.
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?
I just saw above warning.
> 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.".
You may right.
From "objdump -r xen/drivers/vpci/msi.o"
I can get
RELOCATION RECORDS FOR [.rodata.vpci]:
OFFSET TYPE VALUE
0000000000000000 R_X86_64_64 .data.rel.ro.local.init_msi_t
RELOCATION RECORDS FOR [.data.rel.ro.local.init_msi_t]:
OFFSET TYPE VALUE
0000000000000008 R_X86_64_64 .text.init_msi
0000000000000010 R_X86_64_64 .text.cleanup_msi
And from "objdump -D xen/drivers/vpci/msi.o"
I can get
Disassembly of section .rodata.vpci:
0000000000000000 <init_msi_entry>:
...
Disassembly of section .data.rel.ro.local.init_msi_t:
0000000000000000 <init_msi_t>:
0: 05 00 00 00 00 add $0x0,%eax
...
>
> Jan
--
Best regards,
Jiqian Chen.
On 05.06.2025 14:50, Roger Pau Monné wrote:
> On Mon, May 26, 2025 at 05:45:53PM +0800, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -36,8 +36,8 @@ struct vpci_register {
>> };
>>
>> #ifdef __XEN__
>> -extern vpci_register_init_t *const __start_vpci_array[];
>> -extern vpci_register_init_t *const __end_vpci_array[];
>> +extern vpci_capability_t *const __start_vpci_array[];
>> +extern vpci_capability_t *const __end_vpci_array[];
These also want to gain another const, to match ...
>> @@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>> */
>> #define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
>>
>> -#define REGISTER_VPCI_INIT(x, p) \
>> - static vpci_register_init_t *const x##_entry \
>> - __used_section(".data.vpci." p) = (x)
>> +#define REGISTER_PCI_CAPABILITY(cap, finit, fclean, ext) \
>
> REGISTER_VPCI_CAPABILITY() if possible (note the added V).
>
>> + static vpci_capability_t finit##_t = { \
>
> static const?
... this.
Jan
© 2016 - 2026 Red Hat, Inc.