Hi Stefano,
Thank you for the review Stefano Stabellini <[email protected]> writes: > On Wed, 31 Aug 2022, Volodymyr Babchuk wrote: >> Prior to this change, lifetime of pci_dev objects was protected by global >> pcidevs_lock(). We are going to get if of this lock, so we need some >> other mechanism to ensure that those objects will not disappear under >> feet of code that access them. Reference counting is a good choice as >> it provides easy to comprehend way to control object lifetime with >> better granularity than global super lock. >> >> This patch adds two new helper functions: pcidev_get() and >> pcidev_put(). pcidev_get() will increase reference counter, while >> pcidev_put() will decrease it, destroying object when counter reaches >> zero. >> >> pcidev_get() should be used only when you already have a valid pointer >> to the object or you are holding lock that protects one of the >> lists (domain, pseg or ats) that store pci_dev structs. >> >> pcidev_get() is rarely used directly, because there already are >> functions that will provide valid pointer to pci_dev struct: >> pci_get_pdev() and pci_get_real_pdev(). They will lock appropriate >> list, find needed object and increase its reference counter before >> returning to the caller. >> >> Naturally, pci_put() should be called after finishing working with a >> received object. This is the reason why this patch have so many >> pcidev_put()s and so little pcidev_get()s: existing calls to >> pci_get_*() functions now will increase reference counter >> automatically, we just need to decrease it back when we finished. >> >> This patch removes "const" qualifier from some pdev pointers because >> pcidev_put() technically alters the contents of pci_dev structure. >> >> Signed-off-by: Volodymyr Babchuk <[email protected]> > > tabs everywhere in this patch > Oh, yes, sorry. I asked sometime ago, and want to ask again: instead of adding EMACS magic into each file, we can put one .dir-locals.el file with basically the same config in xen/ directory. This will accomplish two things: - there will be no need to add EMACS magic strings into each new file - the same config will apply to files that do not have magic strings. Files with different coding style rules can be filtered by code in .dir-locals.el and/or by strategically placed files in sub-directories. I am happy to hear maintainers opinion about this. >> --- >> >> - Jan, can I add your Suggested-by tag? >> --- >> xen/arch/x86/hvm/vmsi.c | 2 +- >> xen/arch/x86/irq.c | 4 + >> xen/arch/x86/msi.c | 41 ++++++- >> xen/arch/x86/pci.c | 4 +- >> xen/arch/x86/physdev.c | 17 ++- >> xen/common/sysctl.c | 5 +- >> xen/drivers/passthrough/amd/iommu_init.c | 12 ++- >> xen/drivers/passthrough/amd/iommu_map.c | 6 +- >> xen/drivers/passthrough/pci.c | 131 +++++++++++++++-------- >> xen/drivers/passthrough/vtd/quirks.c | 2 + >> xen/drivers/video/vga.c | 10 +- >> xen/drivers/vpci/vpci.c | 6 +- >> xen/include/xen/pci.h | 18 ++++ >> 13 files changed, 201 insertions(+), 57 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c >> index 75f92885dc..7fb1075673 100644 >> --- a/xen/arch/x86/hvm/vmsi.c >> +++ b/xen/arch/x86/hvm/vmsi.c >> @@ -912,7 +912,7 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) >> >> spin_unlock(&msix->pdev->vpci->lock); >> process_pending_softirqs(); >> - /* NB: we assume that pdev cannot go away for an alive domain. >> */ >> + >> if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) ) >> return -EBUSY; >> if ( pdev->vpci->msix != msix ) >> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c >> index cd0c8a30a8..d8672a03e1 100644 >> --- a/xen/arch/x86/irq.c >> +++ b/xen/arch/x86/irq.c >> @@ -2174,6 +2174,7 @@ int map_domain_pirq( >> msi->entry_nr = ret; >> ret = -ENFILE; >> } >> + pcidev_put(pdev); > > I think it would be better to move pcidev_put just after done: I'd love to do this, but pdev is declared inside "if" block while "done:" is outside of this scope. I can move pdev into outer scope if you believe that it will be better. [...] All other comments were taken into account.
