Re: [PATCH v5 08/10] vpci/rebar: Free Rebar resources when init_rebar() fails

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

On Fri, Jun 06, 2025 at 08:32:35AM +0000, Chen, Jiqian wrote:
> On 2025/6/5 23:14, Roger Pau Monné wrote:
> > On Mon, May 26, 2025 at 05:45:57PM +0800, Jiqian Chen wrote:
> >> When init_rebar() fails, current logic return fail and free Rebar-related
> >> resources in vpci_deassign_device(). But the previous new changes will
> >> hide Rebar 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 Rebar.
> >>
> >> To do that, implement cleanup function for Rebar.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> > 
> > LGTM, just one nit about a bounds check.
> > 
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> v4->v5 changes:
> >> * Change definition "static void cleanup_rebar" to "static int cf_check 
> >> cleanup_rebar" since cleanup hook is changed to be int.
> >>
> >> v3->v4 changes:
> >> * Change function name from fini_rebar() to cleanup_rebar().
> >> * Change the error number to be E2BIG and ENXIO in init_rebar().
> >>
> >> v2->v3 changes:
> >> * Use fini_rebar() to remove all register instead of in the failure path 
> >> of init_rebar();
> >>
> >> v1->v2 changes:
> >> * Called vpci_remove_registers() to remove all possible registered 
> >> registers instead of using a array to record all registered register.
> >>
> >> Best regards,
> >> Jiqian Chen.
> >> ---
> >>  xen/drivers/vpci/rebar.c | 35 ++++++++++++++++++++++++-----------
> >>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> >> index 9cafd80ca2c9..4b1892fab3d6 100644
> >> --- a/xen/drivers/vpci/rebar.c
> >> +++ b/xen/drivers/vpci/rebar.c
> >> @@ -49,6 +49,26 @@ static void cf_check rebar_ctrl_write(const struct 
> >> pci_dev *pdev,
> >>      bar->guest_addr = bar->addr;
> >>  }
> >>  
> >> +static int cf_check cleanup_rebar(struct pci_dev *pdev)
> >> +{
> >> +    uint32_t ctrl;
> >> +    unsigned int nbars;
> >> +    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
> >> +                                                        
> >> PCI_EXT_CAP_ID_REBAR);
> >> +
> >> +    if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
> > 
> > I think you could check rebar_offset < PCI_CFG_SPACE_SIZE to be more
> > accurate?
> OK.
> Do I need to change in init_rebar()?

Hm, pci_find_ext_capability() will never return a value <
PCI_CFG_SPACE_SIZE different than 0, so maybe none of this is needed.
Just leave it like you have, and please ignore the other requests to
change the checks, sorry for the fuss.

Thanks, Roger.