Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2021 13:34, Oleksandr Andrushchenko wrote:
> Possible locking and other work needed:
> =======================================
> 
> 1. pcidevs_{lock|unlock} is too heavy and is per-host
> 2. pdev->vpci->lock cannot be used as vpci is freed by vpci_remove_device
> 3. We may want a dedicated per-domain rw lock to be implemented:
> 
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 28146ee404e6..ebf071893b21 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -444,6 +444,7 @@ struct domain
> 
>   #ifdef CONFIG_HAS_PCI
>       struct list_head pdev_list;
> +    rwlock_t vpci_rwlock;
> +    bool vpci_terminating; <- atomic?
>   #endif
> then vpci_remove_device is a writer (cold path) and vpci_process_pending and
> vpci_mmio_{read|write} are readers (hot path).

Right - you need such a lock for other purposes anyway, as per the
discussion with Julien.

> do_physdev_op(PHYSDEVOP_pci_device_remove) will need 
> hypercall_create_continuation
> to be implemented, so when re-start removal if need be:
> 
> vpci_remove_device()
> {
>    d->vpci_terminating = true;
>    remove vPCI register handlers <- this will cut off PCI_COMMAND emulation 
> among others
>    if ( !write_trylock(d->vpci_rwlock) )
>      return -ERESTART;
>    xfree(pdev->vpci);
>    pdev->vpci = NULL;
> }
> 
> Then this d->vpci_rwlock becomes a dedicated vpci per-domain lock for
> other operations which may require it, e.g. virtual bus topology can
> use it when assigning vSBDF etc.
> 
> 4. vpci_remove_device needs to be removed from vpci_process_pending
> and do nothing for Dom0 and crash DomU otherwise:

Why is this? I'm not outright opposed, but I don't immediately see why
trying to remove the problematic device wouldn't be a reasonable course
of action anymore. vpci_remove_device() may need to become more careful
as to not crashing, though.

Jan