| Message ID | 20230314205612.3703668-6-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vpci: first series in preparation for vpci on ARM | expand |
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@epam.com> > Suggested-by: Jan Beulich <jbeulich@suse.com> > > --- > > 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.
On 17.03.2023 09:43, Roger Pau Monné wrote: > 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. FWIW: +1 Jan
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);
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. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> Suggested-by: Jan Beulich <jbeulich@suse.com> --- v3: - Moved from another patch series --- xen/drivers/vpci/vpci.c | 3 +++ 1 file changed, 3 insertions(+)