Re: [PATCH v3 5/6] vpci: use reference counter to protect vpci state

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

On Tue, Mar 14, 2023 at 08:56:30PM +0000, Volodymyr Babchuk wrote:
> vPCI MMIO handlers are accessing pdevs without protecting this
> access with pcidevs_{lock|unlock}. This is not a problem as of now
> as these are only used by Dom0. But, towards vPCI is used also for
> guests, we need to properly protect pdev and pdev->vpci from being
> removed while still in use.
> 
> For that use pdev reference counting.

I wonder whether vPCI does need to take another reference to the
device.  This all stems from me not having it fully clear how the
reference counting is supposed to be used for pdevs.

As mentioned in a previous patch, I would expect device assignation to
take a reference, and hence vPCI won't need to take an extra refcount
since vPCI can only be used once the device has been assigned to a
domain, and hence already has at least an extra reference taken from
the fact it's assigned to a domain.

If anything I would add an ASSERT(pdev->refcount > 1) or equivalent.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
> 
> v3:
>  - Moved from another patch series
> ---
>  xen/drivers/vpci/vpci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 199ff55672..005f38dc77 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -62,6 +62,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>      xfree(pdev->vpci->msi);
>      xfree(pdev->vpci);
>      pdev->vpci = NULL;
> +    pcidev_put(pdev);
>  }
>  
>  int vpci_add_handlers(struct pci_dev *pdev)
> @@ -72,6 +73,8 @@ int vpci_add_handlers(struct pci_dev *pdev)
>      if ( !has_vpci(pdev->domain) )
>          return 0;
>  
> +    pcidev_get(pdev);
> +
>      /* We should not get here twice for the same device. */
>      ASSERT(!pdev->vpci);

You are missing a pcidev_put() in case allocation of pdev->vpci fails.

Thanks, Roger.