| Message ID | 20250327073214.158210-1-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] vpci: Hide capability when it fails to initialize | expand |
On Thu, Mar 27, 2025 at 03:32:12PM +0800, Jiqian Chen wrote: > When vpci fails to initialize a capability of a device, it just > return error instead of catching and processing exception. That > makes the entire device unusable. > > So, refactor REGISTER_VPCI_INIT to contain more capability specific > information, and try to hide capability when initialization fails > in vpci_assign_device(). > > 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. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > Hi all, > > This patch aims to hide a capability when its initialization fails. > That causes we can't rely on vpci_deassign_device() to clean up assigned > resources, so, following two patches clean up resources in the failure > path of init function. > > Best regards, > Jiqian Chen. > --- > xen/drivers/vpci/header.c | 3 +- > xen/drivers/vpci/msi.c | 2 +- > xen/drivers/vpci/msix.c | 2 +- > xen/drivers/vpci/rebar.c | 2 +- > xen/drivers/vpci/vpci.c | 65 +++++++++++++++++++++++++++++++++------ > xen/include/xen/vpci.h | 27 ++++++++++++---- > 6 files changed, 81 insertions(+), 20 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index ef6c965c081c..8c8e4ac5698a 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -745,7 +745,7 @@ static int bar_add_rangeset(const struct pci_dev *pdev, struct vpci_bar *bar, > return !bar->mem ? -ENOMEM : 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; > @@ -1007,7 +1007,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..9d7a9fd8dba1 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_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi, VPCI_PRIORITY_LOW); > > void vpci_dump_msi(void) > { > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c > index 6bd8c55bb48e..50e5f38c1e09 100644 > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -753,7 +753,7 @@ static int cf_check init_msix(struct pci_dev *pdev) > > return 0; > } > -REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH); > +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, VPCI_PRIORITY_HIGH); > > /* > * Local variables: > diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c > index 793937449af7..7c53ee031887 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_EXTEND_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, VPCI_PRIORITY_LOW); > > /* > * Local variables: > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 1e6aa5d799b9..a8362e46e097 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,47 @@ static int assign_virtual_sbdf(struct pci_dev *pdev) > > #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ > > +static int vpci_init_cap_with_priority(struct pci_dev *pdev, > + const char *priority) > +{ > + for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ ) > + { > + const vpci_capability_t *capability = __start_vpci_array[i]; > + const unsigned int cap_id = capability->id; > + unsigned int pos; > + int rc; > + > + if ( *(capability->priority) != *priority ) > + continue; > + > + if ( !capability->is_ext ) > + pos = pci_find_cap_offset(pdev->sbdf, cap_id); > + else > + pos = pci_find_ext_capability(pdev->sbdf, cap_id); > + > + if ( !pos ) > + continue; > + > + rc = capability->init(pdev); > + > + if ( rc ) > + { > + printk(XENLOG_WARNING "%pd %pp: cap init fail rc=%d, try to hide\n", > + pdev->domain, &pdev->sbdf, rc); > + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, > + pos, capability->is_ext ? 4 : 1, NULL); Are you sure this works as intended? The capability ID 0 is marked as "reserved" in the spec, so it's unclear to me how OSes would handle finding such capability on the list - I won't be surprised if some implementations decide to terminate the walk. It's fine to mask the capability ID for the ones that we don't want to expose, but there's further work to do IMO. The usual way to deal with masking capabilities is to short circuit the capability from the linked list, by making the previous capability "Next Capability Offset" point to the next capability in the list, thus skipping the current one. So: capability[n - 1].next_cap = capability[n].next_cap IOW: you will need to add the handler to the previous capability on the list. That's how it's already done in init_header(). > + if ( rc ) > + { > + printk(XENLOG_ERR "%pd %pp: fail to hide cap rc=%d\n", > + pdev->domain, &pdev->sbdf, rc); > + return rc; > + } > + } > + } > + > + return 0; > +} > + > void vpci_deassign_device(struct pci_dev *pdev) > { > unsigned int i; > @@ -128,7 +169,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,12 +199,19 @@ 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; > - } > + /* > + * Capabilities with high priority like MSI-X need to > + * be initialized before header > + */ > + rc = vpci_init_cap_with_priority(pdev, VPCI_PRIORITY_HIGH); > + if ( rc ) > + goto out; I understand this is not introduced by this change, but I wonder if there could be a way to ditch the priority stuff for capabilities, specially now that we only have two "priorities": before or after PCI header initialization. Thanks, Roger.
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ef6c965c081c..8c8e4ac5698a 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -745,7 +745,7 @@ static int bar_add_rangeset(const struct pci_dev *pdev, struct vpci_bar *bar, return !bar->mem ? -ENOMEM : 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; @@ -1007,7 +1007,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..9d7a9fd8dba1 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_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi, VPCI_PRIORITY_LOW); void vpci_dump_msi(void) { diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index 6bd8c55bb48e..50e5f38c1e09 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -753,7 +753,7 @@ static int cf_check init_msix(struct pci_dev *pdev) return 0; } -REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH); +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, VPCI_PRIORITY_HIGH); /* * Local variables: diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c index 793937449af7..7c53ee031887 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_EXTEND_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, VPCI_PRIORITY_LOW); /* * Local variables: diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 1e6aa5d799b9..a8362e46e097 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,47 @@ static int assign_virtual_sbdf(struct pci_dev *pdev) #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ +static int vpci_init_cap_with_priority(struct pci_dev *pdev, + const char *priority) +{ + for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ ) + { + const vpci_capability_t *capability = __start_vpci_array[i]; + const unsigned int cap_id = capability->id; + unsigned int pos; + int rc; + + if ( *(capability->priority) != *priority ) + continue; + + if ( !capability->is_ext ) + pos = pci_find_cap_offset(pdev->sbdf, cap_id); + else + pos = pci_find_ext_capability(pdev->sbdf, cap_id); + + if ( !pos ) + continue; + + rc = capability->init(pdev); + + if ( rc ) + { + printk(XENLOG_WARNING "%pd %pp: cap init fail rc=%d, try to hide\n", + pdev->domain, &pdev->sbdf, rc); + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, + pos, capability->is_ext ? 4 : 1, NULL); + if ( rc ) + { + printk(XENLOG_ERR "%pd %pp: fail to hide cap rc=%d\n", + pdev->domain, &pdev->sbdf, rc); + return rc; + } + } + } + + return 0; +} + void vpci_deassign_device(struct pci_dev *pdev) { unsigned int i; @@ -128,7 +169,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,12 +199,19 @@ 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; - } + /* + * Capabilities with high priority like MSI-X need to + * be initialized before header + */ + rc = vpci_init_cap_with_priority(pdev, VPCI_PRIORITY_HIGH); + if ( rc ) + goto out; + + rc = vpci_init_header(pdev); + if ( rc ) + goto out; + + rc = vpci_init_cap_with_priority(pdev, VPCI_PRIORITY_LOW); out: __maybe_unused; if ( rc ) diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 807401b2eaa2..fa13397ae409 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -13,12 +13,16 @@ 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; + const char *priority; + bool is_ext; + int (*init)(struct pci_dev *pdev); +} vpci_capability_t; + #define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) /* @@ -29,9 +33,20 @@ 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_CAP(cap, x, p, ext) \ + static vpci_capability_t x##_t = { \ + .id = (cap), \ + .init = (x), \ + .priority = (p), \ + .is_ext = (ext), \ + }; \ + static vpci_capability_t *const x##_entry \ + __used_section(".data.vpci." p) = &(x##_t) + +#define REGISTER_VPCI_LEGACY_CAP(cap, x, p) REGISTER_VPCI_CAP(cap, x, p, false) +#define REGISTER_VPCI_EXTEND_CAP(cap, x, p) REGISTER_VPCI_CAP(cap, x, p, 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);
When vpci fails to initialize a capability of a device, it just return error instead of catching and processing exception. That makes the entire device unusable. So, refactor REGISTER_VPCI_INIT to contain more capability specific information, and try to hide capability when initialization fails in vpci_assign_device(). 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. Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- Hi all, This patch aims to hide a capability when its initialization fails. That causes we can't rely on vpci_deassign_device() to clean up assigned resources, so, following two patches clean up resources in the failure path of init function. Best regards, Jiqian Chen. --- xen/drivers/vpci/header.c | 3 +- xen/drivers/vpci/msi.c | 2 +- xen/drivers/vpci/msix.c | 2 +- xen/drivers/vpci/rebar.c | 2 +- xen/drivers/vpci/vpci.c | 65 +++++++++++++++++++++++++++++++++------ xen/include/xen/vpci.h | 27 ++++++++++++---- 6 files changed, 81 insertions(+), 20 deletions(-)