[prev in list] [next in list] [prev in thread] [next in thread] 

List:       xen-devel
Subject:    Re: [RFC PATCH 07/10] xen: pci: add per-device locking
From:       Jan Beulich <jbeulich () suse ! com>
Date:       2023-02-28 16:46:48
Message-ID: c143af4c-85b8-b379-a8c5-3c4d6703887d () suse ! com
[Download RAW message or body]

On 31.08.2022 16:11, Volodymyr Babchuk wrote:
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc(
>  
>      nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE;
>  
> +    pcidev_lock(entry->pdev);
>      list_for_each_entry( desc, &entry->pdev->msi_list, list )
>          if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX &&
> -             desc->msi_attrib.entry_nr == nr_entry )
> +             desc->msi_attrib.entry_nr == nr_entry ) {
> +	    pcidev_unlock(entry->pdev);
>              return desc;

This is a potentially problematic pattern: desc has a backlink to pdev,
just like "entry" has. _If_ locking is required here (and the
refcounting is insufficient), then it is questionable whether the lock
can actually be dropped before returning. The idea with refcounting was,
though, that entities holding a reference can be sure the pdev won't go
away.

But of course there's also the question what "access to device itself"
(as stated in the description) does (or does not) constitute. I think
it is pretty crucial that for every new lock it is spelled out clearly
what it protects.

Seeing the list iteration pattern here (and at least once below)
another question is whether a lock like the one here may want to be a
read/write one.

Jan

[prev in list] [next in list] [prev in thread] [next in thread]