Re: [PATCH v2] vpci: introduce per-domain lock to protect vpci structure

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 15.02.22 10:11, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> @@ -171,8 +173,24 @@ static int __init apply_map(struct domain *d, const 
> struct pci_dev *pdev,
>       struct map_data data = { .d = d, .map = true };
>       int rc;
>   
> +    ASSERT(rw_is_write_locked(&d->vpci_rwlock));
> +
>       while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == 
> -ERESTART )
> +    {
> +        /*
> +         * FIXME: apply_map is called from dom0 specific init code when
> +         * system_state < SYS_STATE_active, so there is no race condition
> +         * possible between this code and vpci_process_pending. So, neither
> +         * vpci_process_pending may try to acquire the lock in read mode and
> +         * also destroy pdev->vpci in its error path nor pdev may be disposed
> +         * yet. This means that it is not required to check if the relevant
> +         * pdev->vpci still exists after re-acquiring the lock.
> +         */

> I'm not sure why you need to mention vpci_process_pending here:
> apply_map and defer_map are mutually exclusive, so given the current
> code it's impossible to get in a situation where apply_map is called
> while there's pending work on the vCPU (ie: v->vpci.mem != NULL).
>
> Also there's no need for a FIXME tag: the current approach doesn't
> require any fixes unless we start using apply_map in a different
> context.
>
> Hence I think the comment should be along the lines of:
>
> /*
>  * It's safe to drop and reacquire the lock in this context without
>  * risking pdev disappearing because devices cannot be removed until the
>  * initial domain has been started.
>  */
This sounds good, will use this
> 
> Thanks, Roger.