Re: [PATCH v5 09/10] vpci/msi: Free MSI resources when init_msi() fails

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jun 06, 2025 at 08:38:49AM +0000, Chen, Jiqian wrote:
> On 2025/6/5 23:19, Roger Pau Monné wrote:
> > On Mon, May 26, 2025 at 05:45:58PM +0800, Jiqian Chen wrote:
> >> When init_msi() fails, current logic return fail and free MSI-related
> >> resources in vpci_deassign_device(). But the previous new changes will
> >> hide MSI capability and return success, it can't reach
> >> vpci_deassign_device() to remove resources if hiding success, so those
> >> resources must be removed in cleanup function of MSI.
> >>
> >> To do that, implement cleanup function for MSI.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> v4->v5 changes:
> >> * Change definition "static void cleanup_msi" to "static int cf_check 
> >> cleanup_msi" since cleanup hook is changed to be int.
> >> * Add a read-only register for MSI Control Register in the end of 
> >> cleanup_msi.
> >>
> >> v3->v4 changes:
> >> * Change function name from fini_msi() to cleanup_msi().
> >> * Remove unnecessary comment.
> >> * Change to use XFREE to free vpci->msi.
> >>
> >> v2->v3 changes:
> >> * Remove all fail path, and use fini_msi() hook instead.
> >> * Change the method to calculating the size of msi registers.
> >>
> >> v1->v2 changes:
> >> * Added a new function fini_msi to free all MSI resources instead of using 
> >> an array to record registered registers.
> >>
> >> Best regards,
> >> Jiqian Chen.
> >> ---
> >>  xen/drivers/vpci/msi.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> >> index 2d45c7867de7..4e106c39efae 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;
> >>  }
> >>  
> >> +static int cf_check cleanup_msi(struct pci_dev *pdev)
> >> +{
> >> +    int rc;
> >> +    unsigned int end, size;
> >> +    struct vpci *vpci = pdev->vpci;
> >> +    const unsigned int msi_pos = pdev->msi_pos;
> >> +    const unsigned int ctrl = msi_control_reg(msi_pos);
> >> +
> >> +    if ( !msi_pos || !vpci->msi )
> > 
> > Possibly same request as the previous patch, msi_pos should be after
> > the PCI standard header.
> msi_pos <= PCI_CAPABILITY_LIST ?
> Or msi_pos < 0x40 ?

Hm, no, ignore this, sorry.

Thanks, Roger.