Re: [RFC PATCH 05/10] xen: pci: introduce reference counting for pdev

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Stefano,

Thank you for the review

Stefano Stabellini <sstabellini@xxxxxxxxxx> 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 <volodymyr_babchuk@xxxxxxxx>
>
> 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.