| Message ID | 20250409064528.405573-8-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support hiding capability when its initialization fails | expand |
On Wed, Apr 09, 2025 at 02:45:27PM +0800, Jiqian Chen wrote: > When init_msi() fails, the previous new changes will hide MSI > capability, it can't rely on vpci_deassign_device() to remove > all MSI related resources anymore, those resources must be > cleaned up in failure path of init_msi. > > To do that, add a new function to free MSI resources. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > cc: "Roger Pau Monné" <roger.pau@citrix.com> > --- > v1->v2 changes: > * Added a new function fini_msi to free all MSI resources instead of using an array to record registered registers. > --- > xen/drivers/vpci/msi.c | 47 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 10 deletions(-) > > diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c > index ca89ae9b9c22..48a466dba0ef 100644 > --- a/xen/drivers/vpci/msi.c > +++ b/xen/drivers/vpci/msi.c > @@ -193,6 +193,33 @@ static void cf_check mask_write( > msi->mask = val; > } > > +/* 12 is size of MSI structure for 32-bit Message Address without PVM */ > +#define MSI_STRUCTURE_SIZE_32 12 I'm confused by this. The minimum size of the capability is 4 bytes for the capability pointer, plus 4 bytes for the message address, plus 2 bytes for the message data. So that's 10 bytes in total. I think it would be best if you calculate the size based on the existing msi_ macros. if ( masking ) end = msi_pending_bits_reg(msi_pos, address64); else end = msi_mask_bits_reg(msi_pos, address64) - 2; size = end - msi_control_reg(msi_pos); > + > +static void fini_msi(struct pci_dev *pdev) > +{ > + unsigned int size = MSI_STRUCTURE_SIZE_32; > + > + if ( !pdev->vpci->msi ) > + return; > + > + if ( pdev->vpci->msi->address64 ) > + size += 4; > + if ( pdev->vpci->msi->masking ) > + size += 4; > + > + /* > + * Remove all possible registered registers except capability ID > + * register and next capability pointer register, which will be > + * removed in mask function. > + *msi_mask_bits_reg/ > + vpci_remove_registers(pdev->vpci, > + msi_control_reg(pdev->msi_pos), > + size - PCI_MSI_FLAGS); > + xfree(pdev->vpci->msi); > + pdev->vpci->msi = NULL; > +} > + > static int cf_check init_msi(struct pci_dev *pdev) > { > unsigned int pos = pdev->msi_pos; > @@ -209,12 +236,7 @@ static int cf_check init_msi(struct pci_dev *pdev) > ret = vpci_add_register(pdev->vpci, control_read, control_write, > msi_control_reg(pos), 2, pdev->vpci->msi); > if ( ret ) > - /* > - * NB: there's no need to free the msi struct or remove the register > - * handlers form the config space, the caller will take care of the > - * cleanup. > - */ > - return ret; > + goto fail; > > /* Get the maximum number of vectors the device supports. */ > control = pci_conf_read16(pdev->sbdf, msi_control_reg(pos)); > @@ -237,20 +259,20 @@ static int cf_check init_msi(struct pci_dev *pdev) > ret = vpci_add_register(pdev->vpci, address_read, address_write, > msi_lower_address_reg(pos), 4, pdev->vpci->msi); > if ( ret ) > - return ret; > + goto fail; > > ret = vpci_add_register(pdev->vpci, data_read, data_write, > msi_data_reg(pos, pdev->vpci->msi->address64), 2, > pdev->vpci->msi); > if ( ret ) > - return ret; > + goto fail; > > if ( pdev->vpci->msi->address64 ) > { > ret = vpci_add_register(pdev->vpci, address_hi_read, address_hi_write, > msi_upper_address_reg(pos), 4, pdev->vpci->msi); > if ( ret ) > - return ret; > + goto fail; > } > > if ( pdev->vpci->msi->masking ) > @@ -260,7 +282,7 @@ static int cf_check init_msi(struct pci_dev *pdev) > pdev->vpci->msi->address64), > 4, pdev->vpci->msi); > if ( ret ) > - return ret; > + goto fail; > /* > * FIXME: do not add any handler for the pending bits for the hardware > * domain, which means direct access. This will be revisited when > @@ -269,6 +291,11 @@ static int cf_check init_msi(struct pci_dev *pdev) > } > > return 0; > + > + fail: > + fini_msi(pdev); > + > + return ret; > } > REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi); I wonder if the fini_msi should be referenced in REGISTER_VPCI_{EXTENDED,LEGACY}_CAP(), so that the caller of init_msi() can call fini_msi() on failure and thus avoid all those fail hooks and labels, as the caller can take care of the cleanup. Thanks, Roger.
On 2025/4/15 21:29, Roger Pau Monné wrote: > On Wed, Apr 09, 2025 at 02:45:27PM +0800, Jiqian Chen wrote: >> When init_msi() fails, the previous new changes will hide MSI >> capability, it can't rely on vpci_deassign_device() to remove >> all MSI related resources anymore, those resources must be >> cleaned up in failure path of init_msi. >> >> To do that, add a new function to free MSI resources. >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> --- >> cc: "Roger Pau Monné" <roger.pau@citrix.com> >> --- >> v1->v2 changes: >> * Added a new function fini_msi to free all MSI resources instead of using an array to record registered registers. >> --- >> xen/drivers/vpci/msi.c | 47 +++++++++++++++++++++++++++++++++--------- >> 1 file changed, 37 insertions(+), 10 deletions(-) >> >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c >> index ca89ae9b9c22..48a466dba0ef 100644 >> --- a/xen/drivers/vpci/msi.c >> +++ b/xen/drivers/vpci/msi.c >> @@ -193,6 +193,33 @@ static void cf_check mask_write( >> msi->mask = val; >> } >> >> +/* 12 is size of MSI structure for 32-bit Message Address without PVM */ >> +#define MSI_STRUCTURE_SIZE_32 12 > > I'm confused by this. The minimum size of the capability is 4 bytes > for the capability pointer, plus 4 bytes for the message address, > plus 2 bytes for the message data. So that's 10 bytes in total. The remain 2 bytes is Extended Message Data, PCIe spec has this register's definition even though it is optional. > > I think it would be best if you calculate the size based on the > existing msi_ macros. > > if ( masking ) > end = msi_pending_bits_reg(msi_pos, address64); > else > end = msi_mask_bits_reg(msi_pos, address64) - 2; > > size = end - msi_control_reg(msi_pos); Thanks, I will change to this in next version. > >> + >> +static void fini_msi(struct pci_dev *pdev) >> +{ >> + unsigned int size = MSI_STRUCTURE_SIZE_32; >> + >> + if ( !pdev->vpci->msi ) >> + return; >> + >> + if ( pdev->vpci->msi->address64 ) >> + size += 4; >> + if ( pdev->vpci->msi->masking ) >> + size += 4; >> + >> + /* >> + * Remove all possible registered registers except capability ID >> + * register and next capability pointer register, which will be >> + * removed in mask function. >> + *msi_mask_bits_reg/ >> + vpci_remove_registers(pdev->vpci, >> + msi_control_reg(pdev->msi_pos), >> + size - PCI_MSI_FLAGS); >> + xfree(pdev->vpci->msi); >> + pdev->vpci->msi = NULL; >> +} >> + >> static int cf_check init_msi(struct pci_dev *pdev) >> { >> unsigned int pos = pdev->msi_pos; >> @@ -209,12 +236,7 @@ static int cf_check init_msi(struct pci_dev *pdev) >> ret = vpci_add_register(pdev->vpci, control_read, control_write, >> msi_control_reg(pos), 2, pdev->vpci->msi); >> if ( ret ) >> - /* >> - * NB: there's no need to free the msi struct or remove the register >> - * handlers form the config space, the caller will take care of the >> - * cleanup. >> - */ >> - return ret; >> + goto fail; >> >> /* Get the maximum number of vectors the device supports. */ >> control = pci_conf_read16(pdev->sbdf, msi_control_reg(pos)); >> @@ -237,20 +259,20 @@ static int cf_check init_msi(struct pci_dev *pdev) >> ret = vpci_add_register(pdev->vpci, address_read, address_write, >> msi_lower_address_reg(pos), 4, pdev->vpci->msi); >> if ( ret ) >> - return ret; >> + goto fail; >> >> ret = vpci_add_register(pdev->vpci, data_read, data_write, >> msi_data_reg(pos, pdev->vpci->msi->address64), 2, >> pdev->vpci->msi); >> if ( ret ) >> - return ret; >> + goto fail; >> >> if ( pdev->vpci->msi->address64 ) >> { >> ret = vpci_add_register(pdev->vpci, address_hi_read, address_hi_write, >> msi_upper_address_reg(pos), 4, pdev->vpci->msi); >> if ( ret ) >> - return ret; >> + goto fail; >> } >> >> if ( pdev->vpci->msi->masking ) >> @@ -260,7 +282,7 @@ static int cf_check init_msi(struct pci_dev *pdev) >> pdev->vpci->msi->address64), >> 4, pdev->vpci->msi); >> if ( ret ) >> - return ret; >> + goto fail; >> /* >> * FIXME: do not add any handler for the pending bits for the hardware >> * domain, which means direct access. This will be revisited when >> @@ -269,6 +291,11 @@ static int cf_check init_msi(struct pci_dev *pdev) >> } >> >> return 0; >> + >> + fail: >> + fini_msi(pdev); >> + >> + return ret; >> } >> REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi); > > I wonder if the fini_msi should be referenced in > REGISTER_VPCI_{EXTENDED,LEGACY}_CAP(), so that the caller of > init_msi() can call fini_msi() on failure and thus avoid all those > fail hooks and labels, as the caller can take care of the cleanup. Got it. I will add a hook for fini_x function. > > Thanks, Roger.
On Wed, Apr 16, 2025 at 06:16:36AM +0000, Chen, Jiqian wrote: > On 2025/4/15 21:29, Roger Pau Monné wrote: > > On Wed, Apr 09, 2025 at 02:45:27PM +0800, Jiqian Chen wrote: > >> When init_msi() fails, the previous new changes will hide MSI > >> capability, it can't rely on vpci_deassign_device() to remove > >> all MSI related resources anymore, those resources must be > >> cleaned up in failure path of init_msi. > >> > >> To do that, add a new function to free MSI resources. > >> > >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > >> --- > >> cc: "Roger Pau Monné" <roger.pau@citrix.com> > >> --- > >> v1->v2 changes: > >> * Added a new function fini_msi to free all MSI resources instead of using an array to record registered registers. > >> --- > >> xen/drivers/vpci/msi.c | 47 +++++++++++++++++++++++++++++++++--------- > >> 1 file changed, 37 insertions(+), 10 deletions(-) > >> > >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c > >> index ca89ae9b9c22..48a466dba0ef 100644 > >> --- a/xen/drivers/vpci/msi.c > >> +++ b/xen/drivers/vpci/msi.c > >> @@ -193,6 +193,33 @@ static void cf_check mask_write( > >> msi->mask = val; > >> } > >> > >> +/* 12 is size of MSI structure for 32-bit Message Address without PVM */ > >> +#define MSI_STRUCTURE_SIZE_32 12 > > > > I'm confused by this. The minimum size of the capability is 4 bytes > > for the capability pointer, plus 4 bytes for the message address, > > plus 2 bytes for the message data. So that's 10 bytes in total. > The remain 2 bytes is Extended Message Data, PCIe spec has this register's definition even though it is optional. I was looking at the PCI Local Bus Specification rev 3.0 (because that has nicer figures IMO), and such field is not listed there. It's listed in the PCI Express 6.0.1 specification. I have to admit I've completely forgot about such optional feature. We never expose this bit to the guest in the control register, and consequently we never explicitly handle accesses to what would be the extended message data register, neither allow guests to enable it. If/when we add support for such field we will likely need to adjust fini_msi() to also zap it if present. Thanks, Roger.
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index ca89ae9b9c22..48a466dba0ef 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -193,6 +193,33 @@ static void cf_check mask_write( msi->mask = val; } +/* 12 is size of MSI structure for 32-bit Message Address without PVM */ +#define MSI_STRUCTURE_SIZE_32 12 + +static void fini_msi(struct pci_dev *pdev) +{ + unsigned int size = MSI_STRUCTURE_SIZE_32; + + if ( !pdev->vpci->msi ) + return; + + if ( pdev->vpci->msi->address64 ) + size += 4; + if ( pdev->vpci->msi->masking ) + size += 4; + + /* + * Remove all possible registered registers except capability ID + * register and next capability pointer register, which will be + * removed in mask function. + */ + vpci_remove_registers(pdev->vpci, + msi_control_reg(pdev->msi_pos), + size - PCI_MSI_FLAGS); + xfree(pdev->vpci->msi); + pdev->vpci->msi = NULL; +} + static int cf_check init_msi(struct pci_dev *pdev) { unsigned int pos = pdev->msi_pos; @@ -209,12 +236,7 @@ static int cf_check init_msi(struct pci_dev *pdev) ret = vpci_add_register(pdev->vpci, control_read, control_write, msi_control_reg(pos), 2, pdev->vpci->msi); if ( ret ) - /* - * NB: there's no need to free the msi struct or remove the register - * handlers form the config space, the caller will take care of the - * cleanup. - */ - return ret; + goto fail; /* Get the maximum number of vectors the device supports. */ control = pci_conf_read16(pdev->sbdf, msi_control_reg(pos)); @@ -237,20 +259,20 @@ static int cf_check init_msi(struct pci_dev *pdev) ret = vpci_add_register(pdev->vpci, address_read, address_write, msi_lower_address_reg(pos), 4, pdev->vpci->msi); if ( ret ) - return ret; + goto fail; ret = vpci_add_register(pdev->vpci, data_read, data_write, msi_data_reg(pos, pdev->vpci->msi->address64), 2, pdev->vpci->msi); if ( ret ) - return ret; + goto fail; if ( pdev->vpci->msi->address64 ) { ret = vpci_add_register(pdev->vpci, address_hi_read, address_hi_write, msi_upper_address_reg(pos), 4, pdev->vpci->msi); if ( ret ) - return ret; + goto fail; } if ( pdev->vpci->msi->masking ) @@ -260,7 +282,7 @@ static int cf_check init_msi(struct pci_dev *pdev) pdev->vpci->msi->address64), 4, pdev->vpci->msi); if ( ret ) - return ret; + goto fail; /* * FIXME: do not add any handler for the pending bits for the hardware * domain, which means direct access. This will be revisited when @@ -269,6 +291,11 @@ static int cf_check init_msi(struct pci_dev *pdev) } return 0; + + fail: + fini_msi(pdev); + + return ret; } REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi);
When init_msi() fails, the previous new changes will hide MSI capability, it can't rely on vpci_deassign_device() to remove all MSI related resources anymore, those resources must be cleaned up in failure path of init_msi. To do that, add a new function to free MSI resources. Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- cc: "Roger Pau Monné" <roger.pau@citrix.com> --- v1->v2 changes: * Added a new function fini_msi to free all MSI resources instead of using an array to record registered registers. --- xen/drivers/vpci/msi.c | 47 +++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 10 deletions(-)