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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 18.11.21 16:35, Jan Beulich wrote:
> On 18.11.2021 15:14, Oleksandr Andrushchenko wrote:
>> On 18.11.21 16:04, Roger Pau Monné wrote:
>>> Indeed. In the physdevop failure case this comes from an hypercall
>>> context, so maybe you could do the mapping in place using hypercall
>>> continuations if required. Not sure how complex that would be,
>>> compared to just deferring to guest entry point and then dealing with
>>> the possible cleanup on failure.
>> This will solve one part of the equation:
>>
>> pci_physdev_op
>>          pci_add_device
>>              init_bars -> modify_bars -> defer_map -> 
>> raise_softirq(SCHEDULE_SOFTIRQ)
>>          iommu_add_device <- FAILS
>>          vpci_remove_device -> xfree(pdev->vpci)
>>
>> But what about the other one, e.g. vpci_process_pending is triggered in
>> parallel with PCI device de-assign for example?
> Well, that's again in hypercall context, so using hypercall continuations
> may again be an option. Of course at the point a de-assign is initiated,
> you "only" need to drain requests (for that device, but that's unlikely
> to be worthwhile optimizing for), while ensuring no new requests can be
> issued. Again, for the device in question, but here this is relevant -
> a flag may want setting to refuse all further requests. Or maybe the
> register handling hooks may want tearing down before draining pending
> BAR mapping requests; without the hooks in place no new such requests
> can possibly appear.
This can be probably even easier to solve as we were talking about
pausing all vCPUs:

void vpci_cancel_pending(const struct pci_dev *pdev)
{
     struct domain *d = pdev->domain;
     struct vcpu *v;
     int rc;

     while ( (rc = domain_pause_except_self(d)) == -ERESTART )
         cpu_relax();

     if ( rc )
         printk(XENLOG_G_ERR
                "Failed to pause vCPUs while canceling vPCI map/unmap for %pp 
%pd: %d\n",
                &pdev->sbdf, pdev->domain, rc);

     for_each_vcpu ( d, v )
     {
         if ( v->vpci.map_pending && (v->vpci.pdev == pdev) )

This will prevent all vCPUs to run, but current, thus making it impossible
to run vpci_process_pending in parallel with any hypercall.
So, even without locking in vpci_process_pending the above should
be enough.
The only concern here is that domain_pause_except_self may return
the error code we somehow need to handle...
> Jan
>
Thank you,
Oleksandr