On 20.07.2023 02:32, Volodymyr Babchuk wrote:
> @@ -431,10 +447,23 @@ static void vpci_write_helper(const struct pci_dev
> *pdev,
> r->private);
> }
>
> +/* Helper function to unlock locks taken by vpci_write in proper order */
> +static void unlock_locks(struct domain *d)
> +{
> + ASSERT(rw_is_locked(&d->pci_lock));
> +
> + if ( is_hardware_domain(d) )
> + {
> + ASSERT(rw_is_locked(&d->pci_lock));
Copy-and-past mistake? You've asserted this same condition already above.
> + read_unlock(&dom_xen->pci_lock);
> + }
> + read_unlock(&d->pci_lock);
> +}
> +
> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
> uint32_t data)
> {
> - const struct domain *d = current->domain;
> + struct domain *d = current->domain;
> const struct pci_dev *pdev;
> const struct vpci_register *r;
> unsigned int data_offset = 0;
> @@ -447,8 +476,16 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int size,
>
> /*
> * Find the PCI dev matching the address, which for hwdom also requires
> - * consulting DomXEN. Passthrough everything that's not trapped.
> + * consulting DomXEN. Passthrough everything that's not trapped.
> + * If this is hwdom, we need to hold locks for both domain in case if
> + * modify_bars is called()
> */
> + read_lock(&d->pci_lock);
> +
> + /* dom_xen->pci_lock always should be taken second to prevent deadlock */
> + if ( is_hardware_domain(d) )
> + read_lock(&dom_xen->pci_lock);
But I wonder anyway - can we perhaps get away without acquiring dom_xen's
lock here? Its list isn't altered anymore post-boot, iirc.
> @@ -498,6 +537,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int size,
> ASSERT(data_offset < size);
> }
> spin_unlock(&pdev->vpci->lock);
> + unlock_locks(d);
In this context the question arises whether the function wouldn't better
be named more specific to its purpose: It's obvious here that it doesn't
unlock all the locks involved.
Jan
