| 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.
On 2025/3/27 17:25, Roger Pau Monné wrote: > 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? Yes, I used failure test cases of init_msi/rebar. From the "lspci" result, they were hided from the dom0. But I forgot to test for domUs. > 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(). Oh, I got your opinion. But we may need to discuss this more. In my opinion, there should be two situations: First, if device belongs to hardware domain, there is no emulation of legacy or extended capabilities linked list if I understand codes right. So, for this situation, I think current implementation of my patch is enough for hiding legacy or extended capabilities. Second, if device belongs to common domain, we just need to consider legacy capabilities since all extended capabilities are hided in init_header(). So, for this situation, I need to what you said " capability[n - 1].next_cap = capability[n].next_cap " I am not sure if above are right. > >> + 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. I have an idea, but it seems like a hake. Can we add a flag(maybe name it "msix_initialized") to struct vpci{}? Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we return an error to let modify_decoding() directly return in the process of init_header. And in the start of init_msix(), to set msix_initialized=true, in the end of init_msix(), to call modify_decoding() to setup p2m. Then we can remove the priorities. > > Thanks, Roger.
On Mon, Mar 31, 2025 at 07:26:20AM +0000, Chen, Jiqian wrote: > On 2025/3/27 17:25, Roger Pau Monné wrote: > > 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? > Yes, I used failure test cases of init_msi/rebar. > From the "lspci" result, they were hided from the dom0. > But I forgot to test for domUs. I assume that's only tested with Linux? See my comment below about capability ID 0 being reserved, and hence I think we should not keep capabilities with ID 0 on the list, as it might cause malfunctions to OSes. > > 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(). > Oh, I got your opinion. > But we may need to discuss this more. > In my opinion, there should be two situations: > First, if device belongs to hardware domain, there is no emulation of legacy or extended capabilities linked list if I understand codes right. Yes, but that needs to be fixed, we need to have this kind of emulation uniformly. > So, for this situation, I think current implementation of my patch is enough for hiding legacy or extended capabilities. It works given the current code in Linux. As said above, I don't think this is fully correct according to the PCI spec. > Second, if device belongs to common domain, we just need to consider legacy capabilities since all extended capabilities are hided in init_header(). > So, for this situation, I need to what you said " capability[n - 1].next_cap = capability[n].next_cap " I'm not sure why would want to handle the hardware domain vs unprivileged domains differently here. The way to hide the capabilities should always be the same, like it's currently done for domUs. > I am not sure if above are right. > > > >> + 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. > I have an idea, but it seems like a hake. > Can we add a flag(maybe name it "msix_initialized") to struct vpci{}? > Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we return an error to let modify_decoding() directly return in the process of init_header. > And in the start of init_msix(), to set msix_initialized=true, in the end of init_msix(), to call modify_decoding() to setup p2m. > Then we can remove the priorities. Maybe the initialization of the MSI-X capability could be done after the header, and call vpci_make_msix_hole()? There's a bit of redundancy here in that the BAR is first fully mapped, and then a hole is punched in place of the MSI-X related tables. Seems like the easier option to break the depedency of init_msix() in being called ahead of init_header(). Completely unrelated: looking at vpci_make_msix_hole() I see the call in modify_decoding() is redundant, as modify_bars() already craves the MSI-X regions out of the BARs. Thanks, Roger.
On 31.03.2025 10:43, Roger Pau Monné wrote: > On Mon, Mar 31, 2025 at 07:26:20AM +0000, Chen, Jiqian wrote: >> On 2025/3/27 17:25, Roger Pau Monné wrote: >>> On Thu, Mar 27, 2025 at 03:32:12PM +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[]; >>>> #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? >> Yes, I used failure test cases of init_msi/rebar. >> From the "lspci" result, they were hided from the dom0. >> But I forgot to test for domUs. > > I assume that's only tested with Linux? See my comment below about > capability ID 0 being reserved, and hence I think we should not keep > capabilities with ID 0 on the list, as it might cause malfunctions to > OSes. > >>> 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(). >> Oh, I got your opinion. >> But we may need to discuss this more. >> In my opinion, there should be two situations: >> First, if device belongs to hardware domain, there is no emulation of legacy or extended capabilities linked list if I understand codes right. > > Yes, but that needs to be fixed, we need to have this kind of > emulation uniformly. > >> So, for this situation, I think current implementation of my patch is enough for hiding legacy or extended capabilities. > > It works given the current code in Linux. As said above, I don't > think this is fully correct according to the PCI spec. > >> Second, if device belongs to common domain, we just need to consider legacy capabilities since all extended capabilities are hided in init_header(). >> So, for this situation, I need to what you said " capability[n - 1].next_cap = capability[n].next_cap " > > I'm not sure why would want to handle the hardware domain vs > unprivileged domains differently here. The way to hide the > capabilities should always be the same, like it's currently done for > domUs. FWIW - I agree with Roger on all the point made. Jan
On 2025/3/31 16:43, Roger Pau Monné wrote: > On Mon, Mar 31, 2025 at 07:26:20AM +0000, Chen, Jiqian wrote: >> On 2025/3/27 17:25, Roger Pau Monné wrote: >>> On Thu, Mar 27, 2025 at 03:32:12PM +0800, Jiqian Chen wrote: >>>> #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? >> Yes, I used failure test cases of init_msi/rebar. >> From the "lspci" result, they were hided from the dom0. >> But I forgot to test for domUs. > > I assume that's only tested with Linux? See my comment below about > capability ID 0 being reserved, and hence I think we should not keep > capabilities with ID 0 on the list, as it might cause malfunctions to > OSes. > >>> 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(). >> Oh, I got your opinion. >> But we may need to discuss this more. >> In my opinion, there should be two situations: >> First, if device belongs to hardware domain, there is no emulation of legacy or extended capabilities linked list if I understand codes right. > > Yes, but that needs to be fixed, we need to have this kind of > emulation uniformly. > >> So, for this situation, I think current implementation of my patch is enough for hiding legacy or extended capabilities. > > It works given the current code in Linux. As said above, I don't > think this is fully correct according to the PCI spec. > >> Second, if device belongs to common domain, we just need to consider legacy capabilities since all extended capabilities are hided in init_header(). >> So, for this situation, I need to what you said " capability[n - 1].next_cap = capability[n].next_cap " > > I'm not sure why would want to handle the hardware domain vs > unprivileged domains differently here. The way to hide the > capabilities should always be the same, like it's currently done for > domUs. So, I need to refactor the emulating PCI capability list codes of init_header() to serve for all domain(dom0+domUs) firstly, since current codes only emulate PCI capability list for domUs, right? > >> I am not sure if above are right. >>> >>>> + 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. >> I have an idea, but it seems like a hake. >> Can we add a flag(maybe name it "msix_initialized") to struct vpci{}? >> Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we return an error to let modify_decoding() directly return in the process of init_header. >> And in the start of init_msix(), to set msix_initialized=true, in the end of init_msix(), to call modify_decoding() to setup p2m. >> Then we can remove the priorities. > > Maybe the initialization of the MSI-X capability could be done after > the header, and call vpci_make_msix_hole()? There's a bit of > redundancy here in that the BAR is first fully mapped, and then a hole > is punched in place of the MSI-X related tables. Seems like the > easier option to break the depedency of init_msix() in being called > ahead of init_header(). You mean the sequence should be: vpci_init_header() vpci_init_capability() // all capabilities vpci_make_msix_hole() Right? > > Completely unrelated: looking at vpci_make_msix_hole() I see the call > in modify_decoding() is redundant, as modify_bars() already craves the > MSI-X regions out of the BARs. > > Thanks, Roger.
On Mon, Mar 31, 2025 at 09:32:02AM +0000, Chen, Jiqian wrote: > On 2025/3/31 16:43, Roger Pau Monné wrote: > > On Mon, Mar 31, 2025 at 07:26:20AM +0000, Chen, Jiqian wrote: > >> On 2025/3/27 17:25, Roger Pau Monné wrote: > >>> On Thu, Mar 27, 2025 at 03:32:12PM +0800, Jiqian Chen wrote: > >>>> #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? > >> Yes, I used failure test cases of init_msi/rebar. > >> From the "lspci" result, they were hided from the dom0. > >> But I forgot to test for domUs. > > > > I assume that's only tested with Linux? See my comment below about > > capability ID 0 being reserved, and hence I think we should not keep > > capabilities with ID 0 on the list, as it might cause malfunctions to > > OSes. > > > >>> 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(). > >> Oh, I got your opinion. > >> But we may need to discuss this more. > >> In my opinion, there should be two situations: > >> First, if device belongs to hardware domain, there is no emulation of legacy or extended capabilities linked list if I understand codes right. > > > > Yes, but that needs to be fixed, we need to have this kind of > > emulation uniformly. > > > >> So, for this situation, I think current implementation of my patch is enough for hiding legacy or extended capabilities. > > > > It works given the current code in Linux. As said above, I don't > > think this is fully correct according to the PCI spec. > > > >> Second, if device belongs to common domain, we just need to consider legacy capabilities since all extended capabilities are hided in init_header(). > >> So, for this situation, I need to what you said " capability[n - 1].next_cap = capability[n].next_cap " > > > > I'm not sure why would want to handle the hardware domain vs > > unprivileged domains differently here. The way to hide the > > capabilities should always be the same, like it's currently done for > > domUs. > So, I need to refactor the emulating PCI capability list codes of init_header() to serve > for all domain(dom0+domUs) firstly, since current codes only emulate PCI capability list for domUs, right? Yes, that would be my understanding. The current logic in init_header() needs to be expanded/generalized so it can be used for masking random PCI capabilities, plus adapted to work with PCIe capabilities also. > > > >> I am not sure if above are right. > >>> > >>>> + 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. > >> I have an idea, but it seems like a hake. > >> Can we add a flag(maybe name it "msix_initialized") to struct vpci{}? > >> Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we return an error to let modify_decoding() directly return in the process of init_header. > >> And in the start of init_msix(), to set msix_initialized=true, in the end of init_msix(), to call modify_decoding() to setup p2m. > >> Then we can remove the priorities. > > > > Maybe the initialization of the MSI-X capability could be done after > > the header, and call vpci_make_msix_hole()? There's a bit of > > redundancy here in that the BAR is first fully mapped, and then a hole > > is punched in place of the MSI-X related tables. Seems like the > > easier option to break the depedency of init_msix() in being called > > ahead of init_header(). > You mean the sequence should be: > vpci_init_header() > vpci_init_capability() // all capabilities > vpci_make_msix_hole() > > Right? Yes, I think that would be my preference. The call to vpci_make_msix_hole() should be inside of init_msix(). Thanks, Roger.
On 2025/3/31 19:04, Roger Pau Monné wrote: > On Mon, Mar 31, 2025 at 09:32:02AM +0000, Chen, Jiqian wrote: >> So, I need to refactor the emulating PCI capability list codes of init_header() to serve >> for all domain(dom0+domUs) firstly, since current codes only emulate PCI capability list for domUs, right? > > Yes, that would be my understanding. The current logic in > init_header() needs to be expanded/generalized so it can be used for > masking random PCI capabilities, plus adapted to work with PCIe > capabilities also. OK, I will try to refactor the logic in next version. Hoping the next version will be more in line with your ideas. Thanks! > >>> >>>> >>>>> >>>>>> + /* >>>>>> + * 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. >>>> I have an idea, but it seems like a hake. >>>> Can we add a flag(maybe name it "msix_initialized") to struct vpci{}? >>>> Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we return an error to let modify_decoding() directly return in the process of init_header. >>>> And in the start of init_msix(), to set msix_initialized=true, in the end of init_msix(), to call modify_decoding() to setup p2m. >>>> Then we can remove the priorities. >>> >>> Maybe the initialization of the MSI-X capability could be done after >>> the header, and call vpci_make_msix_hole()? There's a bit of >>> redundancy here in that the BAR is first fully mapped, and then a hole >>> is punched in place of the MSI-X related tables. Seems like the >>> easier option to break the depedency of init_msix() in being called >>> ahead of init_header(). >> You mean the sequence should be: >> vpci_init_header() >> vpci_init_capability() // all capabilities >> vpci_make_msix_hole() >> >> Right? > > Yes, I think that would be my preference. The call to > vpci_make_msix_hole() should be inside of init_msix(). Got it, will do in next version. > > 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(-)