Re: [PATCH v10 03/17] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/12/23 18:09, Volodymyr Babchuk wrote:
> 6. We are removing multiple ASSERT(pcidevs_locked()) instances because
> they are too strict now: they should be corrected to
> ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)), but problem is
> that mentioned instances does not have access to the domain
> pointer and it is not feasible to pass a domain pointer to a function
> just for debugging purposes.

...

> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 20275260b3..466725d8ca 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -988,8 +988,6 @@ static int __pci_enable_msi(struct msi_info *msi, struct 
> msi_desc **desc,
>  {
>      struct msi_desc *old_desc;
> 
> -    ASSERT(pcidevs_locked());
> -
>      if ( !pdev )
>          return -ENODEV;

I know it is mentioned in the commit message, so the ASSERT removal above may 
be okay. However, just to mention it: as we are passing pdev to this function 
now, we can access the domain pointer here. So the ASSERT can be turned into 
(after the !pdev check):

    ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock));

In which case pdev->domain != NULL might also want to be checked.

> 
> @@ -1043,8 +1041,6 @@ static int __pci_enable_msix(struct msi_info *msi, 
> struct msi_desc **desc,
>  {
>      struct msi_desc *old_desc;
> 
> -    ASSERT(pcidevs_locked());
> -
>      if ( !pdev || !pdev->msix )
>          return -ENODEV;

Same here.

> 
> @@ -1154,8 +1150,6 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool 
> off)
>  int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc,
>                    struct pci_dev *pdev)
>  {
> -    ASSERT(pcidevs_locked());
> -
>      if ( !use_msi )
>          return -EPERM;
>