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;
>
