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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2021 16:11, Oleksandr Andrushchenko wrote:
> 
> 
> 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:

I have to admit I'm not sure. It might be easier, but it may also be
less desirable.

> 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...

Not just this. The -ERESTART handling isn't appropriate this way
either. For the moment I can't help thinking that draining would
be preferable over canceling.

Jan