Re: [PATCH v7 11/12] xen/arm: translate virtual PCI bus topology for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.06.2023 12:32, Volodymyr Babchuk wrote:
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -180,6 +180,44 @@ static void vpci_remove_virtual_device(const struct 
> pci_dev *pdev)
>      write_unlock(&pdev->domain->vpci_rwlock);
>  }
>  
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
> +{
> +    struct pci_dev *pdev;
> +
> +    ASSERT(!is_hardware_domain(d));
> +
> +    read_lock(&d->vpci_rwlock);
> +    pcidevs_lock();
> +    for_each_pdev( d, pdev )
> +    {
> +        bool found;
> +
> +        if ( !pdev->vpci )
> +            continue;
> +
> +        spin_lock(&pdev->vpci->lock);

Following the description of patch 1, why does this lock need acquiring here?
The r/w lock acquired above should already guard against modification.

> +        found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf);

The LHS of the && is pointless here - when acquiring the lock you have
already de-referenced pdev->vpci.

> +        spin_unlock(&pdev->vpci->lock);
> +
> +        if ( found )
> +        {
> +            /* Replace guest SBDF with the physical one. */
> +            *sbdf = pdev->sbdf;
> +            pcidevs_unlock();
> +            read_unlock(&d->vpci_rwlock);
> +            return true;

What use is the result to the caller with all locks dropped, and hence
state possibly having changed before the caller gets a chance to look
at the result? If there are reason why this is okay, you want to (a)
spell out those reasons in the description and (b) document this
restriction in a comment, to warn people who may want to (re)use this
function. But really I think the caller needs to hold a suitable lock
until after the result from here was consumed.

Jan